Runtime: Proposal: Memory and Span friendly overloads in Ping class

[sakno]

Explanation: MTU = Maximum Transmission Unit

I'm using Ping class for path MTU discovery using one of the proving method described here. My implementation sending packets with different MTU in a loop. Each iteration requires allocation of byte array in the heap. As a result, it produces a lot of garbage. It would be great to have Memory- and Span-friendly overloads that allow to reuse the same buffer.

public class Ping
{
  public void SendAsync(String hostNameOrAddress, TimeSpan timeout, ReadOnlyMemory<byte> buffer, PingOptions options, object userToken);
  public void SendAsync(IPAddress address, TimeSpan timeout, ReadOnlyMemory<byte> buffer, PingOptions options, object userToken);
  public void SendAsync(string hostNameOrAddress, TimeSpan timeout, ReadOnlyMemory<byte> buffer, object userToken);
  public void SendAsync(IPAddress address, TimeSpan timeout, ReadOnlyMemory<byte> buffer, object userToken);
  public PingReply Send(string hostNameOrAddress, TimeSpan timeout, ReadOnlySpan<byte> buffer, PingOptions options);
  public PingReply Send(IPAddress address, TimeSpan timeout, ReadOnlySpan<byte> buffer, PingOptions options);
  public PingReply Send(string hostNameOrAddress, TimeSpan timeout, ReadOnlySpan<byte> buffer);
  public PingReply Send(IPAddress address, TimeSpan timeout, ReadOnlySpan<byte> buffer);
  public Task<PingReply> SendPingAsync(string hostNameOrAddress, TimeSpan timeout, ReadOnlyMemory<byte> buffer, PingOptions options);
  public Task<PingReply> SendPingAsync(IPAddress address, TimeSpan timeout, ReadOnlyMemory<byte> buffer, PingOptions options);
  public Task<PingReply> SendPingAsync(string hostNameOrAddress, TimeSpan timeout, ReadOnlyMemory<byte> buffer);
  public Task<PingReply> SendPingAsync(IPAddress address, TimeSpan timeout, ReadOnlyMemory<byte> buffer);  
}

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

[sakno]

I'm ready to fix that and create pull request if you will mark this issue with up-for-grabs label.

[davidsh]

Thank you for the issue. It looks like you are proposing several new APIs for the Ping class. Before you can submit a PR, these new APIs need to be reviewed and approved with our API review process.

[msftbot[bot]]

Tagging @dotnet/ncl as an area owner. If you would like to be tagged for a label, please notify danmosemsft.

[davidsh]

@scalablecory

[sakno]

@davidsh , I need to wait for approve, right?

[davidsh]

@davidsh , I need to wait for approve, right?

Yes. It needs to be formally reviewed and marked as "api-approved".

[danmosemsft]

@sakno could you give a sample usage? How is this better than just pooling the buffers you pass in? Are you trying to pass views over one large buffer?

[sakno]

@danmosemsft , the example in my gist which is specified in the original post. Path MTU discovery algorithm uses half-interval search between lowest and highest possible MTU values. As a result, it's not possible to pre-allocate byte arrays. ArrayPool<byte> can't help me because I need precise length of the array. These overloads can help me because then I able to use ArrayPool<byte> and allocate buffer once and pass slices of the same array between iterations.

[svick]

• Is there a reason for including SendAsync overloads, when we have SendPingAsync?
• Should the new overloads of SendPingAsync return ValueTask<PingReply>? It has the potential to avoid Task-related allocations.

[sakno]

@svick , usage of ValueTask is useless due to request-reply nature of ICMP Echo. When using sockets, sending requests and waiting for the reply in 99,9999% cases will never be completed synchronously. Under the hood, the current implementation uses Socket.ReceiveFromAsync which has Task return type. Of course, you can derive from SocketAsyncEventArgs and use ManualResetValueTaskSourceCore to organize pool of such instances. IMHO, this approach adds a lot of complexity. Holding pool of SocketAsyncEventArgs probable have the same memory pressure as Task allocation.

[svick]

@sakno ValueTask-returning Socket.ReceiveFromAsync has been accepted (https://github.com/dotnet/runtime/issues/33418), so it's likely it will be in .Net 5. And while using ManualResetValueTaskSourceCore manually does add some complexity, if the return type is Task, you close that door forever. Also, if https://github.com/dotnet/runtime/issues/13633 is enabled, implementing allocation-avoiding ValueTask methods becomes trivial.

[sakno]

@svick , I think that ReceiveMessageFromAsync with ValueTask can be completed synchronously only if the incoming packet has been received and already placed to OS packet scheduler. In case of ICMP echo it will always be false just because CPU is faster that network and at the time of ReceiveMessageFromAsync call, the response is not yet received.

In other words, ReceiveMessageFromAsync can be completed synchronously if you are receiving stream of packets (RTP or UDP) and speed of such reading is equal or less than speed of packets arrival.

[karelz]

Just a heads up that Ping on Linux will spin off a new process per call if you are not admin:
https://github.com/dotnet/runtime/blob/f498a3171b65215ae3176598869233fee8e4da58/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Unix.cs#L25-L29
In such scenario, I don't think the perf overhead of an allocation matters.

In general, I wonder how common is the scenario where the overloads would benefit. I am a bit torn between adding more APIs for very corner-case scenarios vs. easy to add. @wfurt @scalablecory what are your thoughts?

[scalablecory]

I'd be okay with some spannified overloads here for consistency with rest of BCL.

I would remove the SendAsync overloads, and consider (if implementation allows) removing the timeout parameter in favor of a CancellationToken.

[karelz]

Triage: We would love to see some more data how important this is in the scenarios -- even if it is mostly for Windows / elevated Linux.
@sakno can you share how much it showed up in your cases? Which OS do you use? Windows or elevated Linux?

[sakno]

@karelz , my primary OS is Linux.

I agree with @scalablecory that CancellationToken should be preferred over timeout. According with my work in #35461 I see no blockers, even for unprivileged user on Linux. However, it requires a lot of work.

[GSPP]

Should the timeout parameters be TimeSpan? If not it might be better to put the unit in the parameter name (e.g. timeoutMilliseconds).

[sakno]

@GSPP , the proposed API is updated. Thanks!