-
-
Notifications
You must be signed in to change notification settings - Fork 98
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
Reduce overhead in ManagedNamedPipeClient
during connection loop
#237
Conversation
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.
Co-authored-by: Philipp <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
The implementation of
NamedPipeClientStream
has 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.Sleep
sleep time, and the overhead seen here is actual CPU time being used from spinlocking.Tested on Windows, macOS and linux.