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 1 commit into
Conversation
dougqh
added
comp: core
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 |
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
changed the title
|
@codex review |
![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: 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 |
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 👍 / 👎.
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>
6948f86 to
4556153
Compare
1 participant
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