From fef452c98384eee668ce59ed58e3a6e11e8fc5cc Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Mon, 29 Jun 2026 15:33:37 -0400 Subject: [PATCH 1/2] Add TagMap read-through read mechanism (single parent) Adds optional read-through to OptimizedTagMap: a child map references a frozen parent (withParent) and reads through to it on a local miss, while local entries shadow the parent. The enabler for level-split phase 1 (a span no longer copies the shared trace-level tags down per span). Single-parent by design in phase 1 (anti-false-generalization); written so generalizing to multiple flattened parents is additive. Inert when parent == null (every existing map), so no behavior change off the read-through path. - Read path: getEntry falls through to the parent on a local miss (get*/containsKey inherit it). isDefinitelyEmpty()/estimateSize() added as cheap conservative/upper- bound variants; isEmpty()/size() stay exact (Map contract) and resolve the union. - Removal: a lazily-allocated removedFromParent side-set (also the gate) tombstones a parent-exposed key so it no longer reads through; re-setting clears it. Entry and BucketGroup stay untouched (the side-set is shape-agnostic vs the bare/group duality). - Bulk reads (forEach x3, iterators, collection views): bucket-aligned self-vs-parent merge, first-occurrence-wins, exploiting universal hashing so the shadow check is scoped to the same-index local bucket (no re-hash, no global seen-set). Alloc-free. - checkIntegrity asserts the local emptiness invariant (size==0), not union isEmpty(). Co-Authored-By: Claude Opus 4.8 --- .../main/java/datadog/trace/api/TagMap.java | 324 ++++++++++++++++-- .../trace/api/TagMapReadThroughTest.java | 261 ++++++++++++++ 2 files changed, 558 insertions(+), 27 deletions(-) create mode 100644 internal-api/src/test/java/datadog/trace/api/TagMapReadThroughTest.java diff --git a/internal-api/src/main/java/datadog/trace/api/TagMap.java b/internal-api/src/main/java/datadog/trace/api/TagMap.java index f8f33f1c023..1c834e4449c 100644 --- a/internal-api/src/main/java/datadog/trace/api/TagMap.java +++ b/internal-api/src/main/java/datadog/trace/api/TagMap.java @@ -6,6 +6,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.HashSet; import java.util.Iterator; import java.util.Map; import java.util.NoSuchElementException; @@ -278,6 +279,21 @@ void forEach( /** Checks if the TagMap is writable - if not throws {@link IllegalStateException} */ void checkWriteAccess(); + /** + * Cheap, conservative emptiness check: {@code true} guarantees the map is empty; {@code false} + * means it may be non-empty. Unlike {@link #isEmpty()} (exact, {@link java.util.Map} contract) + * this never resolves read-through parents to an exact union, so it is safe on the hot path. + * Prefer it to {@link #isEmpty()} wherever a conservative answer suffices. + */ + boolean isDefinitelyEmpty(); + + /** + * Cheap upper-bound estimate of the map size ({@code >=} the exact {@link #size()}). Does not + * account for read-through shadowing or removals, so it may over-count; intended for capacity + * hints. Prefer it to {@link #size()} wherever an upper bound suffices. + */ + int estimateSize(); + abstract class EntryChange { public static final EntryRemoval newRemoval(String tag) { return new EntryRemoval(tag); @@ -1191,6 +1207,25 @@ static final class EmptyHolder { private int size; private boolean frozen; + /** + * Optional frozen parent for read-through (level-split phase 1). When non-null, reads that miss + * the local buckets fall through to the parent; a local entry shadows the parent's (local-wins). + * Phase 1 is single-parent by design (anti-false-generalization); generalizing to multiple + * flattened parents is additive. Must be frozen when attached, so it is safely shareable. + */ + private OptimizedTagMap parent; + + /** + * Parent keys removed locally (read-through tombstones). Lazily allocated on the first such + * removal; {@code null} both means "no tombstones" and serves as the gate that keeps the hot + * paths untouched. Only meaningful when {@link #parent} != null. A tombstone stops read-through + * fall-through for its key, so a key removed from a child no longer reads through to the parent. + * Kept off the bucket structure deliberately — it is shape-agnostic (bare-Entry vs BucketGroup) + * and rare, so it costs a lazy allocation on removal rather than complicating the hot bucket + * code. + */ + private Set removedFromParent; + public OptimizedTagMap() { // needs to be a power of 2 for bucket masking calculation to work as intended this.buckets = new Object[1 << 4]; @@ -1212,12 +1247,62 @@ public boolean isOptimized() { @Override public int size() { - return this.size; + // Exact (Map contract). Under read-through resolves the union; prefer estimateSize() for hints. + OptimizedTagMap p = this.parent; + return p == null ? this.size : this.size + this.visibleParentCount(); + } + + /** + * Exact count of parent entries not shadowed locally or tombstoned (the read-through addition). + */ + private int visibleParentCount() { + Object[] parentBuckets = this.parent.buckets; + Object[] thisBuckets = this.buckets; + int count = 0; + for (int i = 0; i < parentBuckets.length; ++i) { + Object parentBucket = parentBuckets[i]; + Object localBucket = thisBuckets[i]; + if (parentBucket instanceof Entry) { + if (parentEntryVisibleInBucket(localBucket, (Entry) parentBucket)) count++; + } else if (parentBucket instanceof BucketGroup) { + for (BucketGroup g = (BucketGroup) parentBucket; g != null; g = g.prev) { + for (int j = 0; j < BucketGroup.LEN; ++j) { + Entry pe = g._entryAt(j); + if (pe != null && parentEntryVisibleInBucket(localBucket, pe)) count++; + } + } + } + } + return count; } @Override public boolean isEmpty() { - return (this.size == 0); + // Exact (Map contract). Under read-through resolves the parent; prefer isDefinitelyEmpty(). + if (this.size != 0) { + return false; + } + OptimizedTagMap p = this.parent; + if (p == null) { + return true; + } + if (this.removedFromParent == null) { + // no local entries and no tombstones -> empty iff the parent is empty (nothing shadows it) + return p.isEmpty(); + } + // size == 0 with tombstones (rare): empty iff every parent entry is tombstoned + return this.visibleParentCount() == 0; + } + + @Override + public boolean isDefinitelyEmpty() { + return this.size == 0 && (this.parent == null || this.parent.isDefinitelyEmpty()); + } + + @Override + public int estimateSize() { + // Upper bound: local + parent, ignoring read-through shadowing/removals (over-counts). + return this.parent == null ? this.size : this.size + this.parent.estimateSize(); } @Deprecated @@ -1328,26 +1413,57 @@ public Set> entrySet() { @Override public Entry getEntry(String tag) { - Object[] thisBuckets = this.buckets; + Entry local = this.getLocalEntry(tag); + if (local != null) { + // Local entry shadows the parent (local-wins) — unchanged hot path. + return local; + } + // Read-through: miss locally, defer to the frozen parent. Single-parent in phase 1. + // The tombstone check lives only here, on the cold miss+parent path — the hot local hit above + // never touches it. + OptimizedTagMap p = this.parent; + if (p == null) { + return null; + } + if (this.removedFromParent != null && this.removedFromParent.contains(tag)) { + return null; // tombstoned: removed locally, do not read through + } + return p.getEntry(tag); + } + /** Looks up an entry in this map's own buckets only — no read-through to the parent. */ + private Entry getLocalEntry(String tag) { + Object[] thisBuckets = this.buckets; int hash = TagMap.Entry._hash(tag); - int bucketIndex = hash & (thisBuckets.length - 1); + return findInBucket(thisBuckets[hash & (thisBuckets.length - 1)], hash, tag); + } - Object bucket = thisBuckets[bucketIndex]; - if (bucket == null) { - return null; - } else if (bucket instanceof Entry) { + /** + * Finds an entry by hash/tag within a single bucket object (Entry | BucketGroup chain | null). + */ + private static Entry findInBucket(Object bucket, int hash, String tag) { + if (bucket instanceof Entry) { Entry tagEntry = (Entry) bucket; - if (tagEntry.matches(tag)) return tagEntry; + return tagEntry.matches(tag) ? tagEntry : null; } else if (bucket instanceof BucketGroup) { - BucketGroup lastGroup = (BucketGroup) bucket; - - Entry tagEntry = lastGroup.findInChain(hash, tag); - return tagEntry; + return ((BucketGroup) bucket).findInChain(hash, tag); } return null; } + /** + * Whether a parent entry is visible through this child at its (shared) bucket: not tombstoned and + * not shadowed by a local entry. Exploits universal hashing — by {@code _hash}, the only local + * entry that could shadow {@code pe} lives in this map's same-index bucket, so we compare against + * {@code localBucket} alone, reusing {@code pe}'s cached hash (no re-hash, no full-map probe). + */ + private boolean parentEntryVisibleInBucket(Object localBucket, Entry pe) { + if (this.removedFromParent != null && this.removedFromParent.contains(pe.tag)) { + return false; // tombstoned: removed locally + } + return findInBucket(localBucket, pe.hash(), pe.tag) == null; // not shadowed by a local entry + } + @Deprecated @Override public Object put(String tag, Object value) { @@ -1399,6 +1515,12 @@ public void set(String tag, double value) { public Entry getAndSet(Entry newEntry) { this.checkWriteAccess(); + // Re-setting a key clears any read-through tombstone for it (the new value overrides the + // removal). Gated on the lazy field, so this is a no-op for the common no-tombstone case. + if (this.removedFromParent != null) { + this.removedFromParent.remove(newEntry.tag); + } + Object[] thisBuckets = this.buckets; int newHash = newEntry.hash(); @@ -1714,6 +1836,32 @@ public boolean remove(String tag) { public Entry getAndRemove(String tag) { this.checkWriteAccess(); + Entry localRemoved = this.removeLocal(tag); + + OptimizedTagMap p = this.parent; + if (p != null) { + // Read-through: if the parent still exposes this key, removing it must also hide it from + // fall-through — install a tombstone. The prior *visible* value (Map.remove contract) is the + // local entry if there was one, otherwise the parent's (which we now hide). Single-parent in + // phase 1; rare path (only when removing a parent-exposed key). + boolean alreadyTombstoned = + this.removedFromParent != null && this.removedFromParent.contains(tag); + if (!alreadyTombstoned) { + Entry parentEntry = p.getEntry(tag); + if (parentEntry != null) { + if (this.removedFromParent == null) { + this.removedFromParent = new HashSet<>(); + } + this.removedFromParent.add(tag); + return localRemoved != null ? localRemoved : parentEntry; + } + } + } + return localRemoved; + } + + /** Removes an entry from this map's own buckets only — no parent/tombstone handling. */ + private Entry removeLocal(String tag) { Object[] thisBuckets = this.buckets; int hash = TagMap.Entry._hash(tag); @@ -1765,6 +1913,23 @@ public TagMap immutableCopy() { } } + /** + * Attaches a frozen parent for read-through (level-split phase 1): reads that miss this map's + * buckets fall through to {@code parent}, while local entries shadow it. The parent must be + * frozen so it is safely shareable across spans/threads without synchronization. + * + *

Package-private — the public, {@code !needsIntercept}-gated wiring lands with the consumer + * change. Single-parent by design in phase 1; generalizing to multiple flattened parents (or a + * {@code withParents(...)} overload) is additive. + */ + OptimizedTagMap withParent(OptimizedTagMap parent) { + if (parent != null && !parent.frozen) { + throw new IllegalStateException("read-through parent must be frozen"); + } + this.parent = parent; + return this; + } + @Override public Iterator iterator() { return new EntryReaderIterator(this); @@ -1792,6 +1957,28 @@ public void forEach(Consumer consumer) { thisGroup.forEachInChain(consumer); } } + + // read-through: emit parent entries not shadowed locally or tombstoned (bucket-aligned merge) + OptimizedTagMap p = this.parent; + if (p != null) { + Object[] parentBuckets = + p.buckets; // leaf parent in phase 1: same length, same bucket per key + for (int i = 0; i < parentBuckets.length; ++i) { + Object parentBucket = parentBuckets[i]; + Object localBucket = thisBuckets[i]; + if (parentBucket instanceof Entry) { + Entry pe = (Entry) parentBucket; + if (parentEntryVisibleInBucket(localBucket, pe)) consumer.accept(pe); + } else if (parentBucket instanceof BucketGroup) { + for (BucketGroup g = (BucketGroup) parentBucket; g != null; g = g.prev) { + for (int j = 0; j < BucketGroup.LEN; ++j) { + Entry pe = g._entryAt(j); + if (pe != null && parentEntryVisibleInBucket(localBucket, pe)) consumer.accept(pe); + } + } + } + } + } } @Override @@ -1811,6 +1998,30 @@ public void forEach(T thisObj, BiConsumer con thisGroup.forEachInChain(thisObj, consumer); } } + + // read-through: emit parent entries not shadowed locally or tombstoned (bucket-aligned merge) + OptimizedTagMap p = this.parent; + if (p != null) { + Object[] parentBuckets = + p.buckets; // leaf parent in phase 1: same length, same bucket per key + for (int i = 0; i < parentBuckets.length; ++i) { + Object parentBucket = parentBuckets[i]; + Object localBucket = thisBuckets[i]; + if (parentBucket instanceof Entry) { + Entry pe = (Entry) parentBucket; + if (parentEntryVisibleInBucket(localBucket, pe)) consumer.accept(thisObj, pe); + } else if (parentBucket instanceof BucketGroup) { + for (BucketGroup g = (BucketGroup) parentBucket; g != null; g = g.prev) { + for (int j = 0; j < BucketGroup.LEN; ++j) { + Entry pe = g._entryAt(j); + if (pe != null && parentEntryVisibleInBucket(localBucket, pe)) { + consumer.accept(thisObj, pe); + } + } + } + } + } + } } @Override @@ -1831,6 +2042,30 @@ public void forEach( thisGroup.forEachInChain(thisObj, otherObj, consumer); } } + + // read-through: emit parent entries not shadowed locally or tombstoned (bucket-aligned merge) + OptimizedTagMap p = this.parent; + if (p != null) { + Object[] parentBuckets = + p.buckets; // leaf parent in phase 1: same length, same bucket per key + for (int i = 0; i < parentBuckets.length; ++i) { + Object parentBucket = parentBuckets[i]; + Object localBucket = thisBuckets[i]; + if (parentBucket instanceof Entry) { + Entry pe = (Entry) parentBucket; + if (parentEntryVisibleInBucket(localBucket, pe)) consumer.accept(thisObj, otherObj, pe); + } else if (parentBucket instanceof BucketGroup) { + for (BucketGroup g = (BucketGroup) parentBucket; g != null; g = g.prev) { + for (int j = 0; j < BucketGroup.LEN; ++j) { + Entry pe = g._entryAt(j); + if (pe != null && parentEntryVisibleInBucket(localBucket, pe)) { + consumer.accept(thisObj, otherObj, pe); + } + } + } + } + } + } } public void clear() { @@ -1895,7 +2130,10 @@ void checkIntegrity() { if (this.size != this.computeSize()) { throw new IllegalStateException("incorrect size"); } - if (this.isEmpty() != this.checkIfEmpty()) { + // Local-structure invariant: the size counter's emptiness must match the local buckets. Uses + // the + // local (this.size == 0), NOT isEmpty(), which under read-through resolves the parent too. + if ((this.size == 0) != this.checkIfEmpty()) { throw new IllegalStateException("incorrect empty status"); } } @@ -2013,7 +2251,12 @@ String toInternalString() { } abstract static class IteratorBase { - private final Object[] buckets; + private final OptimizedTagMap map; + private final Object[] localBuckets; + + // current array being walked: local buckets first, then the parent's (read-through union) + private Object[] buckets; + private boolean inParent = false; private Entry nextEntry; @@ -2023,18 +2266,16 @@ abstract static class IteratorBase { private int groupIndex = 0; IteratorBase(OptimizedTagMap map) { + this.map = map; + this.localBuckets = map.buckets; this.buckets = map.buckets; } public final boolean hasNext() { if (this.nextEntry != null) return true; - while (this.bucketIndex < this.buckets.length) { - this.nextEntry = this.advance(); - if (this.nextEntry != null) return true; - } - - return false; + this.nextEntry = this.advance(); + return this.nextEntry != null; } final Entry nextEntryOrThrowNoSuchElement() { @@ -2062,6 +2303,35 @@ final Entry nextEntryOrNull() { } private final Entry advance() { + while (true) { + Entry tagEntry = this.rawAdvance(); + if (tagEntry != null) { + // local entries emit as-is; parent entries only if not shadowed locally or tombstoned. + // bucketIndex indexes the parent buckets here, which (universal hashing) line up with the + // same-index local bucket — so localBuckets[bucketIndex] is the bucket that could shadow. + if (!this.inParent + || this.map.parentEntryVisibleInBucket( + this.localBuckets[this.bucketIndex], tagEntry)) { + return tagEntry; + } + continue; // parent entry shadowed/tombstoned -> skip + } + + // current array exhausted; switch to the parent's buckets once (read-through union) + if (!this.inParent && this.map.parent != null) { + this.inParent = true; + this.buckets = this.map.parent.buckets; + this.bucketIndex = -1; + this.group = null; + this.groupIndex = 0; + continue; + } + return null; + } + } + + /** Next raw entry in the current bucket array, ignoring shadowing/tombstones. */ + private final Entry rawAdvance() { while (this.bucketIndex < this.buckets.length) { if (this.group != null) { for (++this.groupIndex; this.groupIndex < BucketGroup.LEN; ++this.groupIndex) { @@ -2579,12 +2849,12 @@ static final class Entries extends AbstractSet> { @Override public int size() { - return this.map.computeSize(); + return this.map.size(); } @Override public boolean isEmpty() { - return this.map.checkIfEmpty(); + return this.map.isEmpty(); } @Override @@ -2604,12 +2874,12 @@ static final class Keys extends AbstractSet { @Override public int size() { - return this.map.computeSize(); + return this.map.size(); } @Override public boolean isEmpty() { - return this.map.checkIfEmpty(); + return this.map.isEmpty(); } @Override @@ -2643,12 +2913,12 @@ static final class Values extends AbstractCollection { @Override public int size() { - return this.map.computeSize(); + return this.map.size(); } @Override public boolean isEmpty() { - return this.map.checkIfEmpty(); + return this.map.isEmpty(); } @Override diff --git a/internal-api/src/test/java/datadog/trace/api/TagMapReadThroughTest.java b/internal-api/src/test/java/datadog/trace/api/TagMapReadThroughTest.java new file mode 100644 index 00000000000..0bbc5ca1fcf --- /dev/null +++ b/internal-api/src/test/java/datadog/trace/api/TagMapReadThroughTest.java @@ -0,0 +1,261 @@ +package datadog.trace.api; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.HashMap; +import java.util.Iterator; +import java.util.Map; +import java.util.Set; +import org.junit.jupiter.api.Test; + +/** + * Read-through support, slice 1 (read path): a child {@link OptimizedTagMap} with a frozen parent + * reads through to the parent on a local miss, while local entries shadow the parent (local-wins). + * Removal/tombstones and bulk (iteration/serialize) union come in later slices. + */ +class TagMapReadThroughTest { + + private static OptimizedTagMap frozenParent() { + OptimizedTagMap parent = (OptimizedTagMap) TagMap.create(); + parent.set("a", "parent-a"); + parent.set("b", "parent-b"); + parent.freeze(); + return parent; + } + + @Test + void readsThroughToParentOnMiss() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.set("c", "child-c"); + child.withParent(frozenParent()); + + assertEquals("parent-a", child.getString("a")); // miss locally -> read through + assertEquals("parent-b", child.getString("b")); + assertEquals("child-c", child.getString("c")); // local + assertNull(child.getString("missing")); + assertTrue(child.containsKey("a")); + assertFalse(child.containsKey("missing")); + } + + @Test + void localEntryShadowsParent() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.set("b", "child-b"); // same key as parent + child.withParent(frozenParent()); + + assertEquals("child-b", child.getString("b")); // local wins + assertEquals("parent-a", child.getString("a")); // parent still visible + } + + @Test + void estimateSizeIsUpperBound() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.set("b", "child-b"); // shadows parent "b" + child.set("c", "child-c"); + child.withParent(frozenParent()); + + // true union = {a, b, c} = 3; estimate over-counts the shadowed "b": local 2 + parent 2 = 4 + assertEquals(4, child.estimateSize()); + assertTrue(child.estimateSize() >= 3, "estimateSize must be an upper bound on the true size"); + } + + @Test + void emptinessSemantics() { + OptimizedTagMap emptyOverEmpty = (OptimizedTagMap) TagMap.create(); + emptyOverEmpty.withParent((OptimizedTagMap) TagMap.create().freeze()); + assertTrue(emptyOverEmpty.isEmpty()); + assertTrue(emptyOverEmpty.isDefinitelyEmpty()); + + OptimizedTagMap emptyOverNonEmpty = (OptimizedTagMap) TagMap.create(); + emptyOverNonEmpty.withParent(frozenParent()); + assertFalse(emptyOverNonEmpty.isEmpty(), "a non-empty parent makes the map non-empty"); + assertFalse(emptyOverNonEmpty.isDefinitelyEmpty()); + + assertTrue(((OptimizedTagMap) TagMap.create()).isDefinitelyEmpty()); + } + + @Test + void parentMustBeFrozen() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + OptimizedTagMap mutableParent = (OptimizedTagMap) TagMap.create(); + assertThrows(IllegalStateException.class, () -> child.withParent(mutableParent)); + } + + // --- slice 2: removal / tombstones --- + + @Test + void removingParentKeyHidesItFromChildButNotFromParent() { + OptimizedTagMap parent = frozenParent(); + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.withParent(parent); + + assertEquals("parent-a", child.getString("a")); // visible before removal + child.remove("a"); + + assertNull(child.getString("a")); // tombstoned: no longer reads through + assertFalse(child.containsKey("a")); + assertEquals("parent-b", child.getString("b")); // other parent keys unaffected + assertEquals("parent-a", parent.getString("a")); // frozen parent untouched + } + + @Test + void removeReturnsPriorVisibleValueViaParent() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.withParent(frozenParent()); + + // Map.remove contract: the key was present (via read-through), so removal reports it. + assertTrue(child.remove("a"), "removing a parent-exposed key should report it was present"); + assertNull(child.getString("a")); + } + + @Test + void reSettingARemovedKeyRestoresVisibility() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.withParent(frozenParent()); + + child.remove("a"); + assertNull(child.getString("a")); + + child.set("a", "child-a"); // re-set clears the tombstone + assertEquals("child-a", child.getString("a")); + } + + @Test + void removingAKeyThatIsBothLocalAndParentHidesBoth() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.set("b", "child-b"); // shadows parent "b" + child.withParent(frozenParent()); + + assertEquals("child-b", child.getString("b")); + child.remove("b"); + + assertNull(child.getString("b"), "removal must hide both the local entry and the parent's"); + assertEquals("parent-b", frozenParent().getString("b")); // parent still has it + } + + // --- slice 3a: bulk forEach union + exact size/isEmpty --- + + private static Map collect(OptimizedTagMap map) { + Map out = new HashMap<>(); + map.forEach(e -> out.put(e.tag(), e.objectValue())); + return out; + } + + @Test + void forEachEmitsDedupedUnionLocalWins() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.set("b", "child-b"); // shadows parent "b" + child.set("c", "child-c"); + child.withParent(frozenParent()); // parent {a, b} + + Map u = collect(child); + assertEquals(3, u.size(), "union {a, b, c} with b deduped"); + assertEquals("parent-a", u.get("a")); // read-through + assertEquals("child-b", u.get("b")); // local wins (no duplicate emit) + assertEquals("child-c", u.get("c")); + } + + @Test + void forEachSkipsTombstonedParentKeys() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.set("c", "child-c"); + child.withParent(frozenParent()); + child.remove("a"); // tombstone parent's "a" + + Map u = collect(child); + assertEquals(2, u.size()); + assertFalse(u.containsKey("a")); + assertEquals("parent-b", u.get("b")); + assertEquals("child-c", u.get("c")); + } + + @Test + void biConsumerForEachAlsoEmitsUnion() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.set("c", "child-c"); + child.withParent(frozenParent()); + + Map out = new HashMap<>(); + child.forEach(out, (m, e) -> m.put(e.tag(), e.objectValue())); // non-capturing: alloc-free path + assertEquals(3, out.size()); + assertEquals("parent-a", out.get("a")); + assertEquals("child-c", out.get("c")); + } + + @Test + void sizeIsExactUnion() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.set("b", "child-b"); // shadows + child.set("c", "child-c"); + child.withParent(frozenParent()); + assertEquals(3, child.size()); // {a, b, c} — b deduped, not 4 + + child.remove("a"); + assertEquals(2, child.size()); // {b, c} + } + + @Test + void isEmptyExactWhenAllParentKeysTombstonedAndNoLocal() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.withParent(frozenParent()); // parent {a, b} + assertFalse(child.isEmpty()); + + child.remove("a"); + child.remove("b"); + assertTrue(child.isEmpty(), "all parent keys tombstoned and no local entries -> empty"); + assertEquals(0, child.size()); + } + + // --- slice 3b: pull-based iterators / collection views --- + + @Test + void iteratorEmitsDedupedUnion() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.set("b", "child-b"); // shadows parent "b" + child.set("c", "child-c"); + child.withParent(frozenParent()); + + Map u = new HashMap<>(); + Iterator it = child.iterator(); + while (it.hasNext()) { + TagMap.EntryReader e = it.next(); + u.put(e.tag(), e.objectValue()); + } + assertEquals(3, u.size()); + assertEquals("parent-a", u.get("a")); + assertEquals("child-b", u.get("b")); // local wins, emitted once + assertEquals("child-c", u.get("c")); + } + + @Test + void keySetReflectsUnionAndTombstones() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.set("c", "child-c"); + child.withParent(frozenParent()); + + Set keys = child.keySet(); + assertEquals(3, keys.size()); // a, b, c + assertTrue(keys.contains("a")); + assertTrue(keys.contains("c")); + + child.remove("a"); + assertEquals(2, child.keySet().size()); + assertFalse(child.keySet().contains("a")); + } + + @Test + void valuesAndEntrySetReflectUnion() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.set("b", "child-b"); // shadows parent "b" + child.withParent(frozenParent()); + + assertEquals(2, child.entrySet().size()); // {a, b} — b deduped + assertTrue(child.values().contains("child-b")); // local-won value + assertTrue(child.values().contains("parent-a")); + assertFalse(child.values().contains("parent-b"), "shadowed parent value must not appear"); + } +} From 197e952f4be10ef1d095168e8f457b1a1eb99159 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Mon, 29 Jun 2026 15:36:45 -0400 Subject: [PATCH 2/2] Make TagMap.copy() preserve read-through structure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit copy() went through putAllIntoEmptyMap, which clones only the local buckets and the local size — so a copy of a read-through map dropped the inherited parent tags and the tombstones. Fix copy() to share the (frozen, immutable) parent and copy the tombstone set, so the copy is observationally identical to the original (same union) and independently mutable. Adds behavior-identical-to-flat-map tests: copy equivalence, independent mutability, tombstone preservation, equality with an equivalent flat map, and immutableCopy of a read-through map. This is the safety contract for flipping mergedTracerTags to a parent in the consumer change. Co-Authored-By: Claude Opus 4.8 --- .../main/java/datadog/trace/api/TagMap.java | 9 ++- .../trace/api/TagMapReadThroughTest.java | 78 +++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/internal-api/src/main/java/datadog/trace/api/TagMap.java b/internal-api/src/main/java/datadog/trace/api/TagMap.java index 1c834e4449c..78f5170d770 100644 --- a/internal-api/src/main/java/datadog/trace/api/TagMap.java +++ b/internal-api/src/main/java/datadog/trace/api/TagMap.java @@ -1901,7 +1901,14 @@ private Entry removeLocal(String tag) { @Override public TagMap copy() { OptimizedTagMap copy = new OptimizedTagMap(); - copy.putAllIntoEmptyMap(this); + copy.putAllIntoEmptyMap(this); // clones this map's own (local) buckets + size + // Preserve read-through: share the frozen parent (immutable -> safe to share) and copy the + // tombstones, so the copy is observationally identical to this map (same union) and remains + // independently mutable (writes land on the copy's local buckets, never the shared parent). + copy.parent = this.parent; + if (this.removedFromParent != null) { + copy.removedFromParent = new HashSet<>(this.removedFromParent); + } return copy; } diff --git a/internal-api/src/test/java/datadog/trace/api/TagMapReadThroughTest.java b/internal-api/src/test/java/datadog/trace/api/TagMapReadThroughTest.java index 0bbc5ca1fcf..23de03cdc86 100644 --- a/internal-api/src/test/java/datadog/trace/api/TagMapReadThroughTest.java +++ b/internal-api/src/test/java/datadog/trace/api/TagMapReadThroughTest.java @@ -258,4 +258,82 @@ void valuesAndEntrySetReflectUnion() { assertTrue(child.values().contains("parent-a")); assertFalse(child.values().contains("parent-b"), "shadowed parent value must not appear"); } + + // --- slice 4: behavior-identical to a copy-down / flat map --- + + @Test + void copyIsObservationallyIdentical() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.set("b", "child-b"); // shadows parent "b" + child.set("c", "child-c"); + child.withParent(frozenParent()); // {a, b} + + OptimizedTagMap copy = (OptimizedTagMap) child.copy(); + assertEquals(child.size(), copy.size()); + assertEquals("parent-a", copy.getString("a")); // copy still reads through + assertEquals("child-b", copy.getString("b")); + assertEquals("child-c", copy.getString("c")); + assertEquals(collect(child), collect(copy)); // same union + } + + @Test + void copyIsIndependentlyMutable() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.set("c", "child-c"); + child.withParent(frozenParent()); + + OptimizedTagMap copy = (OptimizedTagMap) child.copy(); + copy.set("c", "copy-c"); // mutate copy's local + copy.remove("a"); // tombstone on copy only + + assertEquals("child-c", child.getString("c"), "original unaffected by copy mutation"); + assertEquals("parent-a", child.getString("a"), "original still reads through a"); + assertEquals("copy-c", copy.getString("c")); + assertNull(copy.getString("a")); + } + + @Test + void copyPreservesTombstones() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.withParent(frozenParent()); + child.remove("a"); // tombstone "a" + + OptimizedTagMap copy = (OptimizedTagMap) child.copy(); + assertNull(copy.getString("a"), "tombstone must carry into the copy"); + assertEquals("parent-b", copy.getString("b")); + } + + /** The contract that lets the consumer flip mergedTracerTags to a parent. */ + @Test + void readThroughMatchesAnEquivalentFlatMap() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.set("b", "child-b"); + child.set("c", "child-c"); + child.withParent(frozenParent()); + + OptimizedTagMap flat = (OptimizedTagMap) TagMap.create(); + flat.set("a", "parent-a"); + flat.set("b", "child-b"); + flat.set("c", "child-c"); + + assertEquals(flat.size(), child.size()); + assertEquals(collect(flat), collect(child)); + assertEquals(flat.keySet(), child.keySet()); + for (String k : new String[] {"a", "b", "c", "missing"}) { + assertEquals(flat.getString(k), child.getString(k), "mismatch for key " + k); + } + } + + @Test + void immutableCopyOfReadThroughIsFrozenAndStillReadsThrough() { + OptimizedTagMap child = (OptimizedTagMap) TagMap.create(); + child.set("c", "child-c"); + child.withParent(frozenParent()); + + OptimizedTagMap frozen = (OptimizedTagMap) child.immutableCopy(); + assertTrue(frozen.isFrozen()); + assertEquals("parent-a", frozen.getString("a")); // union preserved + assertEquals("child-c", frozen.getString("c")); + assertThrows(IllegalStateException.class, () -> frozen.set("x", "y")); // frozen blocks writes + } }