Change HttpConnectionPool.GetHttpConnectionAsync to do read-ahead outside of lock #32495
Conversation
…side of lock 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.
|
LGTM. What about the other places where we are doing potentially expensive work under the lock? Do you think these are worth looking at? I don't think we necessarily need to address these as part of this PR, I just want to understand what our plan here is. |
|
We are also disposing of the connection under the lock in ReturnConnection. This will cause the stream to be closed. We should probably change this to do the dispose outside of the lock. |
I'd like to address such things separately as we find them.
Yes, but the others that I currently know of aren't main-path, e.g. the disposing of the connection you mention should be relatively rare, and if it does happen, we're also going to incur other very expensive things, like setting up a new connection eventually to replace it. That's not to say we shouldn't do it... that case in particular should be easy to fix, so we may as well. I just want to make sure we're focusing our efforts on finding and fixing the most impactful things first. Hopefully like this PR :) But we'll see. I of course agree with your general sentiment that we should be removing as much expensive stuff as we can from within locks. Ideally as you know I'd like to move towards fewer locks, too :) |
That's fine with me. I just want to make sure we don't lose track of the other issues. Unless we construct tests that are explicitly hitting those scenarios, they aren't going to bubble to the top in terms of impactfulness. |
|
I think we're on the same page :) |
|
Here are the results we got with this PR. Added the 2.2 results as a comparison point. The PR being based on a 3.0 branch. This is running distinct load, proxy and downstream servers. The pool is to ensure that a single
|
|
Thanks for validating, @sebastienros and @mikeharder. |
|
How risky would it be to take such a change in 2.2? The perf improvement is very significant, and microservices communication is an important pillar for .NET Core. /cc @Eilon |
It's not particularly risky. It's in an area of code where if we messed up it would be very impactful, but the change itself is relatively straightforward. |
… of lock (dotnet#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.
…side of lock (dotnet/corefx#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. Commit migrated from dotnet/corefx@ac2f2e7
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.
Alternative to #32472. It's hard to say for sure, but waving my hands a bit, it seems like this maybe provides 80% of the benefits of that one, and without the concerns around LIFO vs FIFO.
Fixes #31799.
cc: @geoffkizer, @aviviadi, @ayende, @mikeharder