Add find-or-insert Reservation API to ConcurrentHashtable by dougqh · Pull Request #12462 · DataDog/dd-trace-java

dougqh · 2026-09-11T12:18:06Z

What Does This Do

Adds a find-or-insert Reservation handle to ConcurrentHashtable: an auto-cancelling, deferred-construction API that replaces the separate D1/D2 entry hierarchies with a single self-bound Entry<TEntry> type (mirroring Enum<E extends Enum<E>>).

Rebuilds this API on top of bric3's merged LogCollector change (#12367), rather than rebasing the earlier branch, so this is a clean diff against LogCollector's current, real implementation. Updates LogCollector to use Reservation, taking it inside the existing table write lock (rather than lock-free ahead of it) so the find-or-insert decision stays serialized with drain() and other writers — no duplicate reservation can be mistaken for a full table.

Also swaps ConcurrentHashtable's synchronized/monitor-object locking for ReentrantLock: a Reservation holds the lock open across separate calls (from tryReserve until finish/close), which a lexically-scoped synchronized block can't do.

Motivation

The original Reservation-less API (#12367) forced LogCollector to hand-roll a racy lock-free reject-when-full path and box its keys to fit the D1/D2 generic shape. This introduces a single Reservation API that defers entry construction until a slot is actually available, auto-cancels an unused reservation via AutoCloseable, and drops the self-type-parameterized D1.Entry/D2.Entry boilerplate in favor of one Entry<TEntry> base type.

Switching to ReentrantLock was originally planned as a separate follow-up, but Reservation needed a lock whose acquisition and release could span two calls, which forced the move sooner than planned.

Additional Notes

drain(State, ...) also moved from holding the lock for the whole bucket-array sweep to acquiring/releasing it once per bucket, so a writer can slip in between buckets instead of waiting out the entire drain. Benchmarked trade-off (ConcurrentHashtableDrainBenchmark): draining alone is ~32x slower this way, but writers see ~33% higher throughput while a drain is in progress — accepted since writer throughput (the application-thread hot path) matters more here than drain throughput (an infrequent telemetry-flush operation). ConcurrentHashtableFindBenchmark separately confirms LogCollector.find()'s lock-free hashIterable/hashIterator scan doesn't allocate (eliminated by escape analysis).

Also closes a duplicate-undercounting race flagged in review: tryReserve composed its full-table check and slot increment non-atomically, and neither tryReserve nor LogCollector's hand-rolled recheck ever compared a full table's target bucket against the caller's key before giving up. Added tryReserveFor(state, keyHash), which keeps the write lock open on a full table when the target bucket is non-empty so Reservation's existing locked comparison can still catch a concurrent duplicate — LogCollector no longer needs its own recheck. Added LogCollectorBenchmark.variedKeysNearCapacity to exercise this locked path directly (the other cases only ever hit the lock-free fast path) and confirm it doesn't regress throughput.

A separate follow-up (tracked as APMLP-1838, under the APMLP-1513 epic) will compare synchronized vs ReentrantLock more generally, especially under virtual threads.

Contributor Checklist

Format the title according to the contribution guidelines
Assign the type: and (comp: or inst:) labels in addition to any other useful labels
Avoid using close, fix, or any linking keywords when referencing an issue
Use solves instead, and assign the PR milestone to the issue
Update the CODEOWNERS file on source file addition, migration, or deletion
Update public documentation with any new configuration flags or behaviors
Once approved, use merge queue to merge the PR

Jira ticket: [PROJ-IDENT]

🤖 Generated with Claude Code

Ports the ConcurrentHashtable Reservation API (auto-cancelling, deferred-construction find-or-insert handle; self-bound Entry<TEntry> replacing separate D1/D2 entry hierarchies) onto bric3's merged LogCollector change, and updates LogCollector to use it. Taking the reservation inside the existing table write lock (rather than lock-free ahead of it) keeps LogCollector's find-or-insert decision serialized with drain() and other writers, so no duplicate reservation can ever be mistaken for a full table. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

equals() now only performs the type check and defers the substantive comparison to matches(), avoiding the duplicated logic. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Use ConcurrentHashtable.hashIterable() in find() instead of a manual bucket walk with a keyHash pre-check, and drop the redundant isFull() fast-reject and the second find() done just before taking a reservation -- Reservation#tryGetOrInsertOrNull already performs that same locked comparison via matches(), so repeating it added nothing. That simplification exposed a real bug: tryGetOrInsertOrNull can return an existing match instead of the newly built entry, and that occurrence was never counted. RawLogMessage's live occurrence count now starts at zero so every find-or-insert can unconditionally increment() the returned entry, whether it's the new instance or an existing match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

dd-octo-sts · 2026-09-11T13:22:27Z

🟢 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.83 s	14.64 s	[+0.3%; +2.2%] (maybe worse)
startup:insecure-bank:tracing:Agent	13.56 s	13.71 s	[-1.9%; -0.2%] (maybe better)
startup:petclinic:appsec:Agent	17.66 s	17.39 s	[+0.8%; +2.3%] (maybe worse)
startup:petclinic:iast:Agent	17.51 s	17.64 s	[-1.5%; -0.0%] (maybe better)
startup:petclinic:profiling:Agent	17.55 s	17.46 s	[-0.7%; +1.6%] (no difference)
startup:petclinic:sca:Agent	17.60 s	17.51 s	[-0.6%; +1.6%] (no difference)
startup:petclinic:tracing:Agent	16.73 s	16.73 s	[-1.2%; +1.2%] (no difference)

Commit: c87c904a · 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 · 2026-09-11T13:44:18Z

      super(key);
    }
-
-    long increment() {


To Claude, can we put increment back to make the diff a little smaller.

Done — reverted this file back to master to drop the unrelated diff noise.

dougqh · 2026-09-11T13:45:52Z

  }

-  static final class D1Entry extends ConcurrentHashtable.D1.Entry<String> {
+  /**


To Claude - this Javadoc feels out of place. Was it relocated from elsewhere?
That's how it looks in the diff?

Good catch, yes -- it belongs on SharedState, not LongEntry. It got misplaced during the D1Entry -> LongEntry rework. Moved it back.

dougqh · 2026-09-11T13:46:59Z

    final long value;

-    D1Entry(String key) {
+    LongEntry(String key, long value) {


To Claude, can we keep the old name of D1Entry? I just want to minimize the diff.

Done — kept D1Entry, just added the value parameter.

…ain's lock hold Releases capacity per-entry during drain instead of once at the end, and acquires/releases the table lock per bucket rather than for the whole sweep, so writers can interleave with an in-progress drain instead of blocking for its entire duration. Also cleans up leftover synchronized(...) usage in ConcurrentHashtableSizeManagerTest and simplifies LogCollector.addLogMessage now that Reservation holds the lock itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

… lock contention Addresses the two flag-as-measure findings from the perf review of the ReentrantLock migration: - ConcurrentHashtableFindBenchmark confirms (via -Pjmh.profilers=gc) that the Iterable/Iterator returned by hashIterable/hashIterator is scalar-replaced away rather than allocated on LogCollector's find() hot path. - ConcurrentHashtableDrainBenchmark measures writer throughput against a continuously draining table, giving a repeatable baseline for drain()'s per-bucket lock acquisition versus holding the lock for the whole sweep. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Reimplements the pre-PR whole-sweep-lock drain() strategy locally (using only ConcurrentHashtable's public API) so both locking schemes can be measured side by side in one run, isolating lock granularity as the only variable (both variants release capacity per-entry). Measured at capacity=64 under 3-writer contention: per-bucket locking drains ~32x slower (0.12 vs 3.76 ops/us) but lets writers achieve ~33% higher combined throughput (262.6 vs 197.1 ops/us) while a drain is in progress. Accepted tradeoff -- LogCollector's writers are on the application-thread hot path and drain runs infrequently (once per telemetry flush), so favoring writer throughput over drain throughput is the right call for this caller. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Review feedback: the bare static-imported call read ambiguously at the call site; qualifying it makes clear which table API is in play. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

dougqh · 2026-09-11T14:47:30Z

-    int bucketIndex = bucketIndex(rawLogMessages.buckets, keyHash);
-    // Fast path for duplicates: search the target bucket without locking.
-    RawLogMessage rawLogMessage = find(bucketIndex, keyHash, logLevel, message, throwable);
+    // Fast path for duplicates: search lock-free before ever taking the table lock.


I wanted to make the look-up & insertion process as easy as possible without significantly trading off the performance gains, so I settled on...

first doing a lock-free and allocation-free find step.

and then I encapsulated most of the complexity of doing another find & insert pass into the Reservation class.

Reservation encapsulates several steps...

first a quick lock free capacity check - if that fails, you get back a nop reservation

if the capacity check is successful, reservation takes the lock (which is now a ReentrantLock)

then if the reservation was successful, you construct a new entry to try to insert

tryGetOrInsertOrNull embodies the rest of the process

scans again under lock for matching Entry -- using the new Entry.matches(Entry)

if found, returns the existing Entry

if not found, inserts the new Entry

The trade-off is that you do allocate an Entry object slightly more often, but only after the initial lock-free / allocation-free find has failed to find something. And ideally only after confirming that there's space in the table for the entry.

Reservation does have helpers for lazy construction, but in this case, I wasn't able to use one.

Quick update now that tryReserveFor has landed (16b56c8): addLogMessage calls tryReserveFor(state, keyHash) rather than the plain tryReserve() described above. The bullets still hold for tryReserve(), but tryReserveFor refines the first one — the lock-free short-circuit to a nop reservation only fires when the table's full and the target bucket is empty. If the bucket's non-empty, it still takes the lock (without claiming a slot) purely so the locked comparison pass can catch a concurrent duplicate before giving up — that's what closes the duplicate-undercounting race.

dougqh · 2026-09-11T14:50:33Z

-      if (entry.keyHash != keyHash
-          || !Objects.equals(logLevel, entry.logLevel)
-          || !Objects.equals(message, entry.message)) {
+    for (RawLogMessage entry : ConcurrentHashtable.hashIterable(rawLogMessages, keyHash)) {


I wanted to simplify the find process a bit.

I started by fixing hashIterator. It was supposed to filter & only return entries with matching hashes.

Somehow that got lost in porting over the design from Hashtable. Sorry about that.

And to allow use of the enhanced-for loop, I decided to add hashIterable, too.

I verified that both the Iterable and Iterator these return get eliminated by escape analysis, so there's no allocation cost to using these.

dougqh · 2026-09-11T14:52:01Z

-      if (o == null || getClass() != o.getClass()) return false;
-      RawLogMessage that = (RawLogMessage) o;
-
+    public boolean matches(RawLogMessage that) {


To make the new Reservation.tryGetOrInsert(OrNull)(Entry) method work, I needed Entry-to-Entry matching support. This does impose a bit more work in custom-Entry definition, but I think the simpler lookup-or-insert code is worth it.

dougqh · 2026-09-11T14:53:38Z

+   * true {@code Self} type, so this is enforced by convention, not the compiler.
   */
-  public abstract static class Entry {
+  public abstract static class Entry<TEntry extends Entry<TEntry>> {


Not parameterized on the self-type -- mostly to make implementing matches nicer.
The parameterization of the easy cases D1.Entry and D2.Entry is kept the same, so they stay simple.

dougqh · 2026-09-11T14:59:01Z

      }
-      synchronized (getTableWriteLock(state)) {
-        for (TEntry curEntry = bucketAt(state, index);
+      ReentrantLock lock = getTableWriteLock(state);


I was going to save switching to ReentrantLock for another PR, but Reservation drove me to do it sooner.

From what I understand, ReentrantLock is supposed to work better with virtual threads than synchronized, but I plan on benchmarking that separately.

The actual motivation here is that I wanted to have Reservation encapsulate the lock acquisition which isn't possible with synchronized.

… drain No current caller puts removeIf on a contended path, so leave it as a whole-sweep lock for now, but leave a pointer for future revisit since the restructuring done for drain() would apply the same way here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

dougqh · 2026-09-11T15:47:47Z

-      rawLogMessage = find(bucketIndex, keyHash, logLevel, message, throwable);
-      if (rawLogMessage != null) {
-        rawLogMessage.increment();
+    // Slow path after a miss: tryReserve holds the table write lock for the reservation's whole


To Claude - I don't think the inline comment needs to explain the details of Reservation or the interaction with drain

Done — trimmed to just what's needed at this call site.

dougqh · 2026-09-11T15:49:44Z

   * occupancy with a {@link State} instead of a bare counter — used by {@link D1#removeIf} and
   * {@link D2#removeIf}.
+   *
+   * <p>TODO: no current caller holds the lock across a full sweep the way {@code drain(State, ...)}


To Claude - let's move the TODO into a regular comment into the body of removeIf.
Also, please shorten this a bit

Done — moved into a body comment on removeIf and shortened it.

dougqh · 2026-09-11T15:53:29Z

+      @Strategy @Nonnull Consumer<? super TEntry> drainedEntryConsumer) {
+    ReentrantLock lock = getTableWriteLock(state);
+    AtomicReferenceArray<TEntry> buckets = state.buckets;
+    for (int i = 0; i < buckets.length(); i++) {


As explained in the Javadoc, I updated the locking scheme to allow for higher writer throughput.
I'm not actually striping the lock yet, but I'm limiting the lock hold time.

Each bucket of the table is processed under the lock, then the sizeManager is updated and the lock is released to allow writers to make progress concurrently.

I'm debating making a similar change to removeIf, but haven't done so yet.

- LogCollector.addLogMessage: drop the Reservation/drain interaction detail from the slow-path comment; it doesn't belong at this call site. - ConcurrentHashtable.removeIf: move the per-bucket-locking TODO from the Javadoc into a shorter body comment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

dd-octo-sts · 2026-09-11T16:27:05Z

Hi! 👋 Thanks for your pull request! 🎉

To help us review it, please make sure to:

Add at least one type, and one component or instrumentation label to the pull request

If you need help, please check our contributing guidelines.

datadog-datadog-prod-us1

When the first call reserves the last free slot, a second call for the same key can see a full table before the entry is visible. The second call returns an empty reservation, and LogCollector loses one log occurrence.

_{Open Bits AI session}

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

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fa44aac4ff

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

Closes the jacocoTestCoverageVerification gap on ConcurrentHashtable.Reservation by exercising the previously-untested 2-key tryGetOrInsertOrNull overload, the pre-built-entry tryGetOrInsertOrNull/tryGetOrInsert overloads, and the 2/3/4-key tryGetOrInsert (Maybe-wrapping) overloads. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

tryReserve/tryReserveSlot could increment the size counter and then reject the reservation if the atomic check failed, without releasing the slot; composed via the atomic sizeManager.tryReserve() check instead so success/failure and the increment happen together. Add tryReserveFor(state, keyHash), which closes a race where a full table rejects a reservation without ever checking whether the concurrent entry that filled the table is actually a match for the caller's key. It keeps the write lock open on a full table when the target bucket is non-empty, so Reservation.finish() can still scan for a concurrent duplicate even though no slot was claimed. LogCollector.addLogMessage now uses tryReserveFor instead of hand-rolling its own lock-free duplicate recheck. Also sync D1/D2 drain() Javadoc with the actual per-bucket locking behavior (previously described whole-sweep locking). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

The existing LogCollectorBenchmark cases all hit the lock-free find() fast path on every call, so they never exercise tryReserveFor's locked recheck. Add variedKeysNearCapacity, which cycles through more distinct keys than the table has capacity for, forcing most calls through the locked path this PR's fix touches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Every real caller already had a keyHash on hand, so the plain no-keyHash tryReserve was pure duplication once tryReserveFor existed to fix the undercounting race. Drop it and keep the single merged method under the original tryReserve name. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

dougqh mentioned this pull request Sep 11, 2026

Add find-or-insert Reservation API to ConcurrentHashtable #12454

Closed

6 tasks

dougqh and others added 2 commits September 11, 2026 08:25

Move RawLogMessage equality logic into matches(), have equals() delegate

b8b5037

equals() now only performs the type check and defers the substantive comparison to matches(), avoiding the duplicated logic. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

This comment has been minimized.

Sign in to view

dougqh commented Sep 11, 2026

View reviewed changes

Comment thread internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD1Benchmark.java

dougqh commented Sep 11, 2026

View reviewed changes

Comment thread internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java Outdated

dougqh and others added 3 commits September 11, 2026 10:13

Prefix hashIterable call with ConcurrentHashtable in LogCollector.find

b632878

Review feedback: the bare static-imported call read ambiguously at the call site; qualifying it makes clear which table API is in play. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

dougqh commented Sep 11, 2026

View reviewed changes

dougqh marked this pull request as ready for review September 11, 2026 16:26

dougqh requested a review from a team as a code owner September 11, 2026 16:26

dougqh requested review from jpbempel and removed request for a team September 11, 2026 16:26

dd-octo-sts Bot added the tag: ai generated Largely based on code generated by an AI or LLM label Sep 11, 2026

dougqh requested review from amarziali and bric3 September 11, 2026 16:27

datadog-datadog-prod-us1 Bot reviewed Sep 11, 2026

View reviewed changes

Comment thread internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java Outdated

chatgpt-codex-connector Bot reviewed Sep 11, 2026

View reviewed changes

Comment thread internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java Outdated

Comment thread internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java Outdated

Comment thread internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java

dougqh and others added 4 commits September 11, 2026 12:56

Uh oh!

Conversation

dougqh commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Does This Do

Motivation

Additional Notes

Contributor Checklist

Uh oh!

This comment has been minimized.

dd-octo-sts Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🟢 Java Benchmark SLOs — All performance SLOs passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougqh Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougqh Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dougqh Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougqh Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougqh Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougqh Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dd-octo-sts Bot commented Sep 11, 2026

Uh oh!

datadog-datadog-prod-us1 Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

dougqh commented Sep 11, 2026 •

edited

Loading

dd-octo-sts Bot commented Sep 11, 2026 •

edited

Loading

dougqh Sep 11, 2026 •

edited

Loading

dougqh Sep 11, 2026 •

edited

Loading

dougqh Sep 11, 2026 •

edited

Loading

dougqh Sep 11, 2026 •

edited

Loading

dougqh Sep 11, 2026 •

edited

Loading

dougqh Sep 11, 2026 •

edited

Loading