Repository navigation
Conversation
TagMap now holds its tags in two internal stores: an IdStore for known tags by id and a name store for custom tags by name. In hash mode, the only mode so far, one HashStore serves as both, so the split adds no object per tag. The bucket structure, read-through to frozen parents and removal tombstones move into HashStore unchanged; TagMap delegates to it. The public API and behavior are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TagRouting resolves a tag name once, by KnownTagCodec.keyOrAliasOf, and routes the operation to the store that holds it: a known tag to the id store by its id, anything else to the name store by its name. TagMap's name-keyed getters, setters, removals and entry factories, and its resolved variants, all go through it, so name-to-id resolution lives in one place. A direction-scoped alias is still stored as a custom tag. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DDSpanContext now owns a HashStore (and, outside hash mode, an IdStore) instead of a TagMap, created when the span is constructed and read through to the frozen tracer-level tags. Every tag write and read goes through TagRouting under one lock, the HashStore itself. Direction, alias, interception and pending-alias handling stay in DDSpanContext and DirectionalTags, now applied to the stores. HashStore is no longer an IdStore: it is the name-keyed hash, which also holds known tags by id, and exposes an IdStore adapter over them. The static HASH_MODE flag lets hash mode call the hash directly, so a span needs no adapter object. Internal readers (tag post-processors, the serializer) get a short-lived TagMap view over the span's stores whose name-keyed reads honor the span's direction; getTags() is still a snapshot copy. TagMap.Internals is gone: the store API is the direct path. Its tests move to TagStoresTest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TagRouting.putAll built a TagMap view to reuse TagMap.putAll, but putAll is too big to inline, so the view escaped and cost an allocation on the span's tag-seeding path. In hash mode it now copies into the HashStore directly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The view handed to the post-processor chain escapes, so building one per use cost an allocation at trace write and another at serialization. The span now creates one lazily, under its tag lock, and reuses it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A direction-scoped alias written before the span has a kind is no longer stored in the span's HashStore. It goes into a small holder that DirectionalTags owns, which stays null until the first such write, so the HashStore holds only custom tags. Kind-set resolution walks only the holder, and once every alias has resolved the holder is dropped, so the span returns to the fast path rather than staying flagged for life. The id setters' routing gate now tests the holder for null instead of reading a sticky flag. Reads of a tag with an alias probe the holder only when there is one. Last-write-wins and the removal rules are unchanged. Emitting the span's tags (trace-write and serialization processing, and the getTags snapshot) copies the holder's entries in under their literal names, so a span that never learns its kind still emits them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
The tag-registry generator now assigns each known tag a slot in id bits [47-32] by graph coloring: each concrete span type's resolved tags, and the trace-level tags, form a clique and get distinct slots. So 1L << slotOf(id) is an exact presence bit for a span's tags, and one long holds them all. Tags on no modeled span type get NO_SLOT, which a long shift maps onto bit 63; real slots are capped below it. KnownTagCodec gains SLOT_SHIFT, NO_SLOT, slotOf, hasSlot and slotCount. Nothing in the tracer reads slots yet: this is split out of the dense store (#12856) so the store, the span builder's tag ledger and a map-level routing summary can each build on it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
hasSlot(0L) decoded slot 0, so the id keyOf gives a custom tag looked like the known tag in slot 0. It now returns false for 0L. The slot-count test also allows the full 63 slots the generator permits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A span can still hold two tags that share a slot, so slotOf's presence bit is exact only when it is clear. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each half of a TagMap now has its own bucket array. HashIdStore holds the known tags by id, with a count, a final parent, and slot masks from the tag ids' co-occurrence slots: a clear occupancy bit is a definite miss, and the parent's slots let a read-through miss stop early. Removing an inherited known tag puts that tag's shared, immutable tombstone entry in the local buckets, so it allocates nothing. HashStore holds only custom tags and is created on the first custom write; until then a child borrows its parent's custom store read-only. The raw bucket mechanics move into a stateless HashSupport whose operations report what they did through return values. HASH_MODE is gone. A span now keeps its tags in a TagMap it creates with the span (TagMap.forSpan), locks on it, and routes to it through TagRouting; that TagMap is what its internal readers get, replacing the cached view. DirectionalTags' store-only methods are removed, and its test now exercises the methods the span uses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DDSpanContext no longer routes its reads and writes through a TagMap. It owns its id store, created with the span, and its custom store, created on the first custom write and until then null or borrowed from the read-through parent. Every span read and write goes span to store, and the span locks on its id store, which always exists. The TagMap is again only a cached view for the span's tag post-processors, the serializer and the getTags() snapshot. Before each use the span points it at its current custom store; afterwards it adopts any custom store a reader created through the view. DirectionalTags works on the span's stores through a small TagStores interface the span implements; its test supplies one over a TagMap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
process_id, _dd.trace_span_attribute_schema and _dd.agent_psr are now declared at the trace level, beside language and runtime-id, with the types the tracer writes them with (long, int and double). Every local root span carries them, so as custom tags they made each root span create a custom tag store; as known tags they go to its id store. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DDSpanContext no longer implements a store interface for DirectionalTags or exposes store operations publicly. DirectionalTags now takes the span's id store and its custom store, which may be null or borrowed from the read-through parent. It never creates the custom store: a removal that would have to hide a tag in a borrowed store leaves it untouched and returns true, and the span creates its own store and repeats the removal, which is idempotent. An alias write that resolves to its tag now leaves removing the alias from the custom store to the span. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The id setters now supersede a pending alias themselves, before routing, instead of routing every write of an aliased tag through TagInterceptor while the span held one. TagInterceptor routes on INTERCEPTED alone again and no longer knows aliases exist; DDSpanContext.supersedeAliases is private. The check comes first as hasScopedAlias(id), so for a constant id without an alias it folds away entirely. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TagMap.DirectionSource is gone. The span's cached view now gets the span's direction and whether it may hold pending aliases as two plain fields, set by attach() alongside its current custom store before each use, and a TagMap of its own reads every name as written (direction unknown). DDSpanContext no longer implements an interface for its view, so tagDirection() and hasPendingAliases() are no longer public; its public surface matches master. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
HashIdStore's lookups (reads, the find before a removal, and the read-through visibility check) now match an entry on its 64-bit id through HashSupport.findById, so they no longer resolve the tag's name with nameOf or compare names. Every id-store entry is keyed by id, and the stored bucket hash still filters before the id compare. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| } | ||
|
|
||
| /** An empty parentless store over one bucket, for {@link TagMap#EMPTY}, which is frozen. */ | ||
| static HashIdStore newEmpty() { |
There was a problem hiding this comment.
To Claude - let's move this to the top of the class
I tend to put static parts first -- then instance variables -- then instance methods
Admittedly, I'm not completely strict about this
There was a problem hiding this comment.
Moved newEmpty and its constructor to the top, then the static HashCursor. I did the same in DenseIdStore (DenseCursor now follows the statics). (030fc85)
🤖 Claude
| @Override | ||
| public void set(long tagId, boolean value) { | ||
| this.put( | ||
| new Entry( |
There was a problem hiding this comment.
To Claude - why aren't we using the helper static methods here - newBooleanEntry, etc
Applies to all the prim overloads
There was a problem hiding this comment.
No good reason: they came over from the old Internals.setKnown, which used the 5-arg constructor to skip requireKnownName's null check, since callers validate first. That check is one compare, so the typed setters now use Entry.newAnyEntry / newObjectEntry / newBooleanEntry / … . (030fc85)
🤖 Claude
| } | ||
| if (this.tombstoneMask != 0L) { | ||
| // A bucket merge would count a replaced tombstone as a replacement, so put one at a time. | ||
| HashSupport.forEach(that.buckets, this, (self, entry) -> self.put(entry.entry())); |
There was a problem hiding this comment.
To Claude, maybe, inside this code we call the EntryReader - entryReader
entry.entry reads a little weirdly
There was a problem hiding this comment.
Renamed: the EntryReader parameters in IdStore/HashIdStore/DenseIdStore and DDSpanContext.putEntry are now entryReader, and the local Entry is entry, so it reads Entry entry = entryReader.entry();. (f634ec7)
🤖 Claude
- IdStore.get(long) -> getEntryReader, getKnownValue -> getValue. - put/getAndPut take an EntryReader: the hash store keeps its Entry (free when it is one), the dense store copies the value, so a span storing a flyweight read from a dense map builds no Entry. - HashIdStore's typed setters use the Entry factories. - Static members first: the Cursor type in IdStore, newEmpty and the cursors in the stores. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| * custom store, if any, read-only. No locking: guarded by the owner's lock. | ||
| */ | ||
| @Restricted(allowedIn = {"datadog.trace.core"}) | ||
| public final class HashStore { |
There was a problem hiding this comment.
For others, HashSupport, HashStore, and HashIdStore are where the guts of the old TagMap now live. This code is largely unchanged -- just significantly rearranged.
| * ForkedTest}) with the flag set before anything reads it. The shared suites cover the dense | ||
| * store's behavior when run with {@code DD_TRACE_EXPERIMENTAL_DENSE_TAGS_ENABLED=true}. | ||
| */ | ||
| class DenseIdStoreForkedTest { |
There was a problem hiding this comment.
To Claude - this might be motivation for the SingleLoadedImplementor annotation
| * under one lock that guards both: {@code tagIds}, which always exists. Internal readers get a | ||
| * cached {@link #tagView() view}. | ||
| */ | ||
| private final IdStore tagIds; |
There was a problem hiding this comment.
To Claude - let's rename tagIds -> knownTags
There was a problem hiding this comment.
Renamed to knownTags, including the synchronized (knownTags) sites and the test that reads the field reflectively. (f634ec7)
🤖 Claude
Prose only. Brings comments in line with the code after the #12731 merge and the dense port: no direction-scoped aliases or lookup sentinels in the core, put/getAndPut take an EntryReader, only the hash id store keeps Entry tombstones, the dense store keeps no Entry objects, and the root-span tags' floor precedence covers header tags and prototype defaults too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TagRouting was left holding small helpers once direction handling moved to the OpenTelemetry boundary. The span and the TagMap view each route by name or id themselves (KnownTagCodec.keyOf, then the id store or the custom store), and the pieces core can't build itself, because they reach TagMap's package-private stores, become @restricted statics on the types they belong to: IdStore.create, HashStore.inheritedFrom / create and TagMap.readThroughParent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- DDSpanContext.tagIds -> knownTags. - @GuardedBy("knownTags") on the span's tag-store fields, with Javadoc on knownTags describing the storage and the one-lock scheme (reentrant, frozen parents read lock-free, the unsafe accessors). - EntryReader parameters are named entryReader, so entryReader.entry() no longer reads as entry.entry(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| } | ||
|
|
||
| /** The custom store to write to: created on the first custom write, ending any borrowing. */ | ||
| private HashStore customTagsForWrite() { |
There was a problem hiding this comment.
To Claude - Hmm - I guess this optimization makes sense because most of the customTags exist in the parent layers. I don't like the complexity, but I can buy that it pays off.
The span's view now keeps the stores it was created over. When the span creates its own custom store, it drops the view, and the next bulk reader creates a new one. TagMap.attach goes away. The span copies a map's tags straight into its stores rather than through the view, so the view is created only by the bulk readers late in a span's life, and dropping it is rare. Adoption is skipped for a view the span dropped during its use, so a custom store the span created in a callback isn't replaced by the view's older one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| } | ||
| } | ||
|
|
||
| private void setResolved(long key, String tag, double value) { |
There was a problem hiding this comment.
To Claude - renamed to routeSet
There was a problem hiding this comment.
Done: setResolved and the name-keyed storeTag overloads are both routeSet now, by id plus name or by name only. (9a5583f)
🤖 Claude
The private helpers that send a tag to the known or the custom store are now routeSet, routePut, routePutAll and routeRemoval (from setResolved / storeTag, putEntry, putAllTags and removeStored), so a write that picks its store reads differently from one that already has it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
|
||
| @Override | ||
| public Object getValue(long tagId) { | ||
| Entry entry = this.getEntryReader(tagId); |
There was a problem hiding this comment.
To Claude - let's also add getEntry just so this reads a bit more clearly
|
|
||
| @Override | ||
| String toInternalString() { | ||
| StringBuilder ledger = new StringBuilder(); |
There was a problem hiding this comment.
To Claude - let's call this builder not ledger
There was a problem hiding this comment.
Renamed to builder, here and in TagMap's two toStrings that used the same name. (a722412)
🤖 Claude
DenseIdStore was the helper's only user, so its by-name and Map.Entry modes were dead. It becomes DenseIdStore.DenseReader, an id-only flyweight: tagId() is a field read and entry() needs no branch, with only the name resolved lazily. TAG_ID_NOT_COMPUTED goes with it. IdStore.Cursor becomes an interface, so DenseCursor extends DenseReader and returns itself from next(): one object per iteration instead of a cursor plus a helper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| private Entry nextEntry; | ||
| // Two phases: the id store's visible entries, through its own cursor, then the custom store's | ||
| // chain -- the leaf's buckets first, then each ancestor's in turn (read-through union). | ||
| private IdStore.Cursor ids; // null once the id phase is done |
There was a problem hiding this comment.
To Claude - let's call this idCursor
From review: TagMap's ids / names / namesBorrowed are now knownStore / nameStore / nameStoreBorrowed (and namesForWrite is nameStoreForWrite); the iterator's id cursor is knownCursor. toInternalString's local StringBuilders are builder rather than ledger. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The interception lambdas in seedAbsentTags and setAllTags passed `this` where the base passed `ctx`, which made each a capturing lambda allocated per call; ctx is this, so they pass ctx again. HashIdStore's and HashStore's visibleParentCount counted through forEachParent with an int[] counter, allocated on every size() of a store with a parent (per serialized span); they count with a plain loop again, as the base did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- isPresent reads the reader's own tagId instead of resolving the name again. - createFromParent delegates to readThroughParent, which now carries the exact-emptiness comment; TagMap(TagMap) uses IdStore.create and HashStore.inheritedFrom; nameStoreForWrite uses HashStore.create. - HashSupport.checkBuckets indexes a bucket group's entries by j, not the bucket index i, so the integrity check really inspects groups (copied as-is from the old TagMap.checkIntegrity). - DDSpanContext's constructors drop the tagsSize parameter, which nothing used once the span owned its stores, and CoreTracer no longer computes it per span (that included a size() walking the read-through parent). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Only its own test called it; the span reads values from its stores directly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- isPresent moves above setTag(String, Object)'s Javadoc, which it had been inserted under, so setTag is documented again. - A leftover constructor doc stacked on HashStore.inheritedFrom's is removed. - TagMap.EMPTY's comment described the old length-1 bucket array; it now points at IdStoreFactory, which explains why building EMPTY is safe during class initialization. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What Does This Do
Splits tag storage into strict id and name stores, and has the span own them directly.
TagMapbecomes a thin view for everyone else. No behavior change.The idea: classify a tag name once, at the boundary (known tag → id, custom tag → name, direction-scoped alias → the span's call), then keep the two kinds apart inside.
Layers
KnownTagCodecHashStoreIdStoreHashIdStore) now; a dense store slots in hereTagRoutingDDSpanContextHashStore(also its one lock) and, outside hash mode, anIdStore. All setting logic lives here, withDirectionalTagsandTagInterceptorTagMapNotable changes
TagMap.Internalsis gone. The strict store API is the direct path, so core no longer needs a trusted side door aroundTagMap. Its tests move toTagStoresTest.HASH_MODEis a compile-time constant (currentlytrue). Each id operation isHASH_MODE ? hash.x(id) : ids.x(id), so hash mode has no adapter object and no extra load.TagMapview over the span's stores.getTags()is still a snapshot copy.Entryobjects and clone onlyBucketGroupchains, as before.Motivation
TagMaphad accreted name canonicalization, known-versus-custom classification and, with #12731, direction awareness. That forced core to reach around it throughInternals, and every write route repeated the known-versus-custom and direction logic. Most of #12731's review findings were one write route missing a rule another route had.This puts policy back in the span layer, keeps storage simple and fast, and gives a dense store a clean place to replace the known-tag half. That matters for the biggest remaining allocation sites: point reads building an
Entryper read, primitive metrics stored as boxedEntrys, and the dense store's id arrays.Additional Notes
Performance (JDK 17,
-f 2,-prof gc; base is #12731's tip6aa6d4f1e2):SetTag.knownTag_byIdSetTag.knownTag_byNameSetTag.customTag_byNameGetTag.knownTag_unsafeGetTagGetTag.knownTag_getTagSpanCreation.webServerSpanSpanCreation.bareStartSpan(4 forks)Throughput is within noise. With the adapter instead of the folded
HASH_MODEpath,unsafeGetTagwas +6% and bare id writes +4%.Known cost: +40 B per span. The span's view, handed to the tag post-processor chain, escapes there and isn't scalar-replaced, so the view is cached per span (32 B plus a field). This is a stopgap: the post-processors are due for rework, and once they read the span's stores directly, the view and the field go away. Without the cache it was +32 B on the app thread and another +32 B at serialization.
Semantic note: name-keyed reads through a span's view resolve aliases by the span's direction, where today's raw
unsafeTagsreads don't. No current post-processor reads an alias name.Draft: stacked on #12731's branch, so this diff shows only the split. A follow-up commit moves pending aliases into a small lazily created holder: the custom store then holds only custom tags, and the pending-alias check becomes a null check that resets after resolution.
🤖 Generated with Claude Code