Skip to content

Add StringIndex: a generic open-addressed string set#11660

Open
dougqh wants to merge 2 commits into
dougqh/tagmap-read-through-consumerfrom
dougqh/tagset
Open

Add StringIndex: a generic open-addressed string set#11660
dougqh wants to merge 2 commits into
dougqh/tagmap-read-through-consumerfrom
dougqh/tagset

Conversation

@dougqh

@dougqh dougqh commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Draft — /techdebt + review before ready. Split out of a larger tag-id effort so the generic data structure stands on its own.

What Does This Do

Adds StringIndex an 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, StringIndex performs on par or better than HashSet and HashMap while consuming less memory.
In terms of ops/sec StringIndex out performs Set.of but consumes more memory.

StringIndex excels 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 custom Hashtable) — a flat, allocation-free, open-addressed string set / index:

  • Support — the static algorithm over raw int[] hashes / String[] names. Held in static final fields the refs fold to constants (the hot path).
  • Data — a build-time carrier {int[] hashes, String[] names} (pull into your own fields).
  • an instance wrapper for convenience (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 is indexOf, 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 as indexOf >= 0.

(Renamed from TagSet — the structure is more general than tags; a fixed name→id / membership index is just one of its uses.)

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): static Support 2320HashSet 2198 > instance StringIndex 2098 > Set.copyOf/SetN 1914, and ~2.5–3.5× the array/sortedArray/treeSet forms. The folded Support path is the fastest membership structure (~6% over HashSet); the instance wrapper costs ~10% (a field load), landing near HashSet.
  • ImmutableMapBenchmark (name→value get): static Support 1498 (interned key 2081) > instance 1363 > HashMap 1216 > Map.copyOf/MapN 1049 — StringIndex-as-map is the fastest get, and a parallel long[]/int[] avoids the boxing a HashMap<String, Long> pays.
  • StringIndexSwitchBenchmark (name→id vs a hand-written string switch): 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 than HashSet (no per-element Nodes) but ~27% heavier than Set.copyOf/SetN (it carries the cached int[] hashes + a 2×-oversized table) — so vs the JDK compact immutables the edge is speed + the indexOf→parallel-array capability, not footprint.

StringIndexTest covers hashing/zero-sentinel, probe + wraparound, table-full, and the parallel-payload usage.

🤖 Generated with Claude Code

@dougqh dougqh added comp: core Tracer core tag: ai generated Largely based on code generated by an AI or LLM tag: no release notes Changes to exclude from release notes type: feature Enhancements and improvements labels Jun 17, 2026
@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

🎯 Code Coverage (details)
Patch Coverage: 95.29%
Overall Coverage: 57.24% (+0.50%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: b9d0922 | Docs | Datadog PR Page | Give us feedback!

@dougqh dougqh changed the title Add TagSet: a generic open-addressed string set Add StringIndex: a generic open-addressed string set Jun 23, 2026
@dd-octo-sts

dd-octo-sts Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite Status
Startup 🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results
Scenario Candidate master Δ (95% CI of mean)
startup:insecure-bank:iast:Agent 14.08 s 13.87 s [+0.7%; +2.2%] (maybe worse)
startup:insecure-bank:tracing:Agent 12.94 s 13.00 s [-1.3%; +0.3%] (no difference)
startup:petclinic:appsec:Agent 16.95 s 16.60 s [+1.2%; +2.9%] (significantly worse)
startup:petclinic:iast:Agent 16.82 s 16.44 s [-1.9%; +6.6%] (no difference)
startup:petclinic:profiling:Agent 16.17 s 16.87 s [-8.3%; +0.0%] (no difference)
startup:petclinic:sca:Agent 16.86 s 16.86 s [-1.2%; +1.2%] (no difference)
startup:petclinic:tracing:Agent 15.62 s 16.05 s [-6.7%; +1.4%] (no difference)

Commit: b9d09221 · CI Pipeline · Benchmarking Platform UI


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 Jun 25, 2026
- 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>
@dougqh dougqh added the tag: performance Performance related changes label Jun 30, 2026
@dougqh dougqh marked this pull request as ready for review June 30, 2026 20:16
@dougqh dougqh requested a review from a team as a code owner June 30, 2026 20:16
@dougqh dougqh requested a review from ygree June 30, 2026 20:16

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread internal-api/build.gradle.kts Outdated
}

// `a` is a stored name on an occupied slot (never null); `b` is a non-null query.
private static boolean eq(String a, String b) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how it would compare to a simple call toString.equals that already has the same shortcut check and likely gets inlined.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, for the most part, I'd expect it to optimize the same in this case.

Since the JIT knows both are Strings, the equals call can devirtualized and inlined. The only real complicating factor is null handling.

There's an implied a != null before the a.equals call, but that's speculatively eliminated by the JIT.

At this point, I honestly cannot remember. I put most of this together a long time in a prototype. I'm only merging it now because I'll soon have a real use case for it.

@dougqh dougqh Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had Claude run a detailed comparison...

Good question — I benchmarked it rather than guess. JMH on Zulu 21 (HotSpot C2), guarded (a == b || a.equals(b)) vs plain (a.equals(b)), across identity-hit / equal-but-distinct-instance / miss, with equals both inlined and forced non-inlined. Quick directional pass (-f 2):

Inlined — the monomorphic String case, i.e. this code path:

scenario guarded plain
interned hit 0.56 0.67
distinct hit 1.79 1.78
miss 1.11 1.17

→ a wash. distinct-hit and miss are dead even; the interned-hit "edge" is 0.1 ns, inside -f 2 noise. So you're right: when String.equals inlines (which it does for a mono String receiver), its own leading this == anObject already provides the identity fast-path, and the explicit guard adds nothing.

The guard only separates when equals can't inline: on an identity hit it saves ~1.6 ns by short-circuiting the call (0.57 vs 2.17 ns), but it costs ~0.3 ns on miss/distinct (a wasted == before the inevitable call). So it's a narrow bet — it pays only if equals doesn't inline and identity hits dominate.

Bottom line: in this path (mono String, equals inlines) the guard is neither win nor loss — fine to keep as cheap belt-and-suspenders, equally fine to drop for simplicity. The "interned literals hit the == fast path" comment is accurate, but inlined equals already gives that.

(Quick -f 2 pass; happy to firm up with -f 5 if useful.)

— Claude (Claude Code)

@dougqh dougqh Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the benchmark above, I decided to drop the reference check.

From Claude...
Decided to drop it — done in 9b6c445. Two reasons:

  1. The benchmark above showed the a == b guard is a wash when equals inlines, which is this path's regime.
  2. The eq() helper was itself an extra call in the hot probe, eating into MaxInlineDepth. Inlining .equals() directly at the probe flattens indexOf → equals by a level, which makes equals more likely to inline — i.e. the exact regime where the guard is moot.

So both the guard and the helper are gone; the probe is now hashes[i] == h && names[i].equals(name). Thanks for the nudge.

— Claude (Claude Code)

@bric3 bric3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread internal-api/build.gradle.kts Outdated
Comment thread internal-api/build.gradle.kts Outdated
@bric3 bric3 changed the title Add StringIndex: a generic open-addressed string set Add StringIndex: a generic open-addressed string set Jul 9, 2026
Comment thread internal-api/src/main/java/datadog/trace/util/StringIndex.java Outdated
@dougqh dougqh changed the base branch from master to dougqh/tagmap-read-through-consumer July 15, 2026 19:41

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Datadog Autotest: PASS

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.

Was this helpful? React 👍 or 👎

📊 Validated against 166 scenarios · Open Bits AI session

🤖 Datadog Autotest · Commit 1d8fd80 · What is Autotest? · Any feedback? Reach out in #autotest

@dougqh dougqh force-pushed the dougqh/tagmap-read-through-consumer branch from 3b9156e to d2aa48f Compare July 15, 2026 21:19
StringIndex is a compact open-addressed string→index structure (the keyOf
substrate the dense tag store builds on): parallel hash/name arrays, linear
probing, on par with HashSet on lookup at a smaller footprint. Includes unit
tests, a footprint test (jol), and comparison benchmarks (vs HashSet/switch).

No TagMap changes — standalone util. Rebased onto the level-split stack
(consumer #11932) as the layer dense-store sits on.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@bric3

bric3 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

coverage is violated:

Rule violated for class datadog.trace.util.StringIndex: instructions covered ratio is 0.7, but expected minimum is 0.8

@dougqh

dougqh commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

coverage is violated:

Rule violated for class datadog.trace.util.StringIndex: instructions covered ratio is 0.7, but expected minimum is 0.8

@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.

… gate

Re-applies the coverage fix dropped by the branch rebase/restack.
jacocoTestCoverageVerification flags StringIndex at 0.7 instruction
coverage (min 0.8): the instance long[] API (mapLongValues / lookup /
lookupOrDefault) and the Support.numSlots(int[]) static were never
exercised. Add two tests covering them.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp: core Tracer core tag: ai generated Largely based on code generated by an AI or LLM tag: no release notes Changes to exclude from release notes tag: performance Performance related changes type: feature Enhancements and improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants