Unify Hashtable static API with ConcurrentHashtable; deprecate Support (perf toolbox) by dougqh · Pull Request #12101 · DataDog/dd-trace-java

dougqh · 2026-07-29T15:14:18Z

What Does This Do?

Reshapes Hashtable so the three tables in this family — Hashtable, FlatHashtable, ConcurrentHashtable — read as one thing, and so a factory name states what it promises the caller rather than how it is built.

Behaviour-neutral for the one existing consumer apart from the strict cap described below. The API is validated by #12312, stacked on top, which migrates client-side statistics onto it.

Why

Two problems.

The building blocks lived in different places — nested under a Support class on Hashtable, flat on ConcurrentHashtable — so moving between the two meant relearning the surface.

And "fixed" meant two contradictory things:

before	fixed spine?	caps entries?	`getOrCreate` past cap
`FlatHashtable.createFixed`	yes	yes	returns `null`
`Hashtable.D1(capacity)`	yes	yes	returns `null`
`ConcurrentHashtable.D1.createFixedBuckets`	yes	no	never refuses

Two of three promised a cap, one promised the opposite. So createFixed gave opposite answers to "will this refuse my insert?" depending on which class you were in. Names now state the promise:

createCapped(maxCapacity) — refuses past the cap. Bounded entries, and with them a bounded footprint: the posture an agent living in someone else's heap should have by default.
createUncapped(expectedCapacity) — reserved, not built. Nothing needs it, and it should read as a migration bridge rather than a peer default.
FlatHashtable keeps createFixed/createGrowable: for open addressing, growth is a correctness requirement, not a performance choice.

How to review this

27 commits, but they are not equally interesting. Suggested order:

The selection guide at the top of Hashtable (mirrored in FlatHashtable) — two questions that decide which of the three you want. If that reads wrong, everything below it is wrong.
D1 end to end — the tier most callers touch: createCapped, get, insert, tryGetOrCreate, tryInsertOrReplace, remove, forEach, clear, drain.
SizeManager — the one genuinely new concept.
State and the statics that take it — the composer tier, for tables D1/D2 don't fit.
Everything else is renames, javadoc, and tests.

If you only read one thing, read 1 and 2. And #12312's diff is ~30 lines and shows this API from the outside — it may be the faster way to judge whether it is any good.

Entries vs. buckets

Every table factory takes entries; only the low-level allocator takes buckets, with capacityFor as the sole bridge:

// what most callers touch — the number is always entries
Hashtable.D1.createCapped(MyEntry.class, maxCapacity)
Hashtable.D2.createCapped(MyEntry.class, maxCapacity)
Hashtable.createCapped(maxCapacity)               // State: spine + SizeManager, for composers

// operations that can refuse say so in the name
TEntry  e  = table.tryGetOrCreate(key, MyEntry::new);   // @Nullable
boolean ok = table.tryInsertOrReplace(entry);           // false == refused

// low level: buckets, load factors, raw building blocks
Hashtable.create(int buckets) / create(Class<E>, int buckets)
Hashtable.capacityFor(cardinalityLimit[, loadFactor])
Hashtable.DEFAULT_LOAD_FACTOR                     // 0.75 — chaining degrades gracefully past 1.0

Sizing a table from a bucket count is the HashMap(initialCapacity) footgun — new HashMap<>(1000) expecting 1000 entries resizes at 750, which is why Guava added newHashMapWithExpectedSize. Worse here, because the right load factor differs per class (0.5 open-addressed, 0.75 chained), so a caller should never need to know it.

D1/D2 constructors are private — createCapped is the only way in, so the posture is explicit at the call site.

`SizeManager` — the one new concept

Reserving a slot and evicting to make room are two directions of one policy, so they live on one object. Keeping them apart meant wiring a cursor to a tracker and remembering to decrement after every unlink — and a missed decrement leaks the cap silently until the table stops accepting anything.

Folded, that class of mistake disappears, and the halves compose into the call a self-evicting miss path actually wants:

if (!Hashtable.tryReserveOrEvict(state, STALE)) {
  return null;                                  // full, nothing evictable — drop the datum
}
Hashtable.insertReserved(state, keyHash, buildEntry());

That replaces an isFull() check plus a hand-rolled evict-and-retry. In #12312 it deletes ~25 lines of cursor-resumed scan from AggregateTable, and the caller no longer knows a cursor exists while still getting its amortization.

Parameter order for the size-tracked statics puts sizeManager/state first. Appending it made tracked and untracked forms differ only in a trailing argument — the wrong shape for a distinction that fails silently and asymmetrically: a missed increment refuses inserts early and gets noticed, a missed decrement leaks the cap until nothing is accepted.

Behaviour changes

Strict entry-count cap on D1/D2. maxCapacity is a hard cap on live entries, not a sizing hint:

insert() returns boolean — false at capacity, instead of growing unboundedly.
tryGetOrCreate() returns null at capacity when the key is absent; a hit is always returned.
tryInsertOrReplace() returns boolean — false only when the key is absent and the table is full; a replacement never grows the table so it always succeeds. It previously threw IllegalStateException, which turned a designed steady state into an exception and allocated a throwable exactly when the table was under most pressure.
The bucket array is sized with load-factor headroom over the cap via capacityFor.

CardinalityLimitReporter — the one existing consumer — is updated for the boolean insert() and the new factory. Its table was previously unbounded; it is now capped at 64.

getOrCreate → tryGetOrCreate, and @Nullable. It was annotated @Nonnull while returning null at capacity, which its own javadoc documented — the name reading as total is how the wrong annotation got there. FlatHashtable is renamed too, since it refuses when fixed; ConcurrentHashtable keeps the plain name because it is uncapped and genuinely cannot refuse.

estimateSize / isLikelyEmpty on the composer tier. A reservation counts the moment it is taken, so between reserving and linking the count reads one high. D1/D2 keep an exact size() — they reserve and link inside one call, so the window is never observable from outside.

D1/D2.remove no longer allocate a capturing predicate. They delegated to removeMatching with e -> e.matches(key), which captures and so cannot be cached by LambdaMetafactory, while tryInsertOrReplace beside it walks the same chain with no lambda. Escape analysis often erases this and remove has no production caller, so this is a consistency fix rather than a measured throughput win.

Out of scope

Deleting Support. It is now a pure delegating facade holding no logic. Use the unified Hashtable API in client-side stats (perf toolbox) #12312 removes the last caller and deletes it.
ConcurrentHashtable's port — the rename, the fuzzy cap, and a drain built on getAndSet rather than unhooking entries (unhooking would truncate an in-flight lock-free reader). Checklist on APMLP-1532.
tryCreateOrUpdate and canned counter entries — the get-then-mutate idiom. APMLP-1669.
CardinalityLimitReporter → FlatHashtable. It never removes an individual entry — it clears wholesale each reporting cycle — so by the new selection guide it belongs on the open-addressed side. APMLP-1797.

Review passes already run

/techdebt, /perf-review, and Codex. Codex found two real defects, both fixed with regression tests verified to fail against the pre-fix code: evictAll deferred its count subtraction past a predicate that could throw, and drain handed entries to the sink with next intact so a retaining sink pinned the chain. Two suggestions were rejected, with reasons in the threads.

Test plan

./gradlew :internal-api:test — full module suite
./gradlew :internal-api:jacocoTestCoverageVerification
./gradlew :internal-api:compileJmhJava — benchmarks updated for the new factories
./gradlew :dd-trace-core:compileJava and :dd-trace-core:test --tests "datadog.trace.common.metrics.*"
Cap enforcement covered on both D1 and D2; tryReserveOrEvict covered for reserve, evict-to-make-room, and refuse

🤖 Generated with Claude Code

dougqh · 2026-07-29T15:32:38Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e4643cba67

ℹ️ 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".

chatgpt-codex-connector · 2026-07-29T15:41:49Z

   */
+  @Deprecated
  public static final class Support {
+    private Support() {}


Preserve the deprecated Support constructor

Because Support was a public nested class with an implicit public no-arg constructor, making the constructor private breaks source and binary compatibility for any existing consumer that instantiated the facade, even if only as a namespace: recompilation now fails, and already-compiled bytecode can hit an access error when loading against this version. Since this change is explicitly keeping Support as a deprecated compatibility facade, leave a deprecated public no-op constructor (or omit the explicit constructor) until the facade is actually removed.

Useful? React with 👍 / 👎.

issue: Confirmed at 18ef04d. At the merge base, public Support declared no constructor, so Java supplied a public no-arg constructor. The current private constructor breaks source access and can cause IllegalAccessError for existing bytecode. Keep a deprecated public no-op constructor while the facade is retained for compatibility.

Confirmed, and intentional for now: `Support` is deprecated and on its way out, but client-side stats still depends on it, so I'm holding off on removing/fixing it further here and will handle the compat break (and the constructor/`MAX_BUCKETS` gaps together) in a follow-on PR once that caller is migrated off it.

🤖 Reply drafted with Claude Code

chatgpt-codex-connector · 2026-07-29T15:41:49Z

     * size {@code n} should pass {@code create(n, MAX_RATIO)} to leave ~25% headroom in the array.
     */
-    public static final float MAX_RATIO = 4.0f / 3.0f;
+    @Deprecated public static final float MAX_RATIO = 4.0f / 3.0f;


Retain Support.MAX_BUCKETS in the facade

Right next to the retained MAX_RATIO, the deprecated facade no longer exposes the package-private MAX_BUCKETS constant that Support previously had. Any in-package consumer (including downstream tests or custom table helpers compiled in datadog.trace.util) that sizes or bounds-checks against Hashtable.Support.MAX_BUCKETS now fails to recompile even though the facade is advertised as source-compatible; keep a deprecated Support.MAX_BUCKETS alias to the outer constant.

Useful? React with 👍 / 👎.

issue: Confirmed at 18ef04d despite the outdated anchor. The merge-base Support exposed package-private MAX_BUCKETS, while the current facade omits it but retains package-private sizeFor. Existing same-package sources referencing Support.MAX_BUCKETS no longer compile; add a deprecated alias if source compatibility is intended.

Same as the constructor-visibility finding above — deferring to the follow-on PR that removes `Support` once client-side stats is migrated off it, rather than patching the deprecated facade further here.

🤖 Reply drafted with Claude Code

dd-octo-sts · 2026-08-26T16:45:00Z

🟢 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.88 s	14.72 s	[-0.1%; +2.2%] (no difference)
startup:insecure-bank:tracing:Agent	13.62 s	13.72 s	[-1.5%; +0.1%] (no difference)
startup:petclinic:appsec:Agent	16.95 s	16.69 s	[+0.7%; +2.4%] (maybe worse)
startup:petclinic:iast:Agent	16.34 s	16.93 s	[-7.6%; +0.7%] (no difference)
startup:petclinic:profiling:Agent	16.17 s	16.62 s	[-7.0%; +1.6%] (no difference)
startup:petclinic:sca:Agent	16.84 s	16.65 s	[+0.3%; +2.1%] (maybe worse)
startup:petclinic:tracing:Agent	16.01 s	15.73 s	[-2.5%; +6.1%] (no difference)

Commit: cbc18556 · 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.

Addresses two review comments on #12101. Throwing IllegalStateException at capacity was the odd one out: insert returns false and tryGetOrCreate returns null, so one class had three refusal conventions. A cap is designed steady-state behaviour rather than a programming error, and an exception allocates a throwable plus stack trace exactly when the table is under the most pressure -- the failure path costing more than the happy path. The throw existed because null already meant "inserted fresh", leaving no spare return value for "refused". Dropping the prior-entry return frees one up: Map.put's return value is rarely read, and a caller that wants it can get() first. So the operation becomes a plain boolean, false only when the key is absent and the table is full -- a replacement swaps one entry for another without growing, so it always succeeds. That also lets the fresh-insert path go through the size-tracked static insertHeadEntryFor(sizeTracker, ...) instead of a separate tryReserve followed by the untracked form, so the class now uses the same one-call shape it offers composers. tryGetOrCreate deliberately keeps isFull() -> create -> increment: its creator runs between the check and the link and may throw, so a slot reserved up front could leak. Commented at the call site so the asymmetry does not read as an oversight. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Addresses the review comments on #12312, with the API additions made in #12101 and percolated here: state.sizeManager.size() -> Hashtable.size(state) ... == 0 -> Hashtable.isEmpty(state) bucketFor(state.buckets, hash) -> bucketFor(state, hash) insertHeadEntryFor(state.buckets, ...) -> insertReserved(state, ...) forEach(state.buckets, ...) -> forEach(state, ...) No reference to state.buckets or state.sizeManager remains -- what State holds is now its own business. Also drops the field comment that re-documented the eviction cursor living inside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

dougqh · 2026-08-26T23:14:13Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 050c304f16

ℹ️ 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".

chatgpt-codex-connector · 2026-08-26T23:20:14Z

+    Hashtable.<TEntry>forEach(buckets, sink);
+    clear(buckets);


Reset tracked occupancy when draining a State

When a caller drains a capped State through the only available static API, drain(state.buckets, sink), this clears the buckets but leaves state.sizeManager unchanged. After draining a full state, size(state) still reports the old count and tryReserveOrEvict can refuse new entries even though the table is empty. Provide a State/SizeManager-aware drain overload, analogous to clear(State), that resets the occupancy after a successful drain.

Useful? React with 👍 / 👎.

Addressed in 33e9f4f, though not by changing this method. drain(Entry[], sink) is the raw building block and deliberately knows nothing about occupancy -- same as clear(Entry[]) and insertHeadEntryFor(Entry[], ...) beside it.

What was missing was the tracked counterpart. Added drain(sizeManager, buckets, sink), its context-passing form, and both State overloads, so a caller holding a State has a form that resets the count; D1.drain/D2.drain now route through it instead of pairing the two calls by hand. The array-taking form stays for composers driving a spine with no manager at all.

note: Confirmed fixed for the reported successful-drain path. State, SizeManager plus buckets, D1, and D2 now route through tracked overloads and reset the manager. The separate exceptional-exit defect remains in #12101 (comment).

chatgpt-codex-connector · 2026-08-26T23:20:14Z

+          count++;
+        }
+      }
+      this.size -= count;


Update occupancy as each eviction succeeds

If evictable.test(...) throws after one or more earlier matches were removed, control never reaches this deferred subtraction, so the bucket chains have changed while size still counts every removed entry. A caller that catches the predicate exception can then observe an inflated size or have later reservations refused despite available capacity; decrement per successful removal or reconcile the accumulated count in a finally block.

Useful? React with 👍 / 👎.

Real bug, fixed in 3909184. evictAll subtracted its running count after the loop, so a predicate throwing part way through left the already-unlinked entries gone from the chains while the count still counted them -- permanently high, which in a capped table eventually means it stops accepting anything. Now decrements per removal, matching evictOne.

Regression test added and verified to fail against the pre-fix code: drives evictAll with a predicate that throws mid-pass, then asserts the count matches what is actually still on the spine.

note: Confirmed fixed at 18ef04d. SizeManager.evictAll decrements immediately after each unlink, and the regression test compares the tracked count with the remaining entries when the predicate throws.

chatgpt-codex-connector · 2026-08-26T23:20:14Z

+  public static <TEntry extends Entry> void drain(
+      @Nonnull Hashtable.Entry[] buckets, @Nonnull Consumer<? super TEntry> sink) {
+    Hashtable.<TEntry>forEach(buckets, sink);
+    clear(buckets);


Detach entries while draining them

When multiple entries share a bucket, this only nulls the bucket-array slot after visiting them and never clears their next links. If a drain sink retains or publishes the former head, that supposedly removed entry continues retaining the rest of the drained chain, unlike entries removed through the mutating iterators; repeated flushes can therefore retain substantially more entry state than the sink intended. Unlink each entry as it is drained.

AGENTS.md reference: AGENTS.md:L77-L77

Useful? React with 👍 / 👎.

Real, fixed in 3909184. drain was forEach followed by Arrays.fill, so entries reached the sink with next intact and a sink retaining one entry of a chain pinned everything behind it -- including entries it had chosen to drop.

Now a single pass that nulls the bucket slot and unhooks each entry before handing it over, reading next first since the sink may do anything with the entry once it has it. That also removes the second pass. Regression test drains a forced collision chain and asserts the drained entries are detached.

note: Confirmed fixed at 18ef04d. Both raw drain forms save next, clear the current entry next link, and only then invoke the sink. The colliding-chain regression test asserts that every drained entry is detached.

Addresses the review comments on #12312, with the API additions made in #12101 and percolated here: state.sizeManager.size() -> Hashtable.size(state) ... == 0 -> Hashtable.isEmpty(state) bucketFor(state.buckets, hash) -> bucketFor(state, hash) insertHeadEntryFor(state.buckets, ...) -> insertReserved(state, ...) forEach(state.buckets, ...) -> forEach(state, ...) No reference to state.buckets or state.sizeManager remains -- what State holds is now its own business. Also drops the field comment that re-documented the eviction cursor living inside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

dougqh · 2026-08-27T00:23:43Z

@arp review

Addresses the review comments on #12312, with the API additions made in #12101 and percolated here: state.sizeManager.size() -> Hashtable.size(state) ... == 0 -> Hashtable.isEmpty(state) bucketFor(state.buckets, hash) -> bucketFor(state, hash) insertHeadEntryFor(state.buckets, ...) -> insertReserved(state, ...) forEach(state.buckets, ...) -> forEach(state, ...) No reference to state.buckets or state.sizeManager remains -- what State holds is now its own business. Also drops the field comment that re-documented the eviction cursor living inside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

chatgpt-codex-connector

💡 Codex Review

dd-trace-java/internal-api/src/main/java/datadog/trace/util/Hashtable.java

Line 1691 in 2d6bdb9

oldCurEntry.setNext(null);

Preserve the chain when replacing an entry with itself

When replacementEntry is the same object returned by next(), this detachment clears the replacement's own next pointer after it has been spliced back into place. Thus an idempotent call such as table.tryInsertOrReplace(table.get(key)) silently disconnects every later entry in a colliding bucket while the size manager continues counting them; subsequent lookups lose those entries and the table may refuse inserts prematurely. Treat an identity replacement as a no-op, or only detach oldCurEntry when it differs from replacementEntry.

ℹ️ 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".

chatgpt-codex-connector · 2026-08-27T03:08:52Z

+    Hashtable.<TEntry>drain(buckets, sink);
+    sizeManager.reset();


Keep occupancy consistent when a drain sink throws

When the supplied Consumer throws during a tracked drain, the raw drain has already unlinked at least one bucket, but control skips sizeManager.reset(). Callers that catch the exception—including users of the new D1, D2, and State drain wrappers—then observe an inflated size and may have subsequent inserts refused despite freed capacity. Update the occupancy as entries are unlinked, or otherwise preserve the remaining chains and reconcile bookkeeping on exceptional exit; the context-passing overload has the same issue.

Useful? React with 👍 / 👎.

issue: Both tracked overloads call raw drain and reset only after it returns. Raw drain clears the bucket before invoking the sink; a focused reproducer leaves zero reachable entries but a tracked size of 3 after either Consumer or BiConsumer throws.

chatgpt-codex-connector · 2026-08-27T03:08:52Z

+          int bucket = iter.currentBucket();
+          iter.remove();
+          this.cursor = bucket;


Advance eviction within a colliding bucket

perf: When many entries collide into one bucket and a non-evictable prefix precedes multiple evictable entries, recording only the removed entry's bucket makes every subsequent evictOne restart at that chain's head. A sequence of successful evictions therefore rescans the same hot prefix each time and can become quadratic, contrary to the method's stated amortization; this is particularly costly when key hashes are externally controlled. Preserve a position within the chain (or otherwise rotate past the examined prefix), and verify the result with a colliding-key JMH case.

AGENTS.md reference: AGENTS.md:L77-L77

Useful? React with 👍 / 👎.

issue: SizeManager stores only the bucket index and starts a fresh iterator at the chain head after each match. With 10 entries that are not evictable followed by 5 evictable colliders, five successful evictions make 55 predicate calls because the 10-entry prefix is scanned each time.

datadog-prod-us1-4

A sink exception leaves the table count above the number of reachable entries. Repeated eviction in one colliding bucket also scans the same prefix and can take quadratic time.

_{Open Bits AI session}

_{🤖 Datadog Autotest · Commit 2d6bdb9 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest}

datadog-prod-us1-4 · 2026-08-27T03:14:40Z

+      @Nonnull Hashtable.Entry[] buckets,
+      @Nonnull Consumer<? super TEntry> sink) {
+    Hashtable.<TEntry>drain(buckets, sink);
+    sizeManager.reset();


Keep the drain count correct after a sink exception

Later inserts can fail although the table has free capacity, and size reports become wrong.

Assertion details

Input: A Consumer or BiConsumer throws while a tracked drain is in progress.

Expected: The count must match the entries that remain after an exception. Both tracked drain forms need safe count updates. Their tests must cover a Consumer and a BiConsumer that throws.

Actual: The raw drain removes a bucket before it calls the sink. An exception stops sizeManager.reset(). The count then includes entries that the table no longer holds.

Was this helpful? React 👍 or 👎
_{🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest}

note: Duplicates #12101 (comment). The focused Consumer and BiConsumer reproductions both leave a tracked size of 3 with zero reachable entries after the sink throws.

datadog-prod-us1-4 · 2026-08-27T03:14:40Z

+        if (evictable.test((TEntry) candidate)) {
+          int bucket = iter.currentBucket();
+          iter.remove();
+          this.cursor = bucket;


Keep eviction progress inside a colliding chain

Collision-heavy input can make repeated successful eviction take quadratic time and cause high CPU use.

Assertion details

Input: One bucket has a long non-evictable prefix followed by several evictable entries. The caller repeatedly uses evictOne or tryReserveOrEvict.

Expected: The cursor must keep progress inside the chain, or it must move past the scanned prefix. Add a test with a long colliding prefix and repeated successful evictions.

Actual: The cursor stores only the bucket number. Each successful eviction from that bucket starts again at the chain head and scans the same prefix.

Was this helpful? React 👍 or 👎
_{🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest}

note: This duplicates #12101 (comment). The colliding-chain reproducer makes 55 predicate calls for five successful evictions because the 10-entry non-evictable prefix is rescanned each time.

Adds D1.removeIf/D2.removeIf plus the static building-block overloads, matching ConcurrentHashtable's removeIf shape. Delegates to the existing SizeManager.evictAll full-table sweep rather than reimplementing traversal.

Adds D1/D2 tryGetOrCreateOrEvict and tryGetOrCreateOrEvictOrNull, matching ConcurrentHashtable's shape and its eviction-before-creator ordering (creator may throw, so the freed slot is not reserved until after it succeeds).

These fused create+update overloads predate Maybe's update() methods proving out as allocation-free under escape analysis, and were an experiment from before that. Maybe.getOrNull()/update() now cover the same shape generically with no production callers of the removed methods.

@setup

Rebasing this branch reintroduced BenchmarkUtils pollution wiring that had already landed independently on the base branch, producing two identically-named @setup(Level.Trial) setUp() methods in TagMapAccessBenchmark (a silent auto-merge, not a flagged conflict). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

drain(SizeManager, ...) delegated to the plain drain then called sizeManager.reset() afterward, so a sink that threw mid-drain skipped the reset entirely -- leaving the count at its pre-drain value even though some entries had already been unlinked, permanently consuming capacity. Decrement per-entry inside the sink wrapper instead, so a partial drain leaves the count matching exactly what the buckets still hold. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

evictOne's inner scan always restarted from the bucket head, so a chain with a non-evictable prefix of length P followed by many evictable entries paid O(P) on every single eviction pulled from that bucket -- N evictions from one hot bucket cost O(P*N) instead of amortized O(chain length). Track the predecessor of the last evicted entry (cursorPrev) alongside the existing bucket cursor, and add a MutatingTableIterator constructor that resumes a chain right after that predecessor instead of at the bucket head. Safe against the predecessor itself having been removed in the meantime: Entry.next() is read lazily, and remove() nulls a removed node's next(), so that case only causes a bounded, self-healing under-scan of the bucket on this one pass, not corruption or lost entries. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

capacityFor(cardinalityLimit, loadFactor) truncated cardinalityLimit / loadFactor to an int before handing off to sizeFor's power-of-two rounding. For small limits that truncation could collapse the requested headroom away entirely -- e.g. capacityFor(1, 0.75f) truncated 1/0.75 = 1.333 down to 1, sizing a 1-bucket array (a 1.0 load factor, not the documented 0.75). Divide in double and round up via Math.ceil before rounding to a power of two, matching the pattern FlatHashtable#capacityFor already uses. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

DEFAULT_LOAD_FACTOR's doc had chain length inverted (~1/factor instead of ~factor); SizeManager.isFull() linked to a nonexistent size() method instead of estimateSize(); State's class doc overclaimed that it "prevents" array/manager drift when its fields were public. Narrowed buckets/sizeManager to package-private, since only datadog.trace.util calls the static building blocks they feed, which also makes the doc's togetherness claim accurate rather than aspirational. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@return

Applies review feedback selectively: adds missing @param/@return tags throughout, trims a couple of inline exception-safety comments while keeping their design-contrast reasoning (why this method's ordering differs from insert/tryInsertOrReplace or tryReserveOrEvict), and states the null contract explicitly on FlatHashtable's tryGetOrCreateOrNull variants. Declines suggestions that would have dropped genuine nuance (createBounded's sizing-ballpark guidance, tryGetOrCreate's refusal-is-steady-state warning, the "under/over promising" rationale for the try prefix, and the evictAll/drain naming disambiguation). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

dougqh · 2026-09-24T16:57:31Z

@bric3 Went through all the outstanding javadoc suggestions and pushed updates in 73204b0 and fa4f452. Summary rather than replying inline to each:

Fixed as genuine bugs:

DEFAULT_LOAD_FACTOR: average chain length was documented backwards (~1/DEFAULT_LOAD_FACTOR instead of ~DEFAULT_LOAD_FACTOR).
SizeManager.isFull(): dead {@link #size()} reference, fixed to estimateSize().
State: softened the "prevents drift" overclaim and narrowed buckets/sizeManager to package-private, which also makes the togetherness claim actually true.

Applied with adjustments (added missing @param/@return tags across createBounded/tryGetOrCreate*, tightened a couple of inline exception-safety comments) while keeping design rationale we didn't want to lose — e.g. why tryGetOrCreateOrNull's ordering differs from insert/tryInsertOrReplace, and the under/over-promising reasoning for the try prefix on FlatHashtable's nullable variants.

Left as-is / declined:

createBounded's sizing-ballpark guidance and "capped names the promise not the mechanism" framing — real nuance, not filler.
tryGetOrCreate's "refusal is a steady state, decide deliberately" warning — this is the part that prevents silent data loss from an ignored Maybe.
evictAll's disambiguation from drain — explains a naming decision the method signature alone doesn't convey.

Several other suggestions (class-level "choosing between the three tables" docs, estimateSize/tryReserve/tryReserveOrEvict docs, the inline evictAll decrement comment) were already superseded by earlier revisions in this branch and didn't need further changes.

Let me know if any of the "declined" calls don't land for you — happy to revisit.

🤖 Reply drafted with Claude Code

…unification

datadog-prod-us1-4

A drain callback that throws after a bucket is cleared can leave the size count above the live entry count. Later inserts can fail before the table reaches its cap.

_{Open Bits AI session}

_{🤖 Bits Code Review · Commit bccf961 · @DataDog review to ask questions}

datadog-prod-us1-4 · 2026-09-24T17:26:45Z

+      @Nonnull SizeManager sizeManager,
+      @Nonnull Hashtable.Entry[] buckets,
+      @Nonnull Consumer<? super TEntry> sink) {
+    drain(


Keep the count correct when a chained drain throws

Later inserts can fail before the table reaches its configured cap.

Assertion details

Input: Drain a tracked bucket with colliding entries. Make the sink throw on an early entry.

Expected: The count must match the entries that remain after the callback throws.

Actual: The drain clears the full bucket first. It decreases the count only for entries that reach the callback before the exception.

Was this helpful? React 👍 or 👎
_{🤖 Bits Code Review · @DataDog review to ask questions} _{· Open Bits AI session}

AlexeyKuznetsov-DD · 2026-09-24T17:35:14Z

@dougqh Is it possible to rebase to latest master? This PR has noisy diff with unrelated changes shown in GitHub diff UI.

dougqh · 2026-09-24T20:16:47Z

@dougqh Is it possible to rebase to latest master? This PR has noisy diff with unrelated changes shown in GitHub diff UI.

@AlexeyKuznetsov-DD Sure, I'll do that now

…llution' into feat/hashtable-api-unification

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: refactoring labels Jul 29, 2026

This comment has been minimized.

Sign in to view

chatgpt-codex-connector Bot reviewed Jul 29, 2026

View reviewed changes

dougqh force-pushed the feat/hashtable-api-unification branch from 43fdfe2 to c04c3de Compare August 26, 2026 16:20

dougqh changed the base branch from master to dougqh/benchmarkutils-map-set-pollution August 26, 2026 16:20