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

[release/2.2] Change HttpConnectionPool.GetConnectionAsync to do read-ahead outside of lock (#) #32568

Merged
merged 1 commit into from Oct 2, 2018

Conversation

Projects
None yet
5 participants
@stephentoub
Copy link
Member

stephentoub commented Oct 2, 2018

Port #32495 to release/2.2.

Minimize the work done inside the lock in order to reduce contention. This means we need to reacquire the lock if the connection isn't usable, but in the fast path case where the connection is usable, we remove the syscalls from being performed while holding the lock.

Related to #31799

Change HttpConnectionPool.GetConnectionAsync to do read-ahead outside…
… of lock (#32495)

Minimize the work done inside the lock in order to reduce contention.  This means we need to reacquire the lock if the connection isn't usable, but in the fast path case where the connection is usable, we remove the syscalls from being performed while holding the lock.

@stephentoub stephentoub added this to the 2.2 milestone Oct 2, 2018

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Oct 2, 2018

Description

SocketsHttpHandler pools connections, and when it takes a connection from the pool, it checks to see whether the connection is still alive. That check is currently happening while a lock is held, which under heavy load and many short requests can result increased contention on the lock. This change moves that check to outside of the lock, so that in the fast path / common case, the lock is held for a very short period of time.

Customer Impact

For microservices where many ASP.NET requests in turn make many HttpClient calls to other services, the contention can result in significantly reduced throughput than is otherwise possible. The benchmark results shown in #32495 (comment) highlight this impact.

Regression?

No

Risk

This code path is critical to SocketsHttpHandler, used in the processing of all requests, so a mistake here could be impactful. But the change itself is minimal: it moves some checks from inside the lock to outside the lock. The net result is that for the common case, we still take the lock once, but we hold it for a much shorter period of time. In the less common case where we were able to grab a connection but then discover after releasing the lock and doing these checks that the connection is no longer valid (e.g. the server closed it after a timeout), we end up needing to loop around and take the lock again, for a small increase in cost on the uncommon path.

@Eilon

Eilon approved these changes Oct 2, 2018

Copy link
Member

Eilon left a comment

I approve for 2.2, though I don't know the code.

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Oct 2, 2018

The Windows failure is an unrelated pipelines test: ReadAsyncCompletesIfFlushAsyncCanceledMidFlush.

@dotnet-bot test Outerloop Windows x64 Debug Build please

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Oct 2, 2018

@jamshedd

This comment has been minimized.

Copy link
Member

jamshedd commented Oct 2, 2018

Approved for 2.2.

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Oct 2, 2018

The Outerloop Windows failures are unrelated:

  • System.MemoryTests.MemoryTests.MemoryManagerPinLargeArray, failed with an OOM on Win10
  • GetResponseAsync_ServerNameNotInDns_ThrowsWebException, due to #31147 which was fixed in master but not other branches
@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Oct 2, 2018

@stephentoub you can hit merge when CI is green (or effectively so)

@stephentoub stephentoub merged commit 7cddd8e into dotnet:release/2.2 Oct 2, 2018

13 of 15 checks passed

Outerloop Windows x64 Debug Build Build finished.
Details
Windows x64 Debug Build Build finished.
Details
CROSS Check Build finished.
Details
Linux arm Release Build Build finished.
Details
Linux arm64 Release Build Build finished.
Details
Linux x64 Release Build Build finished.
Details
Linux-musl x64 Debug Build Build finished.
Details
NETFX x86 Release Build Build finished.
Details
OSX x64 Debug Build Build finished.
Details
Packaging All Configurations x64 Debug Build Build finished.
Details
UWP CoreCLR x64 Debug Build Build finished.
Details
UWP NETNative x86 Release Build Build finished.
Details
WIP ready for review
Details
Windows x86 Release Build Build finished.
Details
license/cla All CLA requirements met.
Details

@stephentoub stephentoub deleted the stephentoub:port32495 branch Oct 2, 2018

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Oct 2, 2018

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment