Store known span tags densely in TagMap by tag-id (phase 2) - #12045
Store known span tags densely in TagMap by tag-id (phase 2)#12045dougqh wants to merge 6 commits into
Conversation
A 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>
…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>
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>
… gate Re-applies the coverage fix dropped by the branch rebase/restack. jacocoTestCoverageVerification flags StringIndex at 0.7 instruction coverage (min 0.8): the instance long[] API (mapLongValues / lookup / lookupOrDefault) and the Support.numSlots(int[]) static were never exercised. Add two tests covering them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… capacityFor Two consistency changes in light of the FlatHashtable strategy family: - Rename Support -> EmbeddingSupport. StringIndex is the LightMap/object-side member (a real object with static factories), and its static-over-raw-arrays tier is exactly the "embed the backing arrays in your own fields" role that LightMap.EmbeddingSupport names. - Replace tableSizeFor with capacityFor(n[, loadFactor]) + DEFAULT_LOAD_FACTOR / LOW_LOAD_FACTOR, mirroring FlatHashtable's sizing (duplicated for now; the two branches are independent, to be unified when the family converges). This also tightens the sizing: the old `while (size <= n)` over-allocated 2x at power-of- two counts (capacityFor(16) is now 32, was 64) while still targeting load factor <= 0.5. capacityFor(0) stays valid (StringIndex allows the empty set). Updates StringIndexTest (sizing expectations + a rejects test) and the three benchmarks referencing the tier. Behavior unchanged except the tighter default table size. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bits has a CI fix ready🟢 Investigated · 🟢 Fix prepared · ⚪ Validation skipped · 🟠 Ready
View in Datadog | Reviewed commit d758efe · Any feedback? Reach out in #deveng-pr-agent |
|
🎯 Code Coverage (details) 🔗 Commit SHA: 6948f86 | Docs | View more details | Give us feedback! |
🟢 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. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d758efe1d9
ℹ️ 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".
| if (Boolean.getBoolean("dd.trace.dense.tags.enabled")) { | ||
| KnownTags.init(); |
There was a problem hiding this comment.
When dd.trace.dense.tags.enabled is true for the first tracer, CoreTracerBuilder.config() has already constructed and frozen defaultSpanTags and localRootSpanTags while the codec was inactive, so known names such as env are stored in buckets before this call activates dense routing. Subsequent lookups search only the dense region for those names, and parent shadow checks assume a known tag can never be bucketed; consequently point reads/removals can miss configured tags and a local dense override can be serialized alongside the bucketed parent value. Initialize the codec before these maps are built, or migrate/fall back to pre-registration bucket entries.
Useful? React with 👍 / 👎.
| private Object knownRawValue(long tagId) { | ||
| int i = this.knownIndexOf(tagId); | ||
| return i < 0 ? null : this.knownValues[i]; |
There was a problem hiding this comment.
With dense routing active, Map.put(knownTag, null) appends an entry and increments knownCount, but this lookup returns null for both an absent ID and a present null value, after which getLocalEntry treats the mapping as absent. The map then reports the entry in size() and iteration while containsKey returns false, unlike the previous bucket implementation and the Map contract; presence must be determined from the index rather than the stored value.
Useful? React with 👍 / 👎.
| EntryReadingHelper reader = this.denseReader; | ||
| if (reader == null) { | ||
| reader = this.denseReader = new EntryReadingHelper(); | ||
| } | ||
| reader.set(KnownTagCodec.nameOf(tagId), value); |
There was a problem hiding this comment.
When a caller retains elements from TagMap.iterator() or TagMap.stream()—for example with stream().collect(toList())—every dense element is represented by this same mutable EntryReadingHelper. Advancing the iterator rewrites previously returned elements, so a collected list contains repeated aliases representing the final dense tag instead of the map's entries; bucket entries were stable before this change. Public iteration must return stable readers, or the reusable flyweight must be confined to a separately documented immediate-consumption traversal API.
Useful? React with 👍 / 👎.
| * — {@code keyOf}/{@code nameOf} depend only on globalSerial + name, not on the (dormant) | ||
| * positional layout. | ||
| */ | ||
| class KnownTagsTest { |
There was a problem hiding this comment.
This test class references KnownTags and permanently registers the global resolver, but its name does not match the build's *ForkedTest* isolation convention, so it runs in the shared test JVM and changes dense routing for every later test in that worker. That makes bucket-only coverage and failures order-dependent; rename it with the ForkedTest suffix so registration cannot leak between test classes.
AGENTS.md reference: AGENTS.md:L63-L63
Useful? React with 👍 / 👎.
d758efe to
992dc14
Compare
|
@codex review |
Known tags (keyOf resolves to a stored id) are held in insertion-ordered parallel arrays (knownIds/knownValues) with NO per-tag Entry object — the allocation lever. Lazily allocated on the first known-tag write; custom tags stay in the hash buckets. Disjoint by construction (known-ness is global), so read-through shadow checks stay within-region and the bucket path is unchanged. - KnownTagCodec (id encoding + resolver) + hand-written KnownTags (keyOf substrate over StringIndex). - The KnownTagCodec is ALWAYS present: CoreTracer registers the real resolver unconditionally at init (so keyOf/nameOf name resolution is always live — the OTel-name mapping later in the stack depends on this), and on first use with nothing registered the codec lazily installs an empty NoKnownTagCodec. Either way it locks after first use, so a map can never be built half-bucketed then half-dense by a late registration. - Dense ROUTING is decoupled from resolution and gated separately by the const-folded KnownTagCodec.DENSE_STORE, captured from the new trace.experimental.dense.tags.enabled Config flag (off by default). TagMap computes keyOf only under this gate, so when off the dense branches dead-code-eliminate and tag storage is byte-identical to the bucket store. - Sizing is a generous fixed stopgap (KNOWN_INIT_CAP=12, the per-type max); exact per-type sizing comes with the tag registry. Reconciled onto the level-split stack (fold + read-through + StringIndex); built on the folded final-class TagMap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 992dc142b4
ℹ️ 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".
| for (int i = 0; i < this.knownCount; ++i) { | ||
| map.put(KnownTagCodec.nameOf(this.knownIds[i]), this.knownValues[i]); |
There was a problem hiding this comment.
When a TagMap is created with createFromParent, fillMap walks only the child's local buckets and dense arrays, so tags visible solely through the parent are silently omitted even though get, size, iteration, and the collection views expose them. This makes exporting a read-through map differ from exporting an equivalent flat map; traverse the visible parent union as forEach does. fillStringMap has the same omission.
Useful? React with 👍 / 👎.
992dc14 to
6948f86
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6948f86d96
ℹ️ 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".
| Object known = this.knownRawValue(id); | ||
| return known == null ? null : Entry.newAnyEntry(tag, known); |
There was a problem hiding this comment.
perf: When dense routing is enabled, every getObject, getString, or containsKey call for a present known tag allocates a new Entry here. This is reachable on the per-span finish path—for example, HttpEndpointPostProcessor reads http.method, http.route, and http.url—while the added allocation benchmark only exercises iteration, whose flyweight avoids this branch. Provide direct dense-value access for point reads and verify the finish path with an allocation profile.
AGENTS.md reference: AGENTS.md:L77-L81
Useful? React with 👍 / 👎.
| public void set(@Nonnull String tag, int value) { | ||
| this.getAndSet(Entry.newIntEntry(tag, value)); | ||
| long id = KnownTagCodec.DENSE_STORE ? KnownTagCodec.keyOf(tag) : 0L; | ||
| if (KnownTagCodec.isStored(id)) { | ||
| this.putKnownLocal(id, tag, Integer.valueOf(value)); |
There was a problem hiding this comment.
perf: With dense routing enabled, setting a known numeric tag now boxes the primitive before storage; for example, BaseDecorator.onPeerConnection sets the known peer.port tag on client spans, and ports outside the Integer cache can allocate an Integer per span. The bucket path previously retained the value in Entry.rawPrim without boxing, and the new JMH benchmark uses only String values, so it does not measure this regression. Preserve an unboxed representation or add a representative numeric allocation benchmark before enabling this path.
AGENTS.md reference: AGENTS.md:L77-L81
Useful? React with 👍 / 👎.
| private final boolean jdkSocketEnabled; | ||
|
|
||
| private final boolean spanBuilderReuseEnabled; | ||
| private final boolean traceDenseTagsEnabled; |
There was a problem hiding this comment.
The new configuration is stored and exposed through a getter but is omitted from Config.toString(), contrary to the repository's configuration checklist. As a result, startup diagnostics and tracer-flare configuration dumps cannot show whether this experimental storage mode was requested, making failures specific to dense routing substantially harder to identify; add traceDenseTagsEnabled to the serialized configuration state.
AGENTS.md reference: AGENTS.md:L37-L43
Useful? React with 👍 / 👎.
What
Adds a dense store for known span tags in
TagMap: tags resolved to a stable tag-id viaKnownTagCodec.keyOfare held in insertion-ordered parallel arrays (knownIds/knownValues) with no per-tagEntryobject — eliminating theTagMap$Entryallocation that macro JFR profiling flagged as the #1 tracer allocation lever.KnownTagCodecis always installed — the real resolver viaKnownTags.register()at tracer init, or a lazily-locked emptyNoKnownTagCodecnull-object if nothing registers. SokeyOf/nameOfare always available, but whether known tags then take the dense storage path is a separate decision.static final DENSE_STORE(trace.experimental.dense.tags.enabled, default off). When off, HotSpot dead-code-eliminates thekeyOfcall and the dense branches, so the default path is byte-identical to the bucket-only store — no new work on the hot path.EntryReadingHelper) — no per-entryEntryalloc on the read/serialize path either.parentDenseVisible(mirrorsparentEntryVisible), nearest-level-wins with tombstone/shadow checks. Disjointness (known tags never bucket) keeps the two stores independent by construction.Why
Removes
TagMap$Entryallocation for known tags (the macro alloc win — see the tracer-overhead JFR profiling). CPU is neutral/parity; the headline is allocation on the app thread.Decoupling name-resolution from dense-routing lets the codec register unconditionally (needed by later PRs in the stack — e.g. OpenTelemetry name resolution) without forcing every tag through the dense store: the store stays opt-in and off-by-default while resolution becomes always-on.
Stack
Sits on
dougqh/tagset(StringIndex / #11660 base). Supersedes the old dense PR #11814.Test
TagMapDenseForkedTest,TagMapDenseFuzzForkedTest(dense on),KnownTagsTest+ default-offTagMapTest/TagMapFuzzTestgreen;spotbugsMain+spotlessJavaCheckclean.🤖 Generated with Claude Code