Skip to content
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

Do not overallocate SocketAddress.Buffer #78860

Merged
merged 7 commits into from
Dec 19, 2022

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Nov 25, 2022

We build SocketAddress.cs in two modes: to get the public type in System.Net.Primitives and - independently - as a System.Net.Sockets internal, so we can access internal members, and work withe the extra bytes allocated at the end of SocketAddress.Buffer for WSARecvFrom which needs a pinned pointer to the socket address size. This duplication was a refactoring technique when Framework code was ported to Core where System.Net.Sockets and System.Net.Primtives live in separate dll-s and socket code cannot access SocketAddress internals.

There seem to be two obvious problems:

  1. Extra bytes are allocated in both compilation modes penalizing the public SocketAddress everywhere, including Unix platforms
  2. The formula to calculate the number of bytes to be allocated is inherited from early .NET Framework days, and seems to be wrong, or at least overly defensive:

Buffer = new byte[(size / IntPtr.Size + 2) * IntPtr.Size];

This allocates 32 bytes for a 16-byte IPv4 address for both SocketAddress and Internals.SocketAddress on a 64bit machine. The intention of the formula was probably to make sure that lpFromlen's buffer is 4-byte aligned (assuming 32 bit), but I don't see any signs in WSARecvFrom docs that there is a need for any alignment. Edit: updated the code reintroducing alignment (though with a "smarter" formula), see discussion.

@ghost
Copy link

ghost commented Nov 25, 2022

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

Issue Details

We build SocketAddress.cs in two modes: to get the public type in System.Net.Primitives and - independently - as a System.Net.Sockets internal, so we can access internals, and allocate some extra bytes for WSARecvFrom which needs a pinned pointer to the socket address size. This was a technique to help porting .NET Framework socket code to Unix.

There seem to be two obvious problems:

  1. Extra bytes are allocated in both compilation modes penalizing the public SocketAddress everywhere, including Unix platforms
  2. The formula to calculate the bytes to be allocated in Internals.SocketAddress is inherited from early .NET Framework days, and seems to be wrong:

Buffer = new byte[(size / IntPtr.Size + 2) * IntPtr.Size];

With this formula we allocate 32 bytes for a 16-byte IPv4 address for both SocketAddress and Internals.SocketAddress on a 64bit machine. The intention of the formula was probably to make sure that lpFromlen's buffer is 4-byte aligned (probably assuming 32 bit), but I don't see any signs in WSARecvFrom docs that there is a need for any alignment.

Author: antonfirsov
Assignees: -
Labels:

area-System.Net.Sockets

Milestone: -

@wfurt
Copy link
Member

wfurt commented Nov 29, 2022

The lpFromlen needs to be aligned to 32/64 boundary on ARM32/64. This is not requirement of the networking stack but more so limitation of the architecture that is not able to access misaligned int. (it it just less efficient on x86/x64)

@antonfirsov
Copy link
Member Author

antonfirsov commented Nov 30, 2022

@wfurt I did some reading on this, and my understanding is that for windows API we need to comply with the C standard which states in [3.2]:

requirement that objects of a particular type be located on storage boundaries with addresses that are particular multiples of a byte address

Meaning that for LPINT which is a pointer to a 32 byte INT, the pointer needs to be 32-byte aligned, regardless of the platform.

ddb50fc updated the code to add the minimum necessary alignment.

Scrap that, I realized that the above wording is not super clear, updated the code to always align to IntPtr.Size.

@antonfirsov
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

// WSARecvFrom needs a pinned pointer to the 32bit socket address size: https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-wsarecvfrom
// Allocate IntPtr.Size extra bytes at the end of Buffer ensuring IntPtr.Size alignment, so we don't need to pin anything else.
// The following forumla will extend 'size' to the alignment boundary then add IntPtr.Size more bytes.
size = (size + IntPtr.Size - 1) / IntPtr.Size * IntPtr.Size + IntPtr.Size;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int Extend(int size) => (size + IntPtr.Size - 1) / IntPtr.Size * IntPtr.Size + IntPtr.Size;

for (int i = 10; i < 30; i++) Console.Write($"{i,2} | ");
Console.WriteLine();
for (int i = 10; i < 30; i++) Console.Write($"{Extend(i),2} | ");

Will print:

10 | 11 | 12 | 13 | 14 | 15 | 16 | 17 | 18 | 19 | 20 | 21 | 22 | 23 | 24 | 25 | 26 | 27 | 28 | 29 |
24 | 24 | 24 | 24 | 24 | 24 | 24 | 32 | 32 | 32 | 32 | 32 | 32 | 32 | 32 | 40 | 40 | 40 | 40 | 40 |

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the old code did

10 | 11 | 12 | 13 | 14 | 15 | 16 | 17 | 18 | 19 | 20 | 21 | 22 | 23 | 24 | 25 | 26 | 27 | 28 | 29 |
24 | 24 | 24 | 24 | 24 | 24 | 32 | 32 | 32 | 32 | 32 | 32 | 32 | 32 | 40 | 40 | 40 | 40 | 40 | 40 |

Sensible address structure would already be aligned IMHO and we really just need to put int on boundary of its size (e.g. 4) and we need to make sure it fits to the buffer. The new code does it and now we do it only for Windows where it is used.

#if !SYSTEM_NET_PRIMITIVES_DLL && WINDOWS
// WSARecvFrom needs a pinned pointer to the 32bit socket address size: https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-wsarecvfrom
// Allocate IntPtr.Size extra bytes at the end of Buffer ensuring IntPtr.Size alignment, so we don't need to pin anything else.
// The following forumla will extend 'size' to the alignment boundary then add IntPtr.Size more bytes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: forumla -> formula

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@antonfirsov
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov
Copy link
Member Author

Outerloop failures are unrelated: #79731, #77414, #77012, #79820

@antonfirsov antonfirsov merged commit 62013d8 into dotnet:main Dec 19, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 18, 2023
@karelz karelz added this to the 8.0.0 milestone Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants