Repository navigation
Conversation
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. |
A pure move: the loops over one bucket array (find, put, remove, copy, merge, the forEach variants, fillMap / fillStringMap, count, isEmpty, the bucket-position check, toInternalString) become static methods of HashSupport, moved verbatim. Each reports what it did through its return value (the entry put replaced, the entry remove removed, how much merge grew the map), and TagMap keeps its fields, its size counter, and the read-through and tombstone handling around them. No new objects or fields, and no behavior change: allocation per trace in TraceShapeBenchmark matches the base within fork-to-fork noise, and the TagMap and core span suites pass unchanged. This prepares the split of known and custom tags into separate stores, which reuse these mechanics. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4c83a5c to
d5a7f64
Compare
…agmap-hash-support
…agmap-hash-support
…agmap-hash-support
…agmap-hash-support
…agmap-hash-support
| * entry it replaced, remove the entry it removed, merge how much the map grew -- and the calling | ||
| * map keeps its own count. Read-through to a parent map and its tombstones stay with the map. | ||
| */ | ||
| final class HashSupport { |
There was a problem hiding this comment.
This is just relocating the guts of TagMap into a static helper class.
This is being done, so that we can transition to a hash store for name and another hash store by ID - which is itself an incremental step towards hash-based store for names only and the new dense store for IDs.
…agmap-hash-support
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
More details
Bucket mutations preserve replacement, removal, and merge-growth reporting to TagMap, maintaining its size accounting. Parent read-through and tombstone handling remain intact.
🤖 Bits Code Review · Commit 76d27dc
parentEntryVisible still masked the hash into a bucket by hand before calling findInBucket, the one place outside HashSupport that knew the bucket array's power-of-2 layout. It now calls HashSupport.find, and findInBucket becomes private. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What Does This Do
A pure move.
TagMap's loops over one bucket array become static methods of a newHashSupportclass, moved verbatim:forEachvariants,fillMapandfillStringMap;isEmpty, the bucket-position check andtoInternalString.Each method reports what it did through its return value: the entry that
putreplaced, the entry thatremoveremoved, and how muchmergegrew the map.TagMapkeeps its fields and its size counter, plus the read-through and tombstone handling around those calls.Motivation
The next PR in the stack splits a span's tags into a known-tag store and a custom-tag store. Both reuse these mechanics, so this moves them out first and reviews the move on its own, separate from the split.
Additional Notes
TagMapand core span suites pass unchanged.Where this is going
This PR is one step in replacing the span's single
TagMapwith storage that knows each tag's kind. The goal is to cut what every span allocates on the application thread, without changing behavior.The end state
TagMapbecomes a thin view for code outside the span.Entryobject per tag, and each span's store is presized from a per-operation hint. With the flag on, a web-request-shaped trace allocates about 30% less on the application thread than on master.TagStore, not aTagMap. That covers post-processors now and serializers later, so noTagMapview gets built for them.TagStorehas its own verbs,loadandstore, apart fromTagMap'sget/setand the span's interceptedgetTag/setTag, soDDSpanContextcan offer both. The tag stores themselves use the same verbs (Add id and name tag stores, not yet used (dense store - part 5) #13006). It starts with what the eager processors use, by name and by id, and grows typed-load and primitive-store variants. Its intercepted counterpart,SpanData(Take the span's tag lock once in setTagsFrom (tag api - part 1) #13030), is whatsetTagsFrom,TagContributorandTagExtractortake, with one lock per batch.Later, not in this stack: cheaper read-through to parent spans, trace-level tags in a per-trace holder (
TraceSegment), and primitive tag values without boxing.The stack. This replaces #12918, split into pieces that can each be reviewed, and measured, on their own.
TagMap. This is the stack's only behavior change, kept separate on purpose.TagMap, and addTraceShapeBenchmarkTagStorefor the eager post-processorsTagMap's bucket mechanics intoHashSupport(a pure move)TagMapa facade over the two storestrace.experimental.dense.tags.enabled(off by default)Every PR after #12999 is behavior-preserving.
The pieces' allocation figures come from a deterministic per-thread probe (
ThreadMXBeanallocated bytes) that builds the same 7-span trace asTraceShapeBenchmark. The #12992 and #12926 figures come fromTraceShapeBenchmarkitself.🤖 Generated with Claude Code