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

Replace single-item pinned buffers in System.Net.WebSockets.Client with PinnableBufferCache. #14911

Closed
pgavlin opened this issue Jul 24, 2015 · 10 comments
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions tenet-performance Performance related issue
Milestone

Comments

@pgavlin
Copy link
Contributor

pgavlin commented Jul 24, 2015

No description provided.

@benaadams
Copy link
Member

Can you also maintain the overloads or equivalents that exist in 4.5.2/4.6 where the internal buffers can be provided?

e.g. AcceptWebSocketAsync(string subProtocol, int receiveBufferSize, TimeSpan keepAliveInterval, ArraySegment internalBuffer)

With application domain knowledge of message size, message frequency, connected client count etc, I'd prefer to pre-allocate a very large buffer into LoH and out of the generations, pin the whole thing and chop it into ArraySegments for use either as send/receive buffers

@benaadams
Copy link
Member

Also means with careful buffer alignments can cut out a data copy with dotnet/coreclr#36

@davidsh
Copy link
Contributor

davidsh commented Aug 8, 2015

cc: @CIPop @SidharthNabar

@CIPop
Copy link
Member

CIPop commented Aug 10, 2015

/cc: @vancem

@benaadams Thank you for your feedback! The pattern that you describe is mostly correct and is still possible without the added complexity of the APIs accepting a buffer (or buffer size). The only part that should be left out of application code is pinning the buffer: this will be performed once, within the first call using that buffer.

At this moment, the implementation for ClientWebSocket is caching the pinned send/receive buffers. If subsequent operations will reuse the buffers, no re-pinning will be performed internally. In the case you describe, you would only have one buffer (and multiple ArraySegments) which means that pinning is done exactly once.

dotnet/corefx#2505 is making this even better by adding internal use of PinnableBufferCache. This will allow caching pinned buffers for cases where the application is not following the recommended pattern you described.

Regarding dotnet/coreclr#36: This is good to know although I'm not sure this would apply to ClientWebSocket. We've removed the memory copy step: instead we pass the data buffers directly to the underlying Win32/WinHttp layer.

@benaadams
Copy link
Member

Ah yes, sorry, so this would be for server websockets which isn't in yet - am a little premature with my feedback and commenting on wrong feature 😈

@CIPop
Copy link
Member

CIPop commented Aug 10, 2015

/cc: @Tratcher

@benaadams Actually the comments fit well for both sides. For server side you should start following https://github.com/aspnet/. The Server-side WebSockets require close integration with the server they run on and so will most likely depend on the service's host (WebListener or IIS for the Windows implementation).

@SidharthNabar
Copy link

@benaadams - Since your request was intended for server WebSockets which is not tracked in this repo, I am closing this issue. Please open an issue in the ASP.NET 5 repo that Cristian mentioned above.

@davidsh
Copy link
Contributor

davidsh commented Feb 28, 2016

Re-opening this issue. It was closed incorrectly.

Since your request was intended for server WebSockets which is not tracked in this repo, I am closing this issue. Please open an issue in the ASP.NET 5 repo that Cristian mentioned above.

This issue is for the System.Net.WebSocket.Client code.

@davidsh davidsh reopened this Feb 28, 2016
@davidsh davidsh self-assigned this Feb 28, 2016
@davidsh davidsh removed their assignment Nov 18, 2016
@karelz
Copy link
Member

karelz commented Dec 5, 2016

This may be a bit involved - we need to check PinnableBufferCache if it is fully implemented for our purpose here.

@davidsh
Copy link
Contributor

davidsh commented Aug 31, 2017

Closing this issue since PinnableBufferCache is now being replaced with Span and other higher performance choices.

@davidsh davidsh closed this as completed Aug 31, 2017
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests

7 participants