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

Unnecesary use of GCHandle in Windows Socket code #86513

Closed
wfurt opened this issue May 19, 2023 · 1 comment · Fixed by #89808
Closed

Unnecesary use of GCHandle in Windows Socket code #86513

wfurt opened this issue May 19, 2023 · 1 comment · Fixed by #89808
Assignees
Labels
area-System.Net.Sockets bug in-pr There is an active PR which will close this issue when it is merged os-windows tenet-performance Performance related issue
Milestone

Comments

@wfurt
Copy link
Member

wfurt commented May 19, 2023

This was reported by customer that application can crash with OoMs because memory can become highly fragmented because of small chunks of pinned memory. That generally points to socketAddress in ConnectEx. (used by Socket.ConnectAsync)

At the moment we hold the GCHandle even after the Connect is finished through life of the Socket. That can be fixed easily but furthermore it does not seems to be needed at all.

For ConnextEx and WSASendTo the socket address is documented as [In] and needs to be preserved only through initial call and not through life of the overlapped IO. (confirmed with Windows networking team) We can use simple fixed instead of allocating GCHandle and pinning the memory in heavy way.

The WSAReceiveFrom is different as the SocketAddress isout and sockerAddressLength is [in,out]. We may use NativeMemory for that as the current logic with caching GCHanlde may not work anyway. The ReceiveFrom will likely receive packets from various sources (for simple duplex simple Receive can be used) and we will keep allocating GCHandle for (almost) every packet.

While the first part should be easy to fix (and TCP gets most use) solving the UDP receiving may take some more effort. That would also contribute to #30797.

@wfurt wfurt added this to the 8.0.0 milestone May 19, 2023
@wfurt wfurt self-assigned this May 19, 2023
@ghost
Copy link

ghost commented May 19, 2023

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

Issue Details

This was reported by customer that application can crash with OoMs because memory can become highly fragmented because of small chunks of pinned memory. That generally points to socketAddress in ConnectEx. (used by Socket.ConnectAsync)

At the moment we hold the GCHandle even after the Connect is finished through life of the Socket. That can be fixed easily but furthermore it does not seems to be needed at all.

For ConnextEx and WSASendTo the socket address is documented as [In] and needs to be preserved only through initial call and not through life of the overlapped IO. (confirmed with Windows networking team) We can use simple fixed instead of allocating GCHandle and pinning the memory in heavy way.

The WSAReceiveFrom is different as the SocketAddress isout and sockerAddressLength is [in,out]. We may use NativeMemory for that as the current logic with caching GCHanlde may not work anyway. The ReceiveFrom will likely receive packets from various sources (for simple duplex simple Receive can be used) and we will keep allocating GCHandle for (almost) every packet.

While the first part should be easy to fix (and TCP gets most use) solving the UDP receiving may take some more effort. That would also contribute to #30797.

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Sockets, os-windows

Milestone: 8.0.0

@karelz karelz added bug tenet-performance Performance related issue labels May 31, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 1, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Sockets bug in-pr There is an active PR which will close this issue when it is merged os-windows tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants