Add StringIndex: a generic open-addressed string set - #11660
Conversation
|
🎯 Code Coverage (details) 🔗 Commit SHA: 1541293 | Docs | View more details | Give us feedback! |
🟢 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. |
- Consume the StringIndex surface API now in #11660: FIXED_HANDLER_IDS via Support.mapIntValues; handlerId resolves via Support.lookup (id, or 0 on miss -- the not-intercepted sentinel, so no separate slot check). - Make TagInterceptor final (nothing extends it; aids JIT devirtualization). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 767af3de1e
ℹ️ 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".
bric3
left a comment
There was a problem hiding this comment.
Note, in general the generated comments, are not that readable. So I'd rather have human crafted comments.
E.g this way of aligning words is difficult to process, one sentence would be ok, but since all comments are like that :
* <p>Slot 0-value is the empty sentinel: {@link Support#hash} never returns 0, so {@code hashes[i]
* == 0} unambiguously means an empty slot.
StringIndex: a generic open-addressed string set
There was a problem hiding this comment.
More details
All 166 adversarial scenarios executed against the new StringIndex implementation pass cleanly — empty-string key (remapped hash 0xDD06), empty index, duplicate deduplication, full wraparound probe chains crossing different-hash occupants, parallel long[]/int[]/T[] payloads with realistic tracer tag sets, non-interned string lookups, and typed-array return from mapValues. The open-addressing algorithm (zero sentinel, linear probe, LF ≤ 0.5) is internally consistent and handles every edge case the diff introduces.
📊 Validated against 166 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 1d8fd80 · What is Autotest? · Any feedback? Reach out in #autotest
3b9156e to
d2aa48f
Compare
|
coverage is violated: |
@bric3 Yes, I was working on plugging that gap yesterday. I added a few convenience methods at the end and that dropped the coverage level. I'll fix it. |
d2aa48f to
1f257aa
Compare
There was a problem hiding this comment.
Large capacity requests can return Integer.MIN_VALUE. This result violates the helper contract and can cause NegativeArraySizeException during allocation.
🤖 Datadog Autotest · Commit 7c4f326 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
bric3
left a comment
There was a problem hiding this comment.
Pre-approving, code looks good, but there's some light tweak around javadoc, and method overloads that take hashes or not.
…actor ceil(n / loadFactor) was narrowed to int before checking bounds, so a sufficiently large n or small loadFactor silently saturated the narrowing conversion and the subsequent << 1 wrapped to a negative capacity, producing NegativeArraySizeException downstream instead of a clear error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
More details
The new index keeps the required open-addressing rules. The static review found no concrete failure in its set, index, mapping, or lookup behavior.
🤖 Datadog Autotest · Commit c20ddd7 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
put is the only array-mutating entry point in EmbeddingSupport; keeping it internal to the package (used only by create) enforces that callers treat the arrays a StringIndex/EmbeddingSupport hands them as build-once/immutable, and removes the raw-hash misuse surface a public put would otherwise expose to external callers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Aligns naming with EmbeddingSupport (renamed from Support for consistency with the in-flight LightMap class) across benchmark methods, javadoc result tables, and prose. No benchmarks re-run -- existing numbers are relabeled. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The stringIndex_embedded rename left columns ragged; realign to the new longest label. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for deea3f2:
What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
JOL's GraphLayout relies on HotSpot-specific Unsafe internals and throws IllegalStateException on IBM/Semeru JVMs, which are part of the CI test matrix. Mirrors the guard already used in ScopeAndContinuationLayoutTest.
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What Does This Do
Adds
StringIndexan open-indexed immutable hash set -- that can also work in conjunction with a separate data array as an immutable map.In terms of ops/sec,
StringIndexperforms on par or better thanHashSetandHashMapwhile consuming less memory.In terms of ops/sec
StringIndexout performsSet.ofbut consumes more memory.StringIndexexcels in situations where map values are primitive types or multiple map values are needed for the same keys.Motivation
Data structure that can be reused throughout dd-trace-java that helps reduce tracer overhead - throughput impact and memory footprint
Additional Notes
StringIndex(datadog.trace.util, alongside the customHashtable) — a flat, allocation-free, open-addressed string set / index:EmbeddingSupport— the static algorithm over rawint[] hashes/String[] names. Held instatic finalfields the refs fold to constants (the hot path).Data— a build-time carrier{int[] hashes, String[] names}(pull into your own fields).of/contains/indexOf).2×-oversized (load factor ≤ 0.5), linear probe + wraparound, hash gates
equals, interned==fast path,0= empty sentinel. Generic — no payload baked in; it just knows names. The headline capability isindexOf, which assigns each known string a stable dense slot; consumers attach a parallel array (e.g.long[] ids) indexed by that slot. Membership (contains) falls out asindexOf >= 0.(Renamed from
TagSet— the structure is more general than tags; a fixed name→id / membership index is just one of its uses.)The static tier is named
EmbeddingSupportand the sizing helpercapacityFor, matching the rest of the flat-collections family (LightMap,Hashtable) so the "static functions over a caller-owned array/spine" tier is named identically across all of them.Benchmarks
The motivating use is a fast, declared name→id table — a "pit of success" alternative to hand-rolled string
switches and per-name caches — but the structure is general. Benchmarks (Apple M1, JDK 17,@Fork(5),@Threads(8); M ops/s):ImmutableSetBenchmark(membership, hit): staticEmbeddingSupport2320 ≳HashSet2198 > instanceStringIndex2098 >Set.copyOf/SetN1914, and ~2.5–3.5× thearray/sortedArray/treeSetforms. The foldedEmbeddingSupportpath is the fastest membership structure (~6% overHashSet); the instance wrapper costs ~10% (a field load), landing nearHashSet.ImmutableMapBenchmark(name→valueget): staticEmbeddingSupport1498 (interned key 2081) > instance 1363 >HashMap1216 >Map.copyOf/MapN1049 — StringIndex-as-map is the fastestget, and a parallellong[]/int[]avoids the boxing aHashMap<String, Long>pays.StringIndexSwitchBenchmark(name→id vs a hand-written stringswitch): on a runtime, varied key — the realistic regime — StringIndex is ~1.85× the switch (2147 vs 1161) and flat across inline/key-shape; the switch only matches it when the key is a compile-time constant (the JIT const-folds the switch away), which production keys never are.Footprint (
StringIndexFootprintTest, JOL): ~9% lighter thanHashSet(no per-elementNodes) but ~27% heavier thanSet.copyOf/SetN(it carries the cachedint[]hashes + a 2×-oversized table) — so vs the JDK compact immutables the edge is speed + theindexOf→parallel-array capability, not footprint.StringIndexTestcovers hashing/zero-sentinel, probe + wraparound, table-full, and the parallel-payload usage.🤖 Generated with Claude Code