-
Notifications
You must be signed in to change notification settings - Fork 234
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
Tune maximum connection pool size for Npgsql, enable multiplexing for non-platform benchmarks #1667
Conversation
OK results for non platform scenarios are in. They need a different number, in part (likely but untested) due to multiplexing being disabled in these cases. |
dc64f6b
to
06df9c3
Compare
Confirmed, enabling multiplexing in the others brings similar improvements :) Pushed commits with minimal files changed and modified the NoResetOnClose check to include Multiplexing as an alternative option (enabling multiplexing implies NoResetOnClose). |
Some detailed benchmark results: Fortunes platform benchmark (raw)
With MaxPoolSize=18, we get an improvement of 461,284-444,239=17045, or 3.8% RPS. Fortunes non-platform rawBaseline (MaxPoolSize=256): 291,597 (x5) With multiplexing, we get an improvement of 304,009-291,597=12412, or 4.2% RPS. Fortunes non-platform EF Core 5 (.NET 6.0, latest benchmark code)Baseline 220,956 (x5) With multiplexing, we get an improvement of 232,890-220,956=11934, or 5.4% RPS. Fortunes non-platform EF Core 6 (.NET 6.0, latest benchmark code)Before: 242,040 (x5) With multiplexing, we get an improvement of 250,554-242,040=8514, or 3.5% RPS. Still to do: measure the non-Fortunes platform benchmarks, enable multiplexing and reduce MaxPoolSize accordingly. Thanks again for this awesome work @NinoFloris - I'm just the benchmarking guy here :) |
By incorperating these changes; can Ben overtake Platform on Fortunes? 🤔😅 TechEmpower/FrameworkBenchmarks#6567 |
You are already running with Full PGO which is cheating ;) |
Is also on .NET 6.0; not my fault the "offical" ones move so sloooowly 😉 |
Here's what this does to all the platform benchmarks (5 iterations for each value):
Note the huge 35% jump for Updates! Caching regressed by 0.3%, though it's so small it might just be variance. Note that I didn't test out different MaxPoolSize values (except for Fortunes yesterday) - all the above were run with MaxPoolSize=18. At some point we can also do that - it's possible that different MaxPoolSize values fit different scenarios better. So @NinoFloris I think it makes sense to just use the same connection string (i.e. the Fortunes one) globally for all the platform benchmarks (see the non-platform database YAML for an example), possibly overriding only for caching to turn this off. |
Sorry to intrude but I remember seeing an improvement of around 40% in Fortunes non-platform (micro / raw), just by enabling Multiplexing (and keeping MaxPoolSize at 256). Even EF improved by about 20%, though I think wrk was reporting errors. Could be wrong though, it was a while ago. |
@n-stefan no intrusion whatsoever! Always good to have comments/thoughts. Here are some more tests for non-platform Fortunes raw (no dapper/ef, 3 iterations per scenario):
So you seem to be right that multiplexing improves things even without changing Max Pool Size (though Max Pool Size does help a bit). I'm not seeing anything like 40% though - any chance you can look into this and see where it came from? I do remember running a few quick benchmarks back when I was finishing the multiplexing feature, and seeing unsatisfactory results with the non-platform benchmarks. Something may have changed since then, e.g. the above is with latest .NET 6. It's also possible I was very focused on platform benchmark perf and neglected to properly test the non-platform benchmarks, I can't be sure any more... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@sebastienros can you merge this?
…xing for non-platform benchmarks (aspnet#1667)" This reverts commit d72cf79.
@roji |
@roji Maximum pool size 256 Maximum pool size 256 Maximum pool size 18 |
@n-stefan what exactly are these runs, where are they coming from and where's their conf? The numbers are totally off compared to what I'm seeing for non-platform TechEmpower Fortunes on Microsoft's Citrine hardware (which is supposed to be identical to the TE machine) - see the numbers I posted above. |
They are local results as I don't have access to Citrine, and this alone is an important difference. |
@n-stefan it's great to see the multiplexing improves perf on your machine (regardless of Max Pool Size). But it's expected that you'd see very different perf on your machine as compared to the TechEmpower hardware, which is considerably stronger (e.g. 12 cores). |
During offline conversations with @roji I noticed the TE Npgsql max pool size being at 256. As I was recently doing some tuning of my own https://twitter.com/NinoFloris/status/1381651573978316809 I experienced first hand how reducing max pool size can have a very positive effect on perf. @roji then tested a matrix of max pool sizes in the perf lab and landed on 18 connections for a 4% increase in RPS on platform fortunes. (from ~444,239 to ~461,284)
This PR reflects the changes to the connection strings in many of the places referring to this 256 max though I skipped orchard and mono. I'm not too familiar with the repo structure and all its yamls so I welcome a thorough review of the touched/missing files.