Uh oh!
There was an error while loading. Please reload this page.
Fold OptimizedTagMap into a final class TagMap - #11967
Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 4 commits intoJul 20, 2026
Merged
Conversation
dougqh
marked this pull request as ready for review
July 15, 2026 17:41
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:8d735dec7e
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment.
More details
48 adversarial scenarios validated the four riskiest areas of this fold: EMPTY direct class-init (safe — private constructor reads no statics), putAll(TagMap) with mismatched 1-vs-16 bucket arrays (correct — loop bounds min-clamped, EMPTY always empty), compute* delegation from Map.super vs. old TagMap.super (identical behavior), and freeze/immutableCopy invariants. No regressions found.
📊 Validated against 48 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 8d735de · What is Autotest? · Any feedback? Reach out in #autotest
This comment has been minimized.
This comment has been minimized.
Contributor
🟢 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 added a commit
that referenced
this pull request
Jul 15, 2026
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>
dougqh added a commit
that referenced
this pull request
Jul 15, 2026
…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>
Contributor
There was a problem hiding this comment.
More details
76 synthetic scenarios covering EMPTY initialization, putAll, compute/computeIfAbsent/computeIfPresent, Entry.create with all types, freeze/copy, and ledger operations all pass cleanly. The one non-trivial behavioral change — Entry.create(String, Object) now returning null for empty CharSequence typed as Object — is intentional, documented, and aligns existing callers (e.g. GitInfo) whose comments already stated that empty values should be treated as absent.
📊 Validated against 76 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 5f8632d · What is Autotest? · Any feedback? Reach out in #autotest
dougqh added a commit
that referenced
this pull request
Jul 15, 2026
…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>
dougqh
commented
Jul 16, 2026
Uh oh!
There was an error while loading. Please reload this page.
dougqh
commented
Jul 16, 2026
Uh oh!
There was an error while loading. Please reload this page.
TagMap was an interface with a single implementation, OptimizedTagMap. The split was vestigial scaffolding from when a second (HashMap-backed) impl existed; with one impl it is false generalization. Collapse them into one `public final class TagMap`: - The interface's abstract method declarations are removed; OptimizedTagMap's bodies become TagMap's methods. - Nested types that were implicitly `public static` in the interface (EntryChange, EntryRemoval, EntryReader, Entry, Ledger) are now written out explicitly as `public static`. - Static factories (create/fromMap/ledger/...) and the EMPTY constant become explicit `public static` members; the EmptyHolder lazy-init note is updated now that there is no interface<->impl class-init cycle. - putAll(TagMap) loses its `instanceof` dispatch (always true once there is one class) and calls the fast path directly. No behavior change; motivation is code simplicity, not performance (a single final class is monomorphic by construction, but CHA already devirtualized the sole impl). Public API is preserved, so callers are unchanged; the 3 tests that referenced OptimizedTagMap now reference TagMap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Post-fold tidy, all TagMap-scoped: - Remove EmptyHolder: with one class there is no interface<->impl class-init cycle to break, and the private constructor reads no statics, so EMPTY is a direct `new TagMap(new Object[1], 0)` initializer. - Static factories (create/fromMap/ledger/...) are now `public static final` (not expressible on the old interface). - assertSize/assertNotEmpty/assertEmpty/checkIntegrity test helpers dropped their now-always-true `instanceof TagMap` guard + redundant cast. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The interface -> final-class fold removed the TagMap interface decls, which carried the @Nullable/@nonnull param annotations from #11963. Re-home them onto the now-concrete methods: @nonnull tag keys and strict-setter values, @nullable on the set(EntryReader)/getAndSet(Entry) sinks (+ the getAndSet contract javadoc). The null-tolerance behavior was already preserved by the fold; this restores the self-describing contract on the write surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per review: these factories NPE on a null map (map.size()/putAll), so the input is non-null by contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dougqhforce-pushed
the
dougqh/fold-optimized-tagmap
branch
from
July 16, 2026 16:56
6abe29d to
0737ca6Comparedougqh added a commit
that referenced
this pull request
Jul 20, 2026
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>
dougqh added a commit
that referenced
this pull request
Jul 20, 2026
…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>
Contributor
/merge |
View all feedbacks in Devflow UI.
The expected merge time in
|
github-merge-queueBot
removed this pull request from the merge queue due to failed status checks
Jul 20, 2026
Uh oh!
There was an error while loading. Please reload this page.
gh-worker-dd-mergequeue-cf854dBot
merged commit Jul 20, 2026
4936611
into
master
592 of 594 checks passed
Uh oh!
There was an error while loading. Please reload this page.
gh-worker-dd-mergequeue-cf854dBot
deleted the
dougqh/fold-optimized-tagmap
branch
July 20, 2026 17:41
dougqh added a commit
that referenced
this pull request
Jul 21, 2026
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>
dougqh added a commit
that referenced
this pull request
Jul 21, 2026
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>
dougqh added a commit
that referenced
this pull request
Jul 21, 2026
…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>
dougqh added a commit
that referenced
this pull request
Jul 21, 2026
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>
dougqh added a commit
that referenced
this pull request
Jul 21, 2026
…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>
dougqh added a commit
that referenced
this pull request
Jul 21, 2026
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>
dougqh added a commit
that referenced
this pull request
Jul 21, 2026
…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>
dougqh added a commit
that referenced
this pull request
Jul 21, 2026
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>
dougqh added a commit
that referenced
this pull request
Jul 21, 2026
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>
dougqh added a commit
that referenced
this pull request
Jul 21, 2026
…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>
dougqh added a commit
that referenced
this pull request
Jul 21, 2026
…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>
gh-worker-dd-mergequeue-cf854dBot
pushed a commit
that referenced
this pull request
Aug 17, 2026
…hase 1a) (#11789) Add TagMap read-through support (level-split phase 1 mechanism) 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> Mark parent field @VisibleForTesting and size the tombstone HashSet small Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Collapse redundant isDefinitelyEmpty() call in isEmpty() A live read-through parent is never definitely-empty: createFromParent drops an empty parent, copy() only forwards an already-vetted parent, and the parent is frozen so it can't become empty. So the size==0 / no-tombstones branch of isEmpty() always returned false via the parent chain walk — collapse it to a documented `return false`. Addresses mcculls' review comment on #11789. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Merge branch 'master' into dougqh/tagmap-read-through Merge branch 'master' into dougqh/tagmap-read-through Merge branch 'master' into dougqh/tagmap-read-through Merge branch 'master' into dougqh/tagmap-read-through Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
gh-worker-dd-mergequeue-cf854dBot
pushed a commit
that referenced
this pull request
Aug 17, 2026
…plit) (phase 1a) (#11932) Add TagMap read-through support (level-split phase 1 mechanism) 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> Wire mergedTracerTags as a read-through parent at span build (level-split 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> Mark parent field @VisibleForTesting and size the tombstone HashSet small Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Collapse redundant isDefinitelyEmpty() call in isEmpty() A live read-through parent is never definitely-empty: createFromParent drops an empty parent, copy() only forwards an already-vetted parent, and the parent is frozen so it can't become empty. So the size==0 / no-tombstones branch of isEmpty() always returned false via the parent chain walk — collapse it to a documented `return false`. Addresses mcculls' review comment on #11789. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Merge branch 'dougqh/tagmap-read-through' into dougqh/tagmap-read-through-consumer Merge branch 'master' into dougqh/tagmap-read-through Merge branch 'master' into dougqh/tagmap-read-through Merge branch 'dougqh/tagmap-read-through' into dougqh/tagmap-read-through-consumer Merge branch 'master' into dougqh/tagmap-read-through Merge branch 'dougqh/tagmap-read-through' into dougqh/tagmap-read-through-consumer Make TagMap fillMap/fillStringMap read-through aware Both bulk-fill helpers walked only this.buckets, so a parent-backed TagMap materialized through them dropped tags visible only via the parent chain and ignored shadowing/tombstones -- unlike putAll, forEach, and iteration, which already honor the read-through union. Branch to the visible-union walk (forEach) when a parent is attached, mirroring putAllOptimizedMap; keep the untouched local-only loop for the common no-parent case so it pays zero overhead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Drop observationally-empty read-through parents (exact isEmpty) createFromParent used isDefinitelyEmpty(), which only checks each level's local size and ignores shadowing/tombstones. With multi-level read-through, a frozen intermediate can be observationally empty (no local entries, every inherited key tombstoned) yet report isDefinitelyEmpty() == false because a farther ancestor still holds entries. Attaching such a parent then let a child take isEmpty()'s no-tombstone fast path and return false while size(), lookup, and iteration all report empty -- a Map-contract violation. Use exact isEmpty() so semantically empty parents are dropped, which also restores the invariant isEmpty()'s fast path relies on: an attached parent always contributes at least one visible entry. The hot path is unaffected -- isEmpty() short-circuits on size != 0, only walking tombstones in the rare all-tombstoned case this fix targets. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Merge remote-tracking branch 'origin/master' into dougqh/tagmap-read-through-consumer # Conflicts: # internal-api/src/main/java/datadog/trace/api/TagMap.java # internal-api/src/test/java/datadog/trace/api/TagMapReadThroughTest.java Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This Do
TagMapwas an interface with a single implementation,OptimizedTagMap. The split was vestigial scaffolding from when a second (HashMap-backed) implementation existed. This folds the impl into onepublic final class TagMapand drops the interface.OptimizedTagMap's bodies becomeTagMap's methods.public staticin the interface (EntryChange,EntryRemoval,EntryReader,Entry,Ledger) are now written out explicitly aspublic static.create/fromMap/ledger/…) andEMPTYbecome explicitpublic staticmembers; theEmptyHolderlazy-init note is updated now that there is no interface↔impl class-init cycle.putAll(TagMap)loses itsinstanceofdispatch (always true with one class) and calls the fast path directly.Motivation
Code simplicity — not performance. A single
finalclass is monomorphic by construction, but CHA already devirtualized the sole impl, so no steady-state change is expected. Public API is preserved, so callers are unchanged.Additional Notes
Stacked on #11963.
From Claude: folded per the long-standing "simplify TagMap" plan; verified public-API-preserving and monomorphism-neutral.
🤖 Generated with Claude Code