fix: prevent duplicate trace_id after CallTraceHashTable expansion#659
Draft
jbachorik wants to merge 2 commits into
Draft
fix: prevent duplicate trace_id after CallTraceHashTable expansion#659jbachorik wants to merge 2 commits into
jbachorik wants to merge 2 commits into
Conversation
…ROF-15396) Thread slot_base through LongHashTable generations so trace_id low bits stay unique across expansions instead of restarting from 0; guard against the slot range overflowing 2^32 with a counter for observability.
Contributor
CI Test ResultsRun: #29330594689 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-07-14 12:16:16 UTC |
Contributor
Benchmark Results (commit a6ab878)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124506625 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10287 ms (21 iters) | ✅ 10265 ms (21 iters) | ≈ -0.2% (±11.4%) | — / — |
| akka-uct | 25 | ✅ 8865 ms (24 iters) | ✅ 8823 ms (24 iters) | ≈ -0.5% (±9.4%) | — / — |
| finagle-chirper | 21 | ✅ 5948 ms (33 iters) | ✅ 6028 ms (33 iters) | ≈ +1.3% (±25.7%) | |
| finagle-chirper | 25 | ✅ 5455 ms (36 iters) | ✅ 5496 ms (36 iters) | ≈ +0.8% (±24.1%) | |
| fj-kmeans | 21 | ✅ 2847 ms (66 iters) | ✅ 2764 ms (68 iters) | 🟢 -2.9% | — / — |
| fj-kmeans | 25 | ✅ 2811 ms (66 iters) | ✅ 2804 ms (66 iters) | ≈ -0.2% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2063 ms (90 iters) | ✅ 2083 ms (89 iters) | ≈ +1% (±2.7%) | — / — |
| future-genetic | 25 | ✅ 2028 ms (91 iters) | ✅ 2101 ms (88 iters) | 🔴 +3.6% | — / — |
| naive-bayes | 21 | ✅ 1273 ms (134 iters) | ✅ 1273 ms (135 iters) | ≈ 0% (±33%) | — / — |
| naive-bayes | 25 | ✅ 1013 ms (169 iters) | ✅ 1013 ms (170 iters) | ≈ 0% (±31.4%) | — / — |
| reactors | 21 | ✅ 15847 ms (15 iters) | ✅ 16518 ms (15 iters) | ≈ +4.2% (±7.9%) | — / — |
| reactors | 25 | ✅ 18435 ms (15 iters) | ✅ 18690 ms (15 iters) | ≈ +1.4% (±4.3%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 1947 / 1968 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 2288 / 2290 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 8620 / 8482 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 8154 / 8569 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 3 | 1268 / 1261 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1302 / 1258 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 2907 / 2901 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 2961 / 2852 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 5 | 3511 / 3546 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 7 | 3475 / 3486 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1641 / 1558 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1886 / 1870 | ✅ / ✅ | ✅ / ✅ |
Contributor
Benchmark Results (commit 9747d1a)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124510382 Commit: ✅ Within expected boundariesNo significant runtime deltas (all within run-to-run noise) and no internal-counter outliers. Runtime details (per benchmark × JDK)
Internal counter details (ddprof)ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
CallTraceHashTable::put()could assign the sametrace_idto two different call traces once the table expanded: capacity doubles on expansion but the newLongHashTablegeneration's slot numbering restarted from 0, whileinstance_id(the upper 32 bits oftrace_id) stays fixed until the whole table retires. This PR threads aslot_basethrough eachLongHashTablegeneration so the low 32 bits oftrace_id(slot_base + slot) stay unique across all generations of one active tenure, and adds an overflow guard (with aCALLTRACE_STORAGE_EXPANSION_SKIPPEDcounter) that skips expansion rather than letslot_base + capacitycarry intoinstance_id's bits.Motivation:
An ASan stress test showed ~4% duplicate trace IDs after expansion, causing downstream JFR constant-pool/symbolication consumers to merge or misattribute samples for the colliding traces.
Additional Notes:
The expansion-overflow guard's boundary check and capacity-doubling logic are extracted into two pure, allocation-free static methods (
CallTraceHashTable::wouldExceedSlotIdRange,nextGenerationCapacity) so they can be unit tested directly — reaching the 2^32 boundary via realput()calls would require billions of inserts.How to test the change?:
New/updated gtest cases in
test_callTraceStorage.cpp:ExpansionProducesNoDuplicateTraceIdsTraceIdStableAcrossExpansionTwoExpansionsWithinOneTenureNoDuplicateTraceIdsCallTraceHashTableOverflowGuardTest.RejectsOnlyValuesThatExceedSlotIdRangeCallTraceHashTableOverflowGuardTest.NextGenerationCapacityIsDoubleRun via
:ddprof-lib:gtestDebug_test_callTraceStorage; all 18 cases pass.For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.Unsure? Have a question? Request a review!