stats: extend the lock-free ref-count release fast path to histograms and relax ref-count orderings by davidvo-lyft · Pull Request #46001 · envoyproxy/envoy

davidvo-lyft · 2026-07-06T19:11:38Z

Rescoped after #45821 landed (which added the same CAS fast path for counters/gauges/text-readouts — this PR originally contained that too; the merge adopts the landed implementation and keeps the remaining, complementary pieces).

What this PR adds on top of main:

Histogram release fast path: ParentHistogramImpl::decRefCount previously took the store's histogram lock on every release; it now uses the same pattern that stats: optimize decRefCount to help reduce hot-spot in stats sink #45821 landed for scalar stats. Per review feedback, the CAS fast path is shared by both call sites via tryDecRefCountFastPath() in refcount_ptr.h (a possibly-final release still falls through to the existing locked path, so the 1->0 transition remains atomic with removal from the by-name set).
Canonical ref-count memory orderings: incRefCount becomes a relaxed fetch_add (creation/resurrection still happen under the respective locks), RefcountHelper moves from seq-cst to relaxed-increment / acq_rel-decrement per the Boost example the code already cites (remedy 1 in stats: excessive ref-count inc/dec causing performance bottleneck #43836), and use_count() becomes a relaxed load (its only non-test caller is the RefcountPtr passthrough).
Multithreaded race tests for the fast path: stats: optimize decRefCount to help reduce hot-spot in stats sink #45821 landed with speed tests only. This adds AllocatorTest.RefCountNonFinalReleaseChurn, AllocatorTest.RefCountCrossThreadFinalRelease (a worker performs the final release while other workers race the fast path), and RefcountPtr.ThreadedCopyDropChurn.
A 2500000 case for server_stats_flush_benchmark (10M metric objects per snapshot — the regime reported in stats: excessive ref-count inc/dec causing performance bottleneck #43836), CI-skipped via skipExpensiveBenchmarks(), and a source/docs/stats.md update.

Benchmarks — interleaved attribution: three configs at the same base commit, prebuilt binaries run round-robin for three rounds (-c opt, CI docker image, aarch64 = Apple M5 Max / colima 12 vCPU; medians of 5 per round, pooled):

bmFlushToSinks	main	main + orderings only	this PR
/1000000 (4M metric objects)	148 ms	149 ms	122 ms (-18%)
/2500000 (10M metric objects)	405/446/455 ms by round	~no effect	318/348/353 ms (-21-22% every round)

Single runs at 10M are noisy (CV 23-39%), but the per-round pairing is consistent. The measured win is the histogram fast path; the orderings don't move this benchmark (their per-op effect shows in #45821's microbenchmarks, not here).

This remains complementary to the direction discussed in #43958/#44031/#44285 (removing the snapshot's per-stat references).

Partially addresses #43836.

Commit Message: stats: extend the lock-free ref-count release fast path to histograms and relax ref-count orderings
Additional Description: see above
Risk Level: Medium (concurrency-sensitive; mirrors the pattern already landed in #45821 for scalar stats; no functional or API changes; final-release locking and lifetime semantics preserved; new multithreaded race tests)
Testing: new multithreaded race tests above plus existing race tests (RefCountDecAllocRaceOrganic/Synchronized); //test/common/stats/..., //test/server:server_test and //test/server:server_stats_flush_benchmark_test pass locally (clang, opt, aarch64), race tests stressed at --runs_per_test=10 on weakly-ordered ARM hardware. Local TSAN is not possible on aarch64 (the prebuilt tsan_libs libc++ is x86_64-only); relying on the CI TSAN job for sanitizer coverage of the lock-free path.
Docs Changes: source/docs/stats.md (lock now required only for final ref-count decrements)
Release Notes: changelogs/current/minor_behavior_changes/stats__lock-free-non-final-ref-count-releases.rst
Platform Specific Features: N/A

Generative AI disclosure: This change was developed with AI assistance (Claude Code). I have reviewed and understand every line, the concurrency reasoning, and the benchmark methodology, and I take full ownership of the submission and of addressing review feedback.

Releasing a reference to a stat currently takes the allocator's global mutex (the store's histogram lock for histograms) on every release, although the lock is only required when the release can be final: the decrement to zero must be atomic with removing the stat from the by-name set to avoid a destruction/lookup race with concurrent re-allocation of the same stat name (envoyproxy#9533). All stat ref-count atomics are also sequentially consistent, where canonical reference counting needs only relaxed increments and acquire/release decrements. This made the ref-count a flush bottleneck (envoyproxy#43836): MetricSnapshotImpl briefly references every stat, so each flush paid a mutex round-trip per stat on the main thread while dropping those references. Non-final releases now decrement with a CAS loop that can never reach zero outside the lock; a release that observes itself last falls through to the existing locked path unchanged, preserving the resurrection-race protection. RefcountHelper and the stat ref-counts now use the canonical orderings (relaxed increment, release/acq-rel decrement) per the Boost example already cited in the code. bmFlushToSinks (-c opt, aarch64, medians of 5 reps): 1000000 (4M metric objects): 195 ms -> 119 ms (-39%) 2500000 (10M metric objects): 584 ms -> 376 ms (-36%) The 2500000 benchmark case is added by this change; run-to-run variance at 10M objects also drops (CV 21% -> 6%). Partially addresses envoyproxy#43836. Signed-off-by: David Vo <davidvo@lyft.com>

repokitteh-read-only · 2026-07-06T19:11:44Z

Hi @davidvo-lyft, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #46001 was opened by davidvo-lyft.

see: more, trace.

ggreenway · 2026-07-07T16:01:03Z

@jmarantz can you take a look at this?

ggreenway · 2026-07-07T16:01:16Z

@davidvo-lyft there's a merge conflict; please resolve

ggreenway · 2026-07-07T16:18:36Z

@davidvo-lyft I just saw #45821 was merged yesterday, which is working on the same thing.

davidvo-lyft · 2026-07-07T17:31:15Z

Thanks for the pointer — I hadn't seen #45821 land (nice speedup, and it independently validates the same fast-path shape). It covers the counter/gauge/text-readout allocator path; the remaining delta in this PR is: the same fast path for histograms (ParentHistogramImpl still takes the store's histogram lock per release), relaxed increments plus the canonical RefcountHelper orderings (the refcount_ptr.h comment / first remedy in #43836), multithreaded race tests for the fast-path vs. final-release interaction (#45821 landed with speed tests only), and a 10M-metric-object case for the flush benchmark. I'm merging main, re-measuring the remaining delta against the post-#45821 baseline, and will resolve the conflict and update the description with fresh numbers today.

…-fastpath Resolves the allocator.cc conflict in favor of the scalar decRefCount fast path that landed in envoyproxy#45821; this branch keeps the relaxed incRefCount/use_count orderings on top of it. Signed-off-by: David Vo <davidvo@lyft.com>

…roxy#45821 Match the snapshot naming and acq_rel compare-exchange ordering of the scalar-stat fast path that landed in envoyproxy#45821, and point the comment at that implementation, so the two copies stay reviewable side by side. Signed-off-by: David Vo <davidvo@lyft.com>

jmarantz · 2026-07-07T17:50:15Z

Can you merge main? I think there's a lot left in this PR to add.

Signed-off-by: David Vo <davidvo@lyft.com>

davidvo-lyft · 2026-07-07T18:22:05Z

Merged main and resolved the conflict (adopted the #45821 implementation for scalar stats verbatim) and rescoped this PR to the remaining delta: the same fast path for histograms (mirroring the #45821 pattern), relaxed increments + canonical RefcountHelper orderings, multithreaded race tests for the fast path (there are currently only speed tests), and the 10M-object benchmark case. Description updated with fresh same-session numbers against post-#45821 main: a further -13% at 4M metric objects; the 10M case shows no measurable delta on my (busy) workstation — CVs there are 25-31%, so I'd treat it as unresolved rather than zero, and I'm happy to re-run on quiet hardware if useful.

jmarantz

LGTM; really great, with one question in the code, and this one for the description:

Is the benefit over #45821 for the sink benchmark due to the improvement for histograms, the improvement on incRefCount, or both?

I did a bit of grepping and use_count is used only for tests, afaict. It is used quite a bit for tests though. If you are confident that 'relaxed' reads are OK for that I'm inclined to believe you.

I don't think I've read enough to become expert on the exotic atomic behavior you are exploiting but I did carefully compare wht you did to the boost implementation you used as a reference, and I was very pleased by your analysis of the small changes you made to it, to accomodate TSAN tooling limitations.

@davidvo-lyft up to you if you want to try a small code-sharing refactor; you could do that as a follow-up also. Please let us know your thoughts.

jmarantz · 2026-07-09T02:00:49Z

+  // the role of the allocator's mutex.
+  ASSERT(ref_count_ >= 1);
+  uint32_t ref_count_snapshot = ref_count_.load(std::memory_order_relaxed);
+  while (ref_count_snapshot > 1) {


can we share this block of code somehow, maybe via a helper class, with the counter/gauge mechanism in allocator.cc?

Done, tryDecRefCountFastPath() in refcount_ptr.h, used by both this and the counter/gauge path. Free function instead of a helper class since the two impls hold their counts differently. The #45821 case-analysis comment moved onto the helper.

Review feedback from jmarantz: extract the CAS fast path introduced in envoyproxy#45821 (and mirrored for histograms here) into tryDecRefCountFastPath() in refcount_ptr.h, used by both StatsSharedImpl::decRefCount and ParentHistogramImpl::decRefCount. A free function over the raw atomic is used rather than a helper class because the two implementations hold their counts differently; the interleaving case analysis moves onto the helper. Signed-off-by: David Vo <davidvo@lyft.com>

Signed-off-by: David Vo <davidvo@lyft.com>

davidvo-lyft · 2026-07-09T05:26:19Z

Thanks! Ran an interleaved comparison (three configs, same base commit, prebuilt binaries round-robin x3 rounds, aarch64, medians of 5): at 4M metric objects main is 148ms, orderings-only 149ms, full branch 122ms. At 10M the full branch won by 21-22% in every round (405/446/455 vs 318/348/353ms), though single runs at that size are noisy, so call it approximate. So the win is the histogram fast path; the orderings (incl. the relaxed incRefCount) don't move this benchmark, which lines up with your "somewhat incremental in benefit" guess in #43836. Folded into the description.

On use_count(): relaxed loads stay atomic and coherent on the variable itself, they just don't synchronize with the writing thread. Every caller outside the RefcountPtr::use_count() passthrough is a test, reading single-threaded or post-join, so there's already a happens-before edge and the value is exact. Happy to drop the hunk if you'd rather keep the diff minimal.

Code sharing: done in this PR, tryDecRefCountFastPath() in refcount_ptr.h, both call sites switched. Went with a free function over a helper class since the two impls hold their counts differently, the atomic is the only shared state. The #45821 case-analysis comment moved onto the helper. Pushed.

jmarantz

LGTM, thank you!

@ggreenway can you do the final approval?

ggreenway

LGTM; just one nit about changelogs

/wait

ggreenway · 2026-07-09T17:55:24Z

@@ -0,0 +1,7 @@
+Extended the lock-free non-final reference-count release fast path (added for counters,


Can you combine this with the previous changelog entry? From the reader/users point of view, it doesn't matter that it was two separate PRs.

Done. #45821 didn't add a changelog entry, so I reworded this one to describe the combined change from the user's point of view (counters/gauges/text readouts and histograms together, no per-PR split).

Review feedback from ggreenway: describe the whole optimization (the scalar-stat half from envoyproxy#45821 plus this PR) as a single user-facing entry rather than splitting it by PR. Signed-off-by: David Vo <davidvo@lyft.com>

davidvo-lyft · 2026-07-09T19:16:17Z

/retest

davidvo-lyft had a problem deploying to external-contributors July 6, 2026 19:11 — with GitHub Actions Error

davidvo-lyft mentioned this pull request Jul 6, 2026

stats: excessive ref-count inc/dec causing performance bottleneck #43836

Open

davidvo-lyft added 2 commits July 7, 2026 13:32

stats: rescope changelog to the remaining delta after envoyproxy#45821

b560386

Signed-off-by: David Vo <davidvo@lyft.com>

davidvo-lyft had a problem deploying to external-contributors July 7, 2026 18:21 — with GitHub Actions Error

davidvo-lyft changed the title ~~stats: add a lock-free fast path for non-final ref-count releases~~ stats: extend the lock-free ref-count release fast path to histograms and relax ref-count orderings Jul 7, 2026

ggreenway assigned jmarantz and ggreenway Jul 8, 2026

jmarantz reviewed Jul 9, 2026

View reviewed changes

davidvo-lyft added 2 commits July 8, 2026 22:48

stats: update changelog figure to the interleaved benchmark result

013ad46

Signed-off-by: David Vo <davidvo@lyft.com>

davidvo-lyft temporarily deployed to external-contributors July 9, 2026 05:25 — with GitHub Actions Inactive

jmarantz reviewed Jul 9, 2026

View reviewed changes

ggreenway requested changes Jul 9, 2026

View reviewed changes

repokitteh-read-only Bot added the waiting label Jul 9, 2026

davidvo-lyft temporarily deployed to external-contributors July 9, 2026 18:07 — with GitHub Actions Inactive

repokitteh-read-only Bot removed the waiting label Jul 9, 2026

ggreenway approved these changes Jul 9, 2026

View reviewed changes

ggreenway enabled auto-merge (squash) July 9, 2026 18:15

ggreenway merged commit 8d34db7 into envoyproxy:main Jul 9, 2026
26 of 27 checks passed

		@@ -0,0 +1,7 @@
		Extended the lock-free non-final reference-count release fast path (added for counters,

Uh oh!

Conversation

davidvo-lyft commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Jul 6, 2026

Uh oh!

ggreenway commented Jul 7, 2026

Uh oh!

ggreenway commented Jul 7, 2026

Uh oh!

ggreenway commented Jul 7, 2026

Uh oh!

davidvo-lyft commented Jul 7, 2026

Uh oh!

jmarantz commented Jul 7, 2026

Uh oh!

davidvo-lyft commented Jul 7, 2026

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

davidvo-lyft Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

davidvo-lyft commented Jul 9, 2026

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

davidvo-lyft Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidvo-lyft commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davidvo-lyft commented Jul 6, 2026 •

edited

Loading