Reduce allocations in SslStream reading/writing by ~30% #13274
Conversation
Saves an allocation per read and write, ammortizing the costs across the whole stream's lifetime.
|
|
||
| internal void CompleteUser(object userResult) | ||
| internal void CompleteUser(int userResult) | ||
| { |
There was a problem hiding this comment.
nit: capitalization
Of what?
There was a problem hiding this comment.
Oh, in the debug message below? GitHub wasn't showing that on the comment view. Will lower-case the R.
There was a problem hiding this comment.
Yes, sorry for the confusion, I meant the R in the comment below.
|
LGTM. Nice work. I do think that longer-term we should revisit the async implementation pattern here and elsewhere. There are likely more wins to be had here. Even with your improvements, async objects account for ~80% of allocated bytes. |
Absolutely. As you suggest, this is trying to make the best of the current design. Note that the SecBufferDesc allocation per operation can also be removed with a bit of work (see my comments in #12935), at which point all allocations per operation will be due to asynchrony. Once that's removed, we'll essentially have two allocations per read/write operation, one for the IAsyncResult and one for the Task wrapping it; it'll take some work, but we should be able to get that down to one, at which point I think that's generally the best we can do with the current API shape (though for some operations, we should be able to eliminate all allocations if they complete synchronously). |
|
LGTM - Excellent work! 👍 |
- Change BufferAsyncResult to allow storing int result, without adding another field - Use that in both SslStream and NegotiateStream to avoid boxing an int per read and write - Also rename AsyncProtocolRequest.CompleteWithError to CompleteUserWithError, to avoid confusion and keep it consistent with the other CompleteUser methods.
bf58fb0 to
07b4825
Compare
|
@dotnet-bot Test Outerloop Windows_NT Debug please |
|
Thanks for the reviews, all. |
|
@dotnet-bot Test OuterLoop Ubuntu14.04 Debug please ("arm32 emulator vms build and test on the main drive which is small. This is tracked with issue https://github.com/dotnet/coreclr/issues/6676") |
|
@dotnet-bot Test OuterLoop Windows_NT Debug please (https://github.com/dotnet/corefx/issues/13281) |
|
@dotnet-bot Test OuterLoop Windows_NT Debug please (https://github.com/dotnet/corefx/issues/13284) |
|
@dotnet-bot Test OuterLoop Windows_NT Debug please (now that https://github.com/dotnet/corefx/issues/13284 is fixed) |
#12935 reduced allocations in SslStream reading/writing by ~70%. This PR reduces it further by another ~30% from where it was after that PR.
Before (10K reads / 10K writes):
After (10K reads / 10K writes):
Main changes:
cc: @geoffkizer, @ericeil, @davidsh, @CIPop, @benaadams, @davidfowl
Contributes to https://github.com/dotnet/corefx/issues/11826