Use C++11 atomics to update session stats#2786
Conversation
a9a8408 to
b464a95
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2786 +/- ##
==========================================
- Coverage 78.52% 78.51% -0.01%
==========================================
Files 680 680
Lines 115362 115356 -6
Branches 16329 16328 -1
==========================================
- Hits 90587 90574 -13
- Misses 23979 23986 +7
Partials 796 796 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi! thanks for progressing on this fix. I must say that I'm quite surprised by the addition of saturated arithmetic in the increments, which is a behavior change. I don't know how stats are consumed, but anywhere stats are available in many products/projects, they're either 64-bit to avoid wraparound, or they just implement regular wraparound at 2^32 that all stats collection tools are perfectly fine with. Here, using saturation means that a process linked with the library will have to be periodically restarted just to permit stats to continue to move. On the 64-core machine where I ran the tests, it means that the saturation is reached every 3 hours, so a process would have to be killed and restarted every 3 hours, otherwise after that time, the stats become unusable. In this case it's quite pointless to waste so much CPU time keeping them up-to-date. As such I would suggest to just continue to let the counter wrap around and not change that behavior. Depending how the stats are consumed, maybe turn them to unsigned int so that they wrap from 4294967295 to zero, like most products. Otherwise you can also change them to size_t and they'll adapt to the platform. 64-bit platforms will benefit from having almost infinite wrapping periods, while 32-bit platforms will be less concerned by this due to their intrinsic limitation and their use cases which is not related to performance (i.e. we won't face thousands of connections per second on 32-bit platforms anymore because those needing this perf level can reach it with the cheapest 64-bit ones). |
|
@wtarreau - Thanks for your insight! Yeah, I was really unsure about the handling of overflow. I'll take your suggestion, simplify the implementation, and preserve the existing behavior. |
|
I'm willing to give it a try to help you confirm it's OK, but I can't seem to find a way to download the final diff in github, I can only visualize it and have not figured how to download it. Any guidance is welcome (I'm always having great difficulties with github interface which turns every basic git operation into overly complicated stunts). |
Thanks! I created this patch from this PR: 0001-Use-C-11-atomics-to-update-session-stats.patch |
|
Perfect, works out of the box, x10 in perf (from 22k cps to 220k) when applied on top of 1.63. Feel free to add a "Tested-by: Willy Tarreau w@1wt.eu" if that helps. |
|
Thanks! |
### Description of changes: Prepare release v1.64.0. ### What's Changed * Update max polyz value by @jakemas in #2787 * ECR Repositories for Android and Formal Verification Images by @skmcgrail in #2794 * Support more "openssl rsa" options by @justsmth in #2777 * Remove python codebuild patches by @WillChilds-Klein in #2793 * Additional options for "openssl c_client" by @justsmth in #2791 * GitHub-based Formal Verification Image Build by @skmcgrail in #2796 * Use C++11 atomics to update session stats by @justsmth in #2786 * Support "openssl dhparam" by @justsmth in #2790 * Add scrutinice pull permissions for aws-lc/amazonlinux repository by @skmcgrail in #2799 * Use GitHub-based Verification Images by @skmcgrail in #2798 * Remove dead code by @torben-hansen in #2797 * Rename snapsafe to VM UBE by @torben-hansen in #2800 * Bump MySQL version tag to 9.5.0 by @samuel40791765 in #2768 * Migrate to macos-15-intel by @samuel40791765 in #2802 * Use right compiler with ruby CI by @samuel40791765 in #2801 * Migrate analytics job to be GitHub triggered by @skmcgrail in #2779 * Support NetBSD by @justsmth in #2754 * Make poly_chknorm constant flow by @jakemas in #2788 * Rename fork to fork UBE by @torben-hansen in #2803 * Extend grv asan timeout for Golang to allow completion by @torben-hansen in #2805 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.
### Description of changes: Prepare release v1.64.0. ### What's Changed * Update max polyz value by @jakemas in aws#2787 * ECR Repositories for Android and Formal Verification Images by @skmcgrail in aws#2794 * Support more "openssl rsa" options by @justsmth in aws#2777 * Remove python codebuild patches by @WillChilds-Klein in aws#2793 * Additional options for "openssl c_client" by @justsmth in aws#2791 * GitHub-based Formal Verification Image Build by @skmcgrail in aws#2796 * Use C++11 atomics to update session stats by @justsmth in aws#2786 * Support "openssl dhparam" by @justsmth in aws#2790 * Add scrutinice pull permissions for aws-lc/amazonlinux repository by @skmcgrail in aws#2799 * Use GitHub-based Verification Images by @skmcgrail in aws#2798 * Remove dead code by @torben-hansen in aws#2797 * Rename snapsafe to VM UBE by @torben-hansen in aws#2800 * Bump MySQL version tag to 9.5.0 by @samuel40791765 in aws#2768 * Migrate to macos-15-intel by @samuel40791765 in aws#2802 * Use right compiler with ruby CI by @samuel40791765 in aws#2801 * Migrate analytics job to be GitHub triggered by @skmcgrail in aws#2779 * Support NetBSD by @justsmth in aws#2754 * Make poly_chknorm constant flow by @jakemas in aws#2788 * Rename fork to fork UBE by @torben-hansen in aws#2803 * Extend grv asan timeout for Golang to allow completion by @torben-hansen in aws#2805 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.
@wtarreau just append |
Issues:
Addresses: #2776
Description of changes:
ssl_update_counteruses atomicfetch_addto increment when atomic operations are available.ssl_read_counteruses atomicloadfor lock-free reads when available.Call-outs:
The previous
ssl_read_counterimplementation was not taking a reference tocounter, so was acquiring the lock for no reason.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.