Minor perf improvements to Sockets #21051
Conversation
Shaves a small but measurable amount off each StartConfiguring call.
It's not 100% the same logic as before; previously, if an operation was in progress while Dispose was called, and then another operation was issued while that previous operation was still in progress, this would throw an OperationDisposedException, whereas now it'll throw an InvalidOperationException until the previous operation completes, then it'll throw an OperationDisposedException. But a) there are race conditions here, so code doing this needs to be able to handle both exceptions for the same failure, and b) the previous logic was inconsistent with the exact same kind of thing in StartConfiguring, for no good reason. This brings them in sink, and shares the same code, also reducing the size of StartOperationCommon.
We only need to update the socket if it's changed.
Move most of the implementation to a "slow" helper
- Factor out duplicated blobs for logging into a single location - Remove cases that are unnecessary after logging change - Avoid unnecessary status check / branch for operations that don't change it (only accept/connect do)
| @@ -33,52 +33,56 @@ public ThreadPoolBoundHandle IOCPBoundHandle | |||
| // Binds the Socket Win32 Handle to the ThreadPool's CompletionPort. | |||
| public ThreadPoolBoundHandle GetOrAllocateThreadPoolBoundHandle(bool trySkipCompletionPortOnSuccess) | |||
| { | |||
| // Check to see if the socket native _handle is already | |||
| // bound to the ThreadPool's completion port. | |||
| if (_released || _iocpBoundHandle == null) | |||
There was a problem hiding this comment.
Could we simplify this to a single test? Is _iocpBoundHandle always null when _released == true? Seems like it should be...
There was a problem hiding this comment.
Maybe with other refactorings or changes, but at present it doesn't look like it... _iocpBoundHandle starts out as null and is then lazily initialized to non-null, after which point it's never set back to null. From the comments in the code, it looks like this is done to support freeing a NativeOverlapped after disposal.
There was a problem hiding this comment.
Makes sense.
Another idea: is there any harm in just returning the existing iocp-bound handle if _released is true and _iocpBoundHandle is not null?
Don't know if any of this matters in practice, so don't block on this...
There was a problem hiding this comment.
You mean changing the condition from:
if (_released || _iocpBoundHandle == null) to instead just be:
if (_iocpBoundHandle == null) such that we don't throw the ObjectDisposedException from here but instead presumably have it throw some kind of exception when the handle is actually used?
Sounds reasonable, but I've not tried it. Not sure if we have tests for that, either. But it'd be worth checking on subsequently.
|
Generally looks great; one small comment above. |
The jit is fairly tolerant of byref/native int mismatches for inline arguments and return values. And some of the new unsafe helpers do this kind of reinterpretation across call boundaries as well. This sometimes leads the jit to lose track of byrefs. A general fix for this is likely somewhat involved. For now we simply detect if we're about to lose track of a byref when substituting a particular kind of return expression, and retype the expression to a byref. Fixes #21051. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
The jit is fairly tolerant of byref/native int mismatches for inline arguments and return values. And some of the new unsafe helpers do this kind of reinterpretation across call boundaries as well. This sometimes leads the jit to lose track of byrefs. A general fix for this is likely somewhat involved. For now we simply detect if we're about to lose track of a byref when substituting a particular kind of return expression, and retype the expression to a byref. Fixes dotnet#21051. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Shaves a few percent off various socket operations. It's close to noise, but appears to be statistically significant and amount to around 2-3% improvement in microbenchmarks that repeatedly receive/send (in which case receives complete asynchronously) and send/receive (in which case most if not all operations complete synchronously).
(At this point most of the overhead associated with sockets (at least in the Windows build) is either in the P/Invokes to native or in peanut butter spread across lots of functions with very low exclusive amounts per function. There's likely a decent amount more to be done on Linux, where we've done less profiling and where we know there are improvements to be made around asynchronous completion and dispatch.)
cc: @geoffkizer, @CIPop, @davidsh, @vancem