Add Accumulator: a striped long counter primitive as an alternative to LongAdder - #12351
Add Accumulator: a striped long counter primitive as an alternative to LongAdder#12351dougqh wants to merge 11 commits into
Conversation
…o LongAdder Enum-keyed long[]-per-stripe storage with cache-line padding, threadId&mask stripe selection, and combine+reset performed atomically under each stripe's own lock -- closing the non-atomic sumThenReset() loss window LongAdder has. Includes a JMH benchmark against LongAdder and the ConcurrentHashMap.computeIfAbsent(AtomicLong::new) anti-pattern. APMLP-1779 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…isions Sizing stripes to exactly availableProcessors() left collisions likely under real contention (birthday-paradox: n(n-1)/(2m) expected colliding pairs), and a collision costs a blocking synchronized wait rather than LongAdder's cheap CAS retry. Doubling the stripe count (floor 4) cuts accumulatorIncrement_highContention from ~0.097 to ~0.040 us/op at the cost of a pricier but far rarer accumulateAnd drain -- the right trade since inc/add run on every call while accumulateAnd runs on a reporting cadence. Benchmark javadoc updated with the re-measured numbers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Accumulator's realistic alternative isn't a single LongAdder but one per counter (there's no multi-counter LongAdder). Fresh instances make LongAdder look ~15x lighter, but that's an artifact of never having grown a Cell[] table under contention. Forcing real concurrent writes shows the opposite: 4 LongAdders under contention (17,560 bytes) end up over 7x heavier than Accumulator's fixed footprint (2,384 bytes), which is paid once at creation and doesn't grow with more contention or more counters, while each contended LongAdder keeps paying independently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Tests the hypothesis that a LongAdder-based helper which actually closes the same sumThenReset() reset hazard (one LongAdder per counter, a per-counter lock guarding both increment and drain) would cost about the same as Accumulator. It doesn't -- it's a clean trade-off inversion, not a wash: Accumulator's thread-sharded stripes win ~10x on the increment path, while the per-counter design wins ~24x on drain, but only because this benchmark has a single counter (its drain cost scales with counter count; Accumulator's is fixed at stripe count). Documented as a data point, not adopted -- both designs close the hazard, and the difference is negligible next to real request/span work either way. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
dougqh
added
type: feature
Rename accumulateAnd to accumulateAndReset, use ThreadSupport.threadId() instead of the deprecated Thread.getId(), add @GuardedBy annotations on the stripe-locked helpers, add @ParametersAreNonnullByDefault, and trim the javadoc (drop the Hashtable/FlatHashtable mention, the not-yet-built non-additive-counter escape hatch, and the C2-specific vectorization detail; shorten the LongAdder comparison). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new counter code has no confirmed defect. Its concurrency test can fail because it does not wait for the drain task, and its map benchmark does not measure the stated allocation path.
🤖 Datadog Autotest · Commit fc2f55c · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc2f55cd81
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
IMHO the benchmarks here should exercise the expected scenario of many threads calling increment, but only one thread periodically summing up the count. Currently they exercise many threads each calling increment and then the same incrementing threads all immediately summing up the count, which is not how it would be used in practice. |
amarziali
left a comment
amarziali
left a comment
There was a problem hiding this comment.
Automated review — request changes
The per-stripe synchronization protocol appears sound when all access goes through the provided operations: writers and combine/reset use the same stripe monitor, preventing the LongAdder.sumThenReset() loss window.
I found two blocking issues:
- The raw
long[][]API does not bind storage to its enum schema. A different enum can be passed toinc/addand silently update the wrong ordinal. Exposing the arrays also makes the synchronization protocol conventional rather than enforceable. - The concurrent drain test does not wait for its drainer to finish before asserting. It can fail against a correct implementation and can leave an active infinite task after a timeout.
The performance evidence also needs revision before its conclusions can support this abstraction:
- The drain benchmark creates many concurrent drainers instead of modeling many writers and one rare reporter.
- The footprint experiment expands unlocked
LongAdderinstances rather than measuring the correctness-equivalent locked alternative. - The stripe-collision explanation contains incorrect math and assumes a distribution not produced by
threadId() & mask. - The CHM measurement is pre-warmed and does not exercise the claimed allocation-under-lock path.
There is also an unmeasured deployment risk on JDK 21–23: virtual threads contending on these monitors during a drain may pin carrier threads. This should either be constrained in the contract or evaluated with a virtual-thread workload.
I recommend encapsulating the storage in an enum-bound owning type, repairing the drainer lifecycle test, and reshaping the measurements around the intended production topology before merging. Since there is no production caller in this PR, migrating one intended caller would also help validate the API and workload assumptions.
This was an automated, read-only review of head 89d980c0f829c6df51d22131aa35b760c529fdb4.
The original static, allocation-free Accumulator API let a caller index its long[][] with a different enum than the one it was created for -- compiles, but silently reads/writes the wrong slot. Move that raw API into a nested EmbeddingSupport namespace, and add a top-level Accumulator<E> that owns its storage and binds inc/add/update/ accumulateAndReset to one enum at construction, mirroring StringIndex's own EmbeddingSupport split in this package. Also fix the stripe-count javadoc: with n contending threads and m stripes, doubling m halves the expected number of colliding pairs (n(n-1)/(2m)), not quarters it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Update call sites to Accumulator.EmbeddingSupport.*, and add a test covering the new typed Accumulator<E> wrapper. - Fix a race in concurrentAccumulateAndDuringWritesNeverExceedsWritten: join the background drainer via Future.get() before the final drain and assertion, instead of racing it. - Assert accumulatorBytes < contendedAdderBytes in contendedFootprint -- the actual claim the test exists to back up, not just that the LongAdder side didn't shrink. - Correct the CHM benchmark's javadoc: the benchmark-scoped map means only the first warmup invocation allocates under the bin lock; every sampled op hits an already-warmed computeIfAbsent lookup. - Add a @Group-based accumulatorMixed-write/accumulatorMixed-drain pair modeling "many writers, one rare drainer," alongside the existing @threads(MAX) benchmark kept as a documented worst-case upper bound. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…iews Restores enum-ordinal type checking inside update()'s critical section and on accumulateAndReset()'s drained result, closing the last gap left by the EmbeddingSupport split. Stripe is constructed fresh under the held lock and is expected to be scalar-replaced by escape analysis for well-behaved (small, non-capturing, non-escaping) mutators; Counts wraps the already-drained array and is a real but infrequent per-drain allocation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…port Pairs typedIncrement/typedUpdate/typedAccumulateAndReset against their EmbeddingSupport equivalents so the wrapper's cost is directly visible: inc/update should track the raw calls closely (Stripe is designed to scalar-replace), while accumulateAndReset is expected to run measurably slower by roughly one small allocation per drain (Counts escapes by design). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…mark javadoc Confirms the wrapper's cost is not measurable: typedIncrement/typedUpdate track EmbeddingSupport within noise (the Stripe scalar-replaces as designed), and typedAccumulateAndReset tracks the raw drain within noise at low contention (the Counts allocation doesn't show up at this granularity). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d count The benchmark calls EmbeddingSupport.accumulateAndReset, not accumulateAnd -- rename to match. Also cap contendedFootprint's thread count at 16: uncapped availableProcessors() on a high-core build agent spins up one thread per core, all busy-spinning for two seconds, which can dominate the host during a parallel test run for no added signal over a fixed small contention level. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Agreed, and this is now in there: |
3 participants
What Does This Do
Adds
Accumulator, an enum-keyed, stripedlong[]-per-stripe counter primitive that closesLongAdder.sumThenReset()'s documented non-atomic reset hazard: increments landing between sum and zero are silently and permanently lost withLongAdder;Accumulator.accumulateAndResetcombines and resets each stripe under the same lock guarding its writers, so nothing can land in the gap.Accumulator.stripeCount()javadoc for the birthday-paradox reasoning and the before/after benchmark numbers).AccumulatorBenchmark) compares againstLongAdder, aConcurrentHashMap.computeIfAbsentanti-pattern, and a per-counter-lockedLongAdderalternative, at low/high contention, with real measured numbers and honest interpretation in the javadoc.AccumulatorFootprintTest) compares retained bytes against NLongAdders fresh vs. under real contention.No caller wired in yet — this is the toolkit primitive from APMLP-1779.
Motivation
The realistic alternative to this primitive is N separate
LongAdderfields, one per counter. That has two problems this PR fixes:LongAdder#sumThenReset()is documented as not atomic against concurrent updates — an increment landing on a cell after it's summed but before it's zeroed is silently and permanently lost.create()/accumulateAndReset()call site instead of N, andupdate()gives atomic multi-counter mutation that a pile of independentLongAdders can't.Migrating a real caller (e.g.
OtlpTelemetryorPayloadDispatcherImpl, both confirmed to use thesumThenReset()pattern per APMLP-1780) is the natural follow-up.Additional Notes
Accumulator's fixed up-front cost ends up lighter than contendedLongAdders once measured under the load it will actually see in production — see the JOL footprint test../gradlew :internal-api:test --tests "datadog.trace.util.Accumulator*"— unit + footprint tests pass./gradlew :internal-api:jmhJarbuilds; JMH benchmark run manually with real numbers captured in javadoc./gradlew :internal-api:spotlessCheckclean/techdebtand/perf-reviewrun over branch changes — no findingsContributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labels (none set yet)close,fix, or any linking keywords when referencing an issueJira ticket: APMLP-1779