Drop unnecessary fields from SocketAsyncEventArgs

[benaadams]

Shrinks the size by 3 pointers on Windows (28 bytes)

Moving the 2 x WSABuffer[]?, 1 x byte[]? to stack rather than heap.

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Shrinks the size by one pointer

Author:	benaadams
Assignees:	-
Labels:	area-System.Net.Sockets
Milestone:	-

[stephentoub]

Isn't the pointer being passed to an overlapped I/O operation? How do we know it's not needed by the operation past the synchronous call?

[benaadams]

Isn't the pointer being passed to an overlapped I/O operation? How do we know it's not needed by the operation past the synchronous call?

Docs specify it can be a stack pointer? https://docs.microsoft.com/en-us/windows/win32/api/mswsock/nc-mswsock-lpfn_wsarecvmsg

If this function is completed in an overlapped manner, it is the Winsock service provider's responsibility to capture this WSABUF structure before returning from this call. This enables applications to build stack-based WSABUF arrays pointed to by the lpBuffers member of the WSAMSG structure pointed to by the lpMsg parameter.

[benaadams]

WSAMsg can probably be stack also...

[stephentoub]

Docs specify it can be a stack pointer

Cool. Makes me a little sad it took us this long to figure that out, but thank you for doing so now :)

[benaadams]

Docs specify it can be a stack pointer

Cool. Makes me a little sad it took us this long to figure that out, but thank you for doing so now :)

Was already doing it in once place:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs

Lines 400 to 406 in 9bb0427

	var wsaBuffer = new WSABuffer { Length = _count, Pointer = (IntPtr)(bufferPtr + _offset) };
	SocketFlags flags = _socketFlags;
	SocketError socketError = Interop.Winsock.WSARecv(
	handle,
	&wsaBuffer,
	1,

[davidfowl]

This is a very nice change. We should do the array pool change for linux as well when we bust the stack alloc limit

[benaadams]

Not entirely sure why the interface is not being set on the PacketInformation currently 🤔

[geoffkizer]

Some general thoughts here:

First, doing stackalloc for WSABUF up to some limit for muti-buffer send/recv is goodness. And 16 seems like a reasonable limit. @tmds already made this change for Linux (though it's in a very different spot in the code) -- I think his limit was 8.

Second, I'm a little wary of removing _wsaBufferArrayPinned entirely. The general philosophy of SAEA is to provide "amortized zero-allocation" for socket IO. As such, we cache the WSABUF array today so that it can hopefully be reused on subsequent requests; we will reallocate if necessary (i.e. you need more WSABUF entries than are available), but we always cache it.

Maybe using ArrayPool here is a reasonable alternative, but I'm not sure. This seems like a case where ArrayPool reuse will be small and may be more trouble than it's worth. And if we are using ArrayPool, we need to be super confident we are not introducing any use-after-free issues.

Third, another approach to shrinking SAEA would be to push some of its fields into "tear off" objects, particularly fields used by less common operations like ReceiveMessageFrom or SendPackets. The long-term plan here is to refactor SAEA in such a way that we can ensure individual operations only need to allocate the state they need. So some sort of tear-off model seems like a step in that direction.

[benaadams]

Found the bug, unlike the WSABuffers what the controlBuffer WSABUF points at has to remain valid until the response completes (much like the data buffers that are pointed at)

[benaadams]

Second, I'm a little wary of removing _wsaBufferArrayPinned entirely. The general philosophy of SAEA is to provide "amortized zero-allocation" for socket IO. As such, we cache the WSABUF array today so that it can hopefully be reused on subsequent requests; we will reallocate if necessary (i.e. you need more WSABUF entries than are available), but we always cache it.

The WSABuffer's only need to be fixed for the short duration of the submit call (they don't need to remain either valid, allocated or pinned waiting for the result); so maintaining a perminatly pinned buffer for potentially the lifetime of the app as soon as a multi-buffer call is made seems excessive, when they might be rare rather than frequent calls?

That might make a case for a non-pinned array; however if you are making calls with more than 16 buffers is the rent and return from the ArrayPool going to be significant vs the IO time? The ArrayPool meaning the buffers are shared across all SocketAsyncEventArgs rather than being lodged in individual ones.

[davidfowl]

I think the fact that WSABufs only need to exist for the synchronous part of the send it's ideal usage of array pool and we get maximum sharing across multiple writes

[benaadams]

Looks like I broke something in my quick tidy up before pushing 😥

[geoffkizer]

Ok, you convinced me. Let's kill _wsaBufferArrayPinned.

[geoffkizer]

@benaadams Where does this PR stand?

[benaadams]

@benaadams Where does this PR stand?

I made a clean up on the stack allocation; but seem to be getting stack corruption now. However can't build anything in .NET6.0 in VS to look deeper currently dotnet/sdk#17461

[karelz]

@benaadams would it make sense to flip the PR to Draft until it is unblocked for further iterations? Thanks!

[benaadams]

@benaadams would it make sense to flip the PR to Draft until it is unblocked for further iterations? Thanks!

Depend how long you think it will be till sdk unbreaks VS? 😅

[karelz]

@benaadams Depend how long you think it will be till sdk unbreaks VS? 😅

No idea honestly. If you're blocked on it for more than a week, I think it is time to swap it to Draft and wait. It can be then easily swapped back ;)
Thanks!

[karelz]

@benaadams looks like a week passed ... perhaps time to switch it to Draft until we can make it actionable? Thanks!

[ghost]

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.