This repository was archived by the owner on Jan 23, 2023. It is now read-only.

dotnet/corefxPublic archive

NotificationsYou must be signed in to change notification settings
Fork4.9k
Star17.6k

Reduce allocations in SslStream reading/writing by ~30%#13274

Merged

stephentoub merged 3 commits intodotnet:masterfromstephentoub:ssl_allocs

Nov 3, 2016

Merged

Reduce allocations in SslStream reading/writing by ~30%#13274

stephentoub merged 3 commits intodotnet:masterfromstephentoub:ssl_allocs

Nov 3, 2016

Conversation

Copy link

Member

stephentoub commentedNov 2, 2016•
edited
Loading

#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:

Every read/write operation was allocating a new AsyncProtocolRequest. Without changing any of the plumbing through SslStream, we can take advantage of the fact that SslStream allows only a single read and a single write operation at a time, which means we can store and reuse an AsyncProtocolRequest instance for reading and one for writing, reusing the instances for all read/write operations on the stream.
Every read operation was boxing an Int32 result. This changes the BufferAsyncResult instance used for reads to reuse one of its existing fields to store that result, rather than storing it in the base LazyAsyncResult's object field.
Also took the opportunity to remove an unused field from BufferAsyncResult, though it doesn't change the size on 64-bit given the other fields on the base class.

cc:@geoffkizer,@ericeil,@davidsh,@CIPop,@benaadams,@davidfowl
Contributes tohttps://github.com/dotnet/corefx/issues/11826

stephentoub added2 commits

November 2, 2016 14:42

Remove unused BufferAsyncResult.IsWrite field

1f5cf31

Reuse AsyncProtocolRequest instances across read/write operations

411c156

Saves an allocation per read and write, ammortizing the costs across the whole stream's lifetime.

dnfclas added the cla-already-signed label

Nov 2, 2016

stephentoub changed the title~~Ssl allocs~~Reduce allocations in SslStream reading/writing by 30%

Nov 2, 2016

stephentoub changed the title~~Reduce allocations in SslStream reading/writing by 30%~~Reduce allocations in SslStream reading/writing by ~30%

Nov 2, 2016

geoffkizer reviewed

Nov 2, 2016

View reviewed changes

src/System.Net.Security/src/System/Net/HelperAsyncResults.cs


		internalvoidCompleteUser(objectuserResult)
		internalvoidCompleteUser(intuserResult)
		{

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

nit: capitalization

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

nit: capitalization

Of what?

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Oh, in the debug message below? GitHub wasn't showing that on the comment view. Will lower-case the R.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, sorry for the confusion, I meant the R in the comment below.

Copy link

geoffkizer commentedNov 2, 2016

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.

davidsh added the area-System.Net label

Nov 2, 2016

Copy link

MemberAuthor

stephentoub commentedNov 2, 2016

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).

Copy link

Member

benaadams commentedNov 2, 2016

LGTM - Excellent work! 👍

davidsh approved these changes

Nov 2, 2016

View reviewed changes

Copy link

Contributor

davidsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM. Thanks@stephentoub !

Avoid boxing Int32 result per operation

07b4825

- 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.

stephentoub force-pushed thessl_allocs branch frombf58fb0 to07b4825Compare

November 2, 2016 20:19

Copy link

MemberAuthor

stephentoub commentedNov 2, 2016

@dotnet-bot Test Outerloop Windows_NT Debug please
@dotnet-bot Test Outerloop Ubuntu14.04 Debug please

Copy link

MemberAuthor

stephentoub commentedNov 2, 2016

Thanks for the reviews, all.

Copy link

MemberAuthor

stephentoub commentedNov 2, 2016

@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 issuehttps://github.com/dotnet/coreclr/issues/6676")