Encode tag ids with a colored slot and collapse dense presence to one occupancy long (phase 2) - #12046
Encode tag ids with a colored slot and collapse dense presence to one occupancy long (phase 2)#12046dougqh wants to merge 7 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>
dougqh
added
comp: core
Bits has a CI fix ready🟢 Investigated · 🟢 Fix prepared · ⚪ Validation skipped · 🟠 Ready Wrapped the dense-tag initialization comment in View in Datadog | Reviewed commit d6e7088 · Any feedback? Reach out in #deveng-pr-agent |
This comment has been minimized.
This comment has been minimized.
dougqh
changed the title
🟢 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
d6e7088 to
2bc5ae7
Compare
dougqh
changed the title
d758efe to
992dc14
Compare
2bc5ae7 to
bb9ebca
Compare
|
@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: bb9ebca1a0
ℹ️ 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".
| // makeTagId(serial=285, slot=13) <conditional> | ||
|
|
||
| public static final String HTTP_STATUS_CODE_NAME = "http.status_code"; | ||
| public static final long HTTP_STATUS_CODE_ID = 0x011E000A00000000L; |
There was a problem hiding this comment.
When an ID-based tag caller uses the advertised sign-bit routing, keyOf("http.status_code") now returns this positive ID, so isIntercepted is false even though TagInterceptor.needsIntercept explicitly handles this tag to populate DDSpanContext.httpStatusCode and apply the 404 resource-name rule. Encode this ID with INTERCEPTED, as is done for the other stored tags that require interceptor side effects.
Useful? React with 👍 / 👎.
| public static final String MEASURED_NAME = "measured"; | ||
| public static final long MEASURED_ID = 0x8009FFFF00000000L; | ||
| // makeTagId(serial=9, slot=NO_SLOT) + intercepted [directive] | ||
|
|
||
| public static final String ANALYTICS_SAMPLE_RATE_NAME = "analytics.sample_rate"; |
There was a problem hiding this comment.
The new reserved mappings use conceptual names rather than the keys handled by TagInterceptor: for example, the runtime keys are _dd.measured and _dd1.sr.eausr, so keyOf(DDTags.MEASURED) and keyOf(DDTags.ANALYTICS_SAMPLE_RATE) both return zero instead of these reserved IDs. The same mismatch affects _dd.origin (registered as origin) and service.name (only service is registered), preventing established tag constants from using the advertised name-to-ID/interceptor route.
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>
Reframe of the dense-store presence layer. Replaces the earlier two-tier (group-decl mask + field-decl bloom) design with a single global colored slot: the tag-id middle 16 bits carry one graph-colored slot coordinate (SLOT_SHIFT=32, SLOT_MASK=0xFFFF), so the dense store tracks presence with one occupancy long instead of a group mask plus a field bloom. Adds the trace-level bit (LEVEL_TRACE) and level-bit read-through in the parent visibility check. KnownTags remains hand-maintained here (src/main); the tag-registry code generator that produces these colored ids lands in the following commit, which relocates the file to src/generated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
992dc14 to
6948f86
Compare
bb9ebca to
4c90564
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c905641c2
ℹ️ 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".
| * rate — only the fast-path hit rate does. Unslotted stored tags ({@link KnownTagCodec#NO_SLOT}) | ||
| * all fold onto one shared bit ({@code slot & 63}); the scan stays authoritative for them. | ||
| */ | ||
| private long knownOccupancy; |
There was a problem hiding this comment.
perf: When dense tags retain their default-disabled setting, this eager occupancy field and knownTraceLevel still increase every TagMap instance from 48 to 56 bytes on HotSpot, even though none of the new filtering executes; DDSpanContext creates and retains one map per span, so this adds 8 bytes of allocation and corresponding GC traffic per span for the default production path. Verify the enabled/disabled allocation tradeoff with the existing JMH/JFR tooling and keep this state out of each map when dense routing is disabled.
AGENTS.md reference: AGENTS.md:L79-L81
Useful? React with 👍 / 👎.
| // makeTagId(serial=299, slot=6) + intercepted <required> | ||
|
|
||
| public static final String VERSION_NAME = "version"; | ||
| public static final long VERSION_ID = 0x012C000C00000004L; |
There was a problem hiding this comment.
perf: When dd.version is configured, InternalTagsAdder inserts version into each span's local TagMap, while CoreTracer.withTracerTags explicitly removes it from the read-through parent; marking this ID LEVEL_TRACE therefore sets knownTraceLevel on most local maps and prevents parentDenseVisible from taking its span-level fast skip, causing occupancy checks and potentially linear local-ID scans for trace-level parent tags during serialization. Clear this bit for version or make the level marker reflect the map tier where the tag is actually stored, and verify the read-through cost with the existing TagMap benchmark.
AGENTS.md reference: AGENTS.md:L79-L81
Useful? React with 👍 / 👎.
6948f86 to
4556153
Compare
1 participant
Reframe of the dense-store presence layer (was: two-tier presence bloom).
Replaces the earlier two-tier design (per-map group-decl mask + field-decl bloom) with a single global colored slot: the tag-id middle 16 bits carry one graph-colored slot coordinate (
SLOT_SHIFT=32,SLOT_MASK=0xFFFF), so the dense store tracks presence with one occupancy long instead of a group mask plus a field bloom. Also adds the trace-level bit (LEVEL_TRACE) and level-bit read-through in the parent-visibility check.KnownTagsstays hand-maintained here (src/main); the tag-registry code generator that produces these colored ids lands in #12047, which relocates the file tosrc/generated.Stacked on #12045 (dense store). Draft.
🤖 Generated with Claude Code