-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[API Proposal]: add direct buffer access to SocketAddress #78757
Comments
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label. |
Tagging subscribers to this area: @dotnet/ncl Issue DetailsBackground and motivationCurrently, Only read access is being considered here; it is not seen as necessary (by me, at least) to support write access (which would make the internal hash optimization awkward), nor to support storage between calls - hence API ProposalAdd a span accessor (note: naming is hard!): public ReadOnlySpan<byte> Span => new ReadOnlySpan<byte>(Buffer, 0, InternalSize); This enforces all the same characteristics of the existing API UsageSocketAddress addr = ...
var bytes = addr.Span;
// use bytes (contrast to) SocketAddr addr = ...
var bytes = /* your choice of temporary buffer here, using addr.Size */
for (int i = 0 ; i < addr.Size; i++)
{
bytes[i] = addr[i];
}
// use bytes Alternative Designs
RisksA caller could use Using a simple span accessor dictates that the memory has a contiguous representation; this restricts future flexibility for refactoring, although this seems pretty unlikely. But maybe a Not a risk, but additionally,
|
Performance characteristics; data first
code (note: context here is talking via P/Invoke, so the using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Net;
using System.Reflection;
using System.Runtime.CompilerServices;
BenchmarkRunner.Run<Scenario>();
public sealed class SpoofedSocketAddress // like SocketAddress, but with the new API
{
internal int InternalSize;
internal byte[] Buffer;
public SpoofedSocketAddress(SocketAddress value)
{
// just copy out the raw values
InternalSize = (int)typeof(SocketAddress).GetField(nameof(InternalSize), BindingFlags.NonPublic | BindingFlags.Instance)!.GetValue(value)!;
Buffer = (byte[])typeof(SocketAddress).GetField(nameof(Buffer), BindingFlags.NonPublic | BindingFlags.Instance)!.GetValue(value)!;
}
public ReadOnlySpan<byte> Span => new ReadOnlySpan<byte>(Buffer, 0, InternalSize);
public int Size
{
get
{
return InternalSize;
}
}
public byte this[int offset]
{
get
{
if (offset < 0 || offset >= Size)
{
throw new IndexOutOfRangeException();
}
return Buffer[offset];
}
}
}
[SimpleJob]
public class Scenario
{
private SpoofedSocketAddress? addr;
[GlobalSetup]
public void Setup()
{
var endpoint = new IPEndPoint(V6 ? IPAddress.IPv6Loopback : IPAddress.Loopback, 8080);
addr = new SpoofedSocketAddress(endpoint.Serialize());
}
[Params(false, true)]
public bool V6 { get; set; }
[Benchmark(Baseline = true)]
[SkipLocalsInit] // to be as fair as possible
public unsafe void Indexer()
{
var addr = this.addr!;
var len = addr.Size;
byte* buffer = stackalloc byte[len];
for (int i = 0; i < len; i++)
{
buffer[i] = addr[i];
}
DoTheThing(buffer, len);
}
[Benchmark]
public unsafe void Span()
{
var span = this.addr!.Span;
fixed (byte* ptr = span)
{
DoTheThing(ptr, span.Length);
}
}
// in real scenario, this would be extern
[MethodImpl(MethodImplOptions.NoInlining)]
static unsafe void DoTheThing(byte* ptr, int len) { }
} |
I don't see much value in the public abstract class EndPoint
{
// Existing:
// public virtual SocketAddress Serialize();
// public virtual EndPoint Create(SocketAddress socketAddress);
public virtual bool TryGetSocketAddressSize(out int size);
public virtual void Serialize(Span<byte> socketAddress);
public virtual EndPoint Create(ReadOnlySpan<byte> socketAddress);
}
// Usage
if (endPoint.TryGetSocketAddressSize(out int size))
{
Span<byte> socketAddress = size < 256 ? stackalloc byte[size] : new byte[size];
endPoint.Serialize(socketAddress);
// use socketAddress
} This could enable us getting rid of many internal @mgravell would such an API be more usable for you as well, or you prefer to work with |
How do you intend to use is @mgravell? While Now, I assume the And you you care about families beyond It seems like Kestrel may get the |
No, this is a different overload for the existing virtual
Successful
This way wouldn't be possible to delegate the buffer allocation to the user. With |
@wfurt the specific scenario I had in mind was "demikernel", which has similar API surface to windsock in some regards, including the address layout; if we wanted to go direct from EndPoint (or at least IPEndPoint) to the byte representation, perhaps with a little help from SocketPal, that would also work; I was initially thinking in terms of avoiding the CPU work of fetch, but avoiding the instance too has some merit. |
@mgravell can you elaborate what do you mean by this? Edit: Do you need to query particular info from the |
@antonfirsov IIRC (I might be misremembering the code) the actual byte population of Buffer is handed to SocketPal. If I'm wrong there, mea culpa - but basically what I'm after is the same fundamental bytes that SocketAddress exposes via the combination of Size and indexer - the bytes commonly handed to the underlying socket APIs |
@mgravell yes but it's an implementation detail in Out of curiosity: where did you encounter this bottleneck in your investigation? Can you show some context/code for your scenario? |
I've hit this problem while writing hyper-v socket endpoints, https://github.com/Wraith2/HyperVSockets/blob/dff2d010cb916976fbeba5ab7c323d2a19c6bf8f/HyperVEndPoint.cs#L35 Needing to do byte copies back and forth through the accessor was a minor annoyance so I decided not to open issue since it didn't appear that anyone else was having a problem. Now that someone else is I've added my 👍 |
@antonfirsov I'm not claiming it is a super bottleneck in Kestrel specifically; indeed, I haven't even got far enough along to get things compiling, let alone working - it simply stood out at me as a particularly striking API gap. For network code, getting this particular serialized byte form is pretty common, and right now, if you're not blessed by IVTA, the way of doing it is incredibly indirect. For TCP, this is indeed unlikely to be a tight pinch, as TCP connections are usually reasonably long lived. For UDP, this is much more noticeable, as this byte sequence is needed per packet. And yes, you could extract and store it, but now you've got yet another copy of everything. Mostly, I was thinking in terms of "paper cuts": here's an API that is demanded by networking scenarios, that could be hugely improved with a one-line addition. |
A I would add a new constructor |
The questions is if I would not mind to add some more functions directly to And there are cases like #78448: The size differs between actual length vs maximum needed. I would not mind the API as proposed either @mgravell but I want to make sure it solves some real problem. (small preference on |
This was addressed in .NET 8. |
Background and motivation
Currently,
SocketAddress
has publicint Size
andbyte this[int index]
accessors; if you need the data, accessing the underlying contents (without IVTA) means using your own buffer of the.Size
, and then looping to copy the data out. This is inefficient and unnecessary.Only read access is being considered here; it is not seen as necessary (by me, at least) to support write access (which would make the internal hash optimization awkward), nor to support storage between calls - hence
ReadOnlySpan<byte>
seems ideal.Originating context: Kestrel (aspnetcore) investigation into socket overheads and perf/scalability, looking into alternative underlying implementations.
API Proposal
Add a span accessor (note: naming is hard!):
This enforces all the same characteristics of the existing
get
accessor, but allows the contents to be accessed much more efficiently.API Usage
(contrast to)
Alternative Designs
AsSpan()
method instead of property getter?CopyTo(Span<byte>)
, but that still forces a copy, rather than in-place direct usageRisks
A caller could use
unsafe
/Unsafe
to access the underlying buffer; changing the contents is already allowed (by the indexerset
accessor), so that isn't by itself an additional risk - however, this would not trigger the hash-code optimization path. This, though, seems a case of "do stupid things, win stupid prizes", i.e. "don't do that". As soon asunsafe
/Unsafe
were used, all guarantees were gone.Using a simple span accessor dictates that the memory has a contiguous representation; this restricts future flexibility for refactoring, although this seems pretty unlikely. But maybe a
bool TryGetSpan(out ReadOnlySpan<byte>)
(orTryGetBuffer
, naming) would be more future-proof there? so: some hypothetical future implementation can at least say "nope" without faulting?Not a risk, but additionally,
Buffer
andInternalSize
are currentlyinternal
rather thanprivate
- I wonder how much code that uses those could be updated to use this new API, and perhaps benefit from bounds-check elision (a loop that usesInternalSize
will not benefit, but a loop over a right-sized span: will).The text was updated successfully, but these errors were encountered: