Reduce overhead in ManagedNamedPipeClient during connection loop
#237
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The implementation of `NamedPipeClientStream` has some egregious use of `SpinOnce`: https://github.com/dotnet/runtime/blob/d59af2cf097acb100ad6a9cba652be34be6f4a2e/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeClientStream.cs#L151-L155 This adds perceivable overhead to the connect process, which we really don't want on a background thread. It is especially noticeable when discord is not running and the reconnect process is constantly running in the background. To get around this, we can request a connect timeout of 0 ms from the underlying API. Checking the implementations, this seems like a valid method, as long as there is external retry logic in place (which there is in the local class). Tested on Windows, macOS and linux.
1 task
Phippe
reviewed
Jul 15, 2023
Co-authored-by: Philipp <[email protected]>
Lachee
approved these changes
Jul 16, 2023
There was a problem hiding this comment.
You're completely right, it does spin later anyways for connection. I'll approve this and increment the version :)
Thanks for the contribution! Really appreciate it. Let me know if you find any other overheads that could be improved. I had put this on a new thread so i didn't think it was such a issue but i suppose on lower end machines it can make plenty of difference.
|
Thanks @Lachee. Would appreciate if there is a nuget release for this in the near future so we don't have to locally hack around it in an ugly way! |
peppy
added a commit
to peppy/osu
that referenced
this pull request
Jul 16, 2023
DavidCarbon
added a commit
to DavidCarbon-SBRW/SBRW.Launcher.Core.Discord
that referenced
this pull request
Jul 17, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The implementation of
NamedPipeClientStreamhas some egregious use ofSpinOnce:https://github.com/dotnet/runtime/blob/d59af2cf097acb100ad6a9cba652be34be6f4a2e/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeClientStream.cs#L151-L155
This adds perceivable overhead to the connect process, which we really don't want on a background thread. It is especially noticeable when discord is not running and the reconnect process is constantly running in the background.
To get around this, we can request a connect timeout of 0 ms from the underlying API. Checking the implementations, this seems like a valid method, as long as there is external retry logic in place (which there is in the local class).
Over 5 failed connection attempts:
Note that this profiled view is considering thread running time, not all time. It does not include actual
Thread.Sleepsleep time, and the overhead seen here is actual CPU time being used from spinlocking.Tested on Windows, macOS and linux.