Uh oh!
There was an error while loading. Please reload this page.
Wire mergedTracerTags as a read-through parent at span build (level-split) (phase 1a) - #11932
Conversation
🟢 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. |
7bb20f4 to
e65e58fCompare3a0a318 to
e692601Comparee65e58f to
3b9156eCompare
This comment has been minimized.
This comment has been minimized.
StringIndex is a compact open-addressed string→index structure (the keyOf substrate the dense tag store builds on): parallel hash/name arrays, linear probing, on par with HashSet on lookup at a smaller footprint. Includes unit tests, a footprint test (jol), and comparison benchmarks (vs HashSet/switch). No TagMap changes — standalone util. Rebased onto the level-split stack (consumer #11932) as the layer dense-store sits on. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3b9156e to
d2aa48fCompareStringIndex is a compact open-addressed string→index structure (the keyOf substrate the dense tag store builds on): parallel hash/name arrays, linear probing, on par with HashSet on lookup at a smaller footprint. Includes unit tests, a footprint test (jol), and comparison benchmarks (vs HashSet/switch). No TagMap changes — standalone util. Rebased onto the level-split stack (consumer #11932) as the layer dense-store sits on. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
e692601 to
b58bcc0Compared2aa48f to
1f257aaCompareStringIndex is a compact open-addressed string→index structure (the keyOf substrate the dense tag store builds on): parallel hash/name arrays, linear probing, on par with HashSet on lookup at a smaller footprint. Includes unit tests, a footprint test (jol), and comparison benchmarks (vs HashSet/switch). No TagMap changes — standalone util. Rebased onto the level-split stack (consumer #11932) as the layer dense-store sits on. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bef20fa to
50cf53dCompare1f257aa to
3237b90Compare50cf53d to
ae64f2aCompare3237b90 to
2225a42Compareae64f2a to
5cd8289Compare2225a42 to
ea094ceCompareThere was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:ea094ce0bc
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
More details
54 adversarial synthetic scenarios — simulating the full CoreTracer.buildSpan() wiring from the PR across 8 core behavioral equivalences and 5 edge cases — all passed. The read-through TagMap path is observationally identical to the old copy-down path for every tested scenario: trace-tag visibility, coreTags/contextualTags precedence override, builder-ledger tombstoning, multi-span parent isolation across config updates, and the version-exclusion change that prevents tombstone allocation on the shared frozen parent.
📊 Validated against 54 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit ea094ce · What is Autotest? · Any feedback? Reach out in #autotest
5cd8289 to
ab56044CompareA fresh, mutable TagMap can read through to a frozen parent on local misses, so a span can layer its own tags over a shared, immutable set (e.g. merged tracer tags) without copying them. - createFromParent(parent): the only way to attach a parent; the parent must be frozen and is fixed at construction (no re-parenting), so read-through can treat it as stable. Single-parent by design in phase 1. - Reads resolve local-first, then the parent; a local entry shadows the parent's (local-wins). Removing a parent key locally records a lazy tombstone (removedFromParent) so it stops reading through; the tombstone set is null until first needed, keeping the hot paths untouched. - size()/isEmpty() are exact (Map contract) and resolve the parent; isDefinitelyEmpty()/estimateSize() are the cheap conservative variants for the hot path. copy() preserves the parent and tombstones; forEach walks local then parent. Built on the folded final-class TagMap (#11967); composes cleanly with the null-tolerant Entry pathway (#11963). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ab56044 to
64c42e6Compareea094ce to
8c7bf14Compare…plit phase 1) Attach the trace's merged tracer tags to each span's TagMap as a frozen read-through parent (via TagMap.createFromParent) at span construction, instead of copying them into every span. The span sees the shared tags on read and only stores its own local tags, so the common trace-level bundle is held once per trace rather than duplicated per span. - CoreTracer builds the frozen merged-tracer-tags parent once; config version is kept out of that bundle. - DDSpanContext attaches the parent at construction (fixed, no re-parenting). - Adds TagMapReadThroughBenchmark (copy-down vs read-through, -prof gc). Stacked on the read-through mechanism (#11789), which builds on the folded final-class TagMap (#11967). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8c7bf14 to
170eedbCompareStringIndex is a compact open-addressed string→index structure (the keyOf substrate the dense tag store builds on): parallel hash/name arrays, linear probing, on par with HashSet on lookup at a smaller footprint. Includes unit tests, a footprint test (jol), and comparison benchmarks (vs HashSet/switch). No TagMap changes — standalone util. Rebased onto the level-split stack (consumer #11932) as the layer dense-store sits on. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…mall Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A live read-through parent is never definitely-empty: createFromParent drops an empty parent, copy() only forwards an already-vetted parent, and the parent is frozen so it can't become empty. So the size==0 / no-tombstones branch of isEmpty() always returned false via the parent chain walk — collapse it to a documented `return false`. Addresses mcculls' review comment on #11789. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dougqh
commented
Jul 25, 2026
Before / after: allocation drops ~130–200 B/op per spanRan the front-half span-creation microbenchmark across this PR's commit range ( Allocation — |
| Arm | Before | After | Δ |
|---|---|---|---|
| bareStartSpan | 1056.0 ± 0.0 | 896.0 ± 12.5 | −160.0 (−15.2%) |
| bareBuildSpan | 1049.6 ± 11.7 | 930.7 ± 49.2 | −118.9 (−11.3%) |
| jdbcClientSpan | 1498.1 ± 42.3 | 1295.4 ± 44.8 | −202.7 (−13.5%) |
| webServerSpan | 1768.0 ± 12.5 | 1640.0 ± 0.0 | −128.0 (−7.2%) |
| webServerSpanViaBuilder | 1949.3 ± 41.7 | 1752.0 ± 0.0 | −197.3 (−10.1%) |
The saving is ~130–200 B/op roughly flat across arms, not scaling with each span's own tag count — the signature of the level-split mechanism: it avoids cloning the fixed shared trace-level bundle, so every span saves about the same regardless of how many of its own tags it sets (even the "bare" arms drop ~150 B/op, since a tagless span still merges that bundle). All deltas clear their error bars.
Throughput — ops/us (directional corroboration only)
| Arm | Before | After | Δ |
|---|---|---|---|
| bareStartSpan | 5.30 ± 0.20 | 5.74 ± 0.52 | +8.4% |
| bareBuildSpan | 5.94 ± 0.31 | 5.77 ± 0.18 | −2.8% |
| jdbcClientSpan | 5.36 ± 0.14 | 5.48 ± 0.60 | +2.1% |
| webServerSpan | 4.24 ± 0.18 | 5.17 ± 0.24 | +22.0% |
| webServerSpanViaBuilder | 4.41 ± 0.09 | 4.98 ± 0.09 | +12.9% |
Throughput agrees directionally (less allocation → less GC pressure). The trustworthy points are the two web arms — tight error bars, clearing their intervals decisively. bareBuildSpan's −2.8% sits inside the combined error, so it's flat, not a regression; the wide after-side bars on bareStartSpan/jdbcClientSpan are per-fork bimodality — don't over-read them.
Methodology
- JMH:
@Threads(8),@Fork(3),@Warmup(5),@Measurement(5),@BenchmarkMode(Throughput),-prof gc. - Front-half only: a no-op
DropWriterdrops finished traces so no serialization/agent I/O leaks into the alloc number. - Tracer config via
@ForkjvmArgs (the realConfigpath, not hand-injected tags):-Ddd.service=petclinic -Ddd.env=staging -Ddd.version=1.2.3 -Ddd.tags=team:apm,dc:us1,cluster:prod-1,owner:tracing,tier:backend,region:us-east-1. - Commit range:
64c42e6(read-through mechanism, parent of this PR's commit) →170eedb(this PR — wires the parent in). - Primary signal is
gc.alloc.rate.norm(deterministic B/op); throughput is directional-only (thermal/bimodality-fragile).
Note: with a bare (unconfigured) tracer this win is invisible — mergedTracerTags is near-empty, so there's nothing to avoid copying. Same code, the only variable is whether the tracer is configured like production.
Bits Code status: ✅ Done Comment @DataDog to request changes |
There was a problem hiding this comment.
Parent-backed TagMaps diverge from their flat predecessors in two paths: fillMap/fillStringMap omit inherited tracer tags, and a fully tombstoned ancestor chain reports non-empty. The first can silently strip configured tags from callers that materialize a span’s tag map.
🤖 Datadog Autotest · Commit 0bbefb0 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Both bulk-fill helpers walked only this.buckets, so a parent-backed TagMap materialized through them dropped tags visible only via the parent chain and ignored shadowing/tombstones -- unlike putAll, forEach, and iteration, which already honor the read-through union. Branch to the visible-union walk (forEach) when a parent is attached, mirroring putAllOptimizedMap; keep the untouched local-only loop for the common no-parent case so it pays zero overhead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
createFromParent used isDefinitelyEmpty(), which only checks each level's local size and ignores shadowing/tombstones. With multi-level read-through, a frozen intermediate can be observationally empty (no local entries, every inherited key tombstoned) yet report isDefinitelyEmpty() == false because a farther ancestor still holds entries. Attaching such a parent then let a child take isEmpty()'s no-tombstone fast path and return false while size(), lookup, and iteration all report empty -- a Map-contract violation. Use exact isEmpty() so semantically empty parents are dropped, which also restores the invariant isEmpty()'s fast path relies on: an attached parent always contributes at least one visible entry. The hot path is unaffected -- isEmpty() short-circuits on size != 0, only walking tombstones in the rare all-tombstoned case this fix targets. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…through-consumer # Conflicts: # internal-api/src/main/java/datadog/trace/api/TagMap.java # internal-api/src/test/java/datadog/trace/api/TagMapReadThroughTest.java
/merge |
View all feedbacks in Devflow UI.
The expected merge time in
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
StringIndex is a compact open-addressed string→index structure (the keyOf substrate the dense tag store builds on): parallel hash/name arrays, linear probing, on par with HashSet on lookup at a smaller footprint. Includes unit tests, a footprint test (jol), and comparison benchmarks (vs HashSet/switch). No TagMap changes — standalone util. Rebased onto the level-split stack (consumer #11932) as the layer dense-store sits on. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
What Does This Do?
Splits trace-level tags & span-level tags into two separate TagMap-s
To maintain full semantic compatibility, this is done via the read-through parent capability introduced in #11789.
Motivation
Reduces some allocation by avoiding the cloning of the BucketGroup collision chain
Reduces span creation time by replacing O(n) map-walk with O(1) referencing of parent TagMap
Additional Notes
Stacked on #11789 (TagMap read-through mechanism) — review that first.
Level-split phase 1: the wiring. Attaches
mergedTracerTagsas a read-through parent at span build (gated on!mergedTracerTagsNeedsIntercept) instead of copying it into every span's storage. This is the change that actually activates read-through — #11789 alone is inert (parent == null).Commits: config-version handling out of the trace-level bundle, the read-through flip (
DDSpanContext.parentTags/CoreTracercopy-vs-share gate), and aTagMapReadThroughBenchmark.🤖 Generated with Claude Code