[WIP] SslStream Throttle - #25187
Conversation
|
#23485 |
|
@dotnet-bot test Outerloop Linux x64 Debug Build please |
|
|
||
| async Task CompleteAsync(TWriteAdapter wAdapter, Task sTask) | ||
| { | ||
| await semaphoreTask.ConfigureAwait(false); |
There was a problem hiding this comment.
This incurs a closure allocation even on the fast path. I think you meant sTask.
There was a problem hiding this comment.
Nah, it's way too easy for anyone to make that mistake. This is an example of why I want the ability to mark lambdas and local functions as non-capturing.
cc: @MadsTorgersen
There was a problem hiding this comment.
I normally do the move it outside the outer function and look for errors but yeah it catches me a bit
|
|
||
| SecurityStatusPal status = _sslState.EncryptData(buffer, ref outBuffer, out int encryptedBytes); | ||
| s_throttle.Release(); | ||
| await WriteEncryptedDataAsync(wAdapter, outBuffer, rentedBuffer, encryptedBytes, status); |
|
|
||
| SecurityStatusPal status = _sslState.EncryptData(buffer, ref outBuffer, out int encryptedBytes); | ||
| s_throttle.Release(); | ||
| return WriteEncryptedDataAsync(writeAdapter, outBuffer, rentedBuffer, encryptedBytes, status); |
There was a problem hiding this comment.
Can these five lines be moved to a Task-returning method? Then it's essentially:
return semaphoreTask.IsCompletedSuccessfully ?
ThatMethodWhateverItsCalled() :
CompleteAsync(writeAdapter, semaphoreTask);
async Task CompleteAsync(TWriteAdapter wAdapter, Task sTask)
{
await sTask.ConfigureAwait(false);
await ThatMethodWhateverItsCalled().ConfigureAwait(false);
}
|
What's interesting is it works (with a small perf hit) on windows. But aspnet isn't working at all with https on linux clean install (without my change but latest) so I need to confirm that somethings not broken. |
I'm suspicious of the test I previously noted as repeatedly failing: |
|
Yeah I am looking at it now to figure out if the issue is my system or code first .. then what .net 2.1 works |
|
@Priya91 @stephentoub I can confirm if I rollback master to before the cancellation PR that asp.net 2.0 works fine, if I take current master (none of my changes) it doesn't work. The test is a simple "hello world" with wrk to connect 256 times. No connection works. I think that PR needs to be looked at. |
|
#25199 Found fix here, (Your suspicion was bang on) FYI with that fix, and this change I am seeing approx 4x speed up on Linux still (16 core azure ubuntu 16.04) but need SslStream stable to move forward |
|
Figures without the extra allocation (both master and this PR with the cancellation commits removed for testing). I didn't bother with loads of connection combos on Linux, because the effect is pretty consistent and clear, and I didn't want to burn all my azure credits when the results are pretty clear. (All results are 16 pipelined requests, on Azure 16 core DS series, AspNetCore helloworld-Techempower) (I also forgot to do both before and after on the 256 for win so can do later)
My takeaway from this, Linux is a no brainer. Windows, well it actually seems to make very little difference except maybe a slight drop in RPS but better average latency. |
|
I don't know if its worth PALing out and pushing down the semaphore out of Windows. To be honest I would like to see the read side fixed first (I suspect that read PR will work fine with the roll back) and then if we could try this with both sides in the mix. |
|
@dotnet-bot test Outerloop Linux x64 Debug Build please |
|
Other one has issues #24497; might want to remove WIP on this one; raise issue for moving semaphore to Linux PAL; then rebasing and investigating failures on other one? |
|
Heh, also need to do the feedback 😉 |
|
Yeah I had done it in a branch with the revert. Tonight I will address the feedback. |
|
@dotnet-bot test Outerloop Linux x64 Debug Build please |
| private static readonly AsyncProtocolCallback s_resumeAsyncReadCallback = new AsyncProtocolCallback(ResumeAsyncReadCallback); | ||
| private static readonly AsyncProtocolCallback s_readHeaderCallback = new AsyncProtocolCallback(ReadHeaderCallback); | ||
| private static readonly AsyncProtocolCallback s_readFrameCallback = new AsyncProtocolCallback(ReadFrameCallback); | ||
| private static readonly SemaphoreSlim s_throttle = new SemaphoreSlim(Environment.ProcessorCount); |
There was a problem hiding this comment.
Have you experimented with this value? I'm wondering if some small multiple (2 or 3) would test better or worse.
There was a problem hiding this comment.
Also, can you add a comment explaining why this exists? Presumably we'll undo this once we upgrade to OpenSSL 1.1. if there's an issue tracking that, we can include its issue number here.
There was a problem hiding this comment.
Will add a comment and issue number.
As to the multiplier, I can retry these numbers I believe that actually a smaller number (/2) was better on boxes with hyperthreading as the aes algo uses pretty much uses the registers/pipeline anyway. As azure doesn't do hyperthreading (well some newer instances do) 1x was the happy medium.
But that is from memory so I will try to retest on physical vs azure, I am limited to 16 cores on azure and my physical has 24 cores so it's also a bit Apple's and oranges.
There isn't a way that I know of to get the "real" core count rather than virtual without me going to native apis...?
There was a problem hiding this comment.
I can retry these numbers I believe that actually a smaller number (/2)
I could believe that as well, but that's also dangerous, especially when using the same logic on Windows.
There isn't a way that I know of to get the "real" core count rather than virtual without me going to native apis...?
Correct; that's not currently exposed anywhere. There have been discussions in the past about exposing more about the machine's topology, but that's never materialized in built-in APIs. I'm fairly certain there are 3rd party implementations of that, though, including if I remember correctly in Joe Duffy's concurrency book.
There was a problem hiding this comment.
Its also more complicated with Jobs/Docker/cg_groups etc
There was a problem hiding this comment.
Yeah I have my own code for that for some stuff I do at work (heavy handed avx montecarlo simulations are a good place to ignore hyperthreads to avoid context thrashing ;)) but I didn't want to include that here. Maybe I can put in an API review. Anyway I will run a basic test at 2x and see what direction it heads
There was a problem hiding this comment.
I will revert my statement then "I have code for bare metal on linux and windows" :)
Anyway @stephentoub I tested 2x and on Ubuntu on Azure (no hyper threading) it goes from ~720k to ~200k-300k, it wasn't consistent on the 2x so I think the lock convoy is in full effect.
Also the same locks are used on encrypt and decrypt so actually we are nearer to 2x anyway as we are only throttling one side with the current change. A different multiplier might be worth a visit if I get the other side in as async and hook it into the semaphore.
I will make the comment change after getting tests to pass I don't want to nuke the long run.
There was a problem hiding this comment.
As aside have a thing for Windows that does Logical vs Physical https://github.com/davidfowl/Channels/blob/master/src/Channels.Networking.Windows.RIO/Internal/CpuInfo.cs#L31-L43
There was a problem hiding this comment.
Me 3 and linux, but I figure that might be pushing the boundary for this PR ;)
There was a problem hiding this comment.
it goes from ~720k to ~200k-300k
Ok. Thanks for checking.
that might be pushing the boundary for this PR
Yes ;)
|
redhat 🔥 System.Reflection.Metadata.Tests fedora |
|
@dotnet-bot test Outerloop Linux x64 Debug Build please |
|
OSX x64 Debug Build has been running 8 hrs; think it has hung (though not timed out) |
|
@dotnet-bot test OSX x64 Debug Build |
|
I suspect the OSX issues is, there is 1 Mac Pro available and its getting hammered. |
|
OSX is still |
|
@dotnet-bot test OSX x64 Debug Build please |
|
|
|
They all did... (all OSX runs) |
|
WIP because I am yet to do any perf/stress tests or tracing. I would probably think that a refactor could be done as well of the adapaters, at least a common Interface, but not until perf has been confirmed. |
|
I have the following error locally as well? Bad merge or is something else going on here? |
|
Most likely a bad merge. There was an issue a while back with the wrong casing for that file, but it was fixed weeks ago. |
|
Okay I will look for the issue... |
|
So my initial findings are (I can write up later),, that it hits windows ~5-10% rps on benchmarks. This seems too much to me, so I will (hopefully tonight if possible) push the Throttle down into the PAL and make it a no-op on windows and only active on Linux. |
|
Results on windows (with the PAL) are as expected... the difference bouncing around (Azure benching seems to be less than perfect) but its basically
So basically no change (some runs were faster than master some slower so above is the mean) Coming up the Linux benchmarks |
|
@dotnet-bot test Outerloop Linux x64 Debug Build please |
|
@stephentoub I have done the Linux thing on Azure... to sum it up But I have made a PR to my PR https://github.com/Drawaes/corefx/pull/6/files It hits ~500k This seems to me a better solution, giving it more than 1 for the semaphore improves it but I don't really have a solid heuristic for what it should be. Thoughts? |
|
@Drawaes, can you explain the fix to me? Why does calling ClearError address the issue, and what kind of impact does that have functionally? And would we still need to do asynchronous WaitAsyncs, or at this point could it just use lock/Monitor, or if we wanted a higher count, Wait/Release on the semaphore rather than WaitAsync/Release. |
|
About that PR (https://github.com/Drawaes/corefx/pull/6/files) @stephentoub. ClearError was previously always called in the c++ SslRead and SslWrite methods (ERR_clear_error) before calling into openssl. But as the throttle methods were specifically introduced to be run before doing any r/w/encrypt/decrypt calls it seems like a valid option to remove the ERR_clear_error calls in the shim and let throttle handle it. Downside is correctness not being guaranteed (false positives, errs from another thread) when calling into pal, now need to remember/know to first call throttle to be correct (a comment might be useful here). As long as all code using sslstate/securechannel/pal etc does throttle before r/w/enc/dec it's correct. @Drawaes good summary? |
Does the throttle guarantee that the subsequent operations are on the same pthread?
Then it's not worth the perf gain. |
|
Why would correctness not be guaranteed? The move have just reduced the lock time. There is no async or any other calls between the clear error and the encrypt/decrypt. I didn't remove the clear error from any call other than the encrypt/decrypt (all other methods still do them in native code). Basically I have changed the code from this To this No where there is there an opportunity for anyone to swap out the thread and let it do something else... |
|
Well I can now call enc/dec without calling throttle. That's what I meant
with correctness
…On Fri, Dec 1, 2017, 21:27 Tim Seaward ***@***.***> wrote:
Why would correctness not be guaranteed? The move have just reduced the
lock time. There is no async or any other calls between the clear error and
the encrypt/decrypt. I didn't remove the clear error from any call other
than the encrypt/decrypt (all other methods still do them in native code).
Basically I have changed the code from this
//Potentially Async Throttle
//PInvoke
Native ClearError
Native Encrypt
//Managed Release
To this
//Potentially Async Throttle
//PInvoke
Native ClearError
//Managed Release
//PInvoke
Native Encrypt/Decrypt
No where there is there an opportunity for anyone to swap out the thread
and let it do something else...
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#25187 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEBfuTXpScwGZXgkpU9QDljIGxmHaQXDks5s8GEegaJpZM4QaSRa>
.
|
|
Ahh sure but its not a public API and its only called in one place in internal classes. I think what @bartonjs took from your wording was the previous situation..... A thread doesn't clean itself up, is reused from the pool and you get an error from another op potentially polluting the state (the Ssl part of OpenSsl uses errors to indicate things like "I need more data etc"). That is still not a possibility with this code. |
|
Yeah that was all my comment was about and I suppose what @stephentoub was asking as well. What's the impact of removing from shim. So yep as it's internal api all callers need to do the correct dance to not get false positives (I've the feeling I'm repeating exactly what I wrote though) |
Yes 100% |
|
You know what, forget the side PR. It's just confusing everyone (I know its better :P but hey). Let's stick with what is in here.... The figures I have are basically these Master ~200k rps It starts to go down from there again, 1/2 seems to be the peak on a 16 core azure box running ubuntu. I have also tried it on my local machine (4 core desktop) and the same pattern holds. I can't test it on 32 cores because I need a 32 core load gen and on my puny azure account I can only have 64 cores in a single region. So I would say go with the throttle with ProcCount/2 it won't effect windows or OSX now because I PAL'd them out. |
@dotnet-bot test Outerloop Linux x64 Debug Build please |
|
Replaced by #25646 |
9 participants
This would satisfy the throttling.
I will test the perf on windows and linux and see if it makes any difference on windows. If it does then I will need to push the semaphore down into the PAL.
I am submitting the PR to check the outerloop.