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

dougqh · 2026-09-10T21:00:03Z

What Does This Do

Builds on #12453's cleanup of ConcurrentHashtable with a higher-level find-or-insert API, and ports LogCollector onto it as a worked example of the resulting ergonomics.

Entry<TEntry extends Entry<TEntry>> (self-bounded, à la Enum<E extends Enum<E>>) forces every concrete entry to implement matches(TEntry), replacing ad-hoc reuse of equals()/hashCode() with a comparison method scoped to what the hashtable actually needs.
ConcurrentHashtable.reserve(state) returns an AutoCloseable Reservation<TEntry> that claims a slot lock-free, defers building the entry until the reservation actually succeeds, and auto-cancels an unclaimed slot on close() if nothing was inserted (e.g. a concurrent write already inserted the same logical entry).
Reservation#tryGetOrInsertOrNull is overloaded from 1 to 4 key components (Function through a new Function4), so callers can pass a non-capturing constructor reference instead of allocating a capturing lambda per call.
SizeManager.cancelReservation() is the new lock-free primitive backing Reservation#close() (symmetric with the existing decrement()).

LogCollector is ported onto this API illustratively — its hand-rolled ConcurrentHashMap<RawLogMessage, AtomicInteger> dedup logic collapses into a lock-free scan plus a single try (Reservation<...> r = ConcurrentHashtable.reserve(...)) { r.tryGetOrInsertOrNull(RawLogMessage::new, ...) }.

Motivation

ConcurrentHashtable is a new toolbox-style primitive with no adopters yet. Porting LogCollector onto it dogfoods the API from inside Java LP before it's offered to other product areas, surfacing ergonomic gaps (like the reserve/insert dance) while the cost of changing the API is still low.

Additional Notes

:internal-api:compileJava, :internal-api:compileTestJava, :internal-api:compileTestGroovy, :internal-api:compileJmhJava all pass
:internal-api:test --tests "datadog.trace.util.ConcurrentHashtable*" — all existing suites plus the new ConcurrentHashtableReservationTest pass
:internal-api:test --tests "datadog.trace.api.telemetry.LogCollectorTest" passes against the ported implementation
./gradlew :internal-api:spotlessApply — no formatting changes needed

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 (n/a — no new top-level files/paths)
Update public documentation with any new configuration flags or behaviors (n/a — no new config flags)
Once approved, use merge queue to merge the PR

🤖 Generated with Claude Code

State.sizeManager was a public field, letting callers outside this class reach into SizeManager directly instead of going through the State-accepting static helpers. Narrow it to package-private and add the missing tryReserve(State) wrapper so external callers (e.g. an upcoming ConcurrentHashtable consumer) have a sanctioned lock-free reservation entry point instead of needing sizeManager exposed. Also move createBounded from the nested State class onto ConcurrentHashtable directly for a nicer call site, per review discussion on PR #12367. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…to ConcurrentHashtable Entry<TEntry extends Entry<TEntry>> now forces every concrete entry to implement matches(TEntry), replacing ad-hoc equals()/hashCode() reuse. ConcurrentHashtable.reserve(state) returns an AutoCloseable Reservation that claims a slot lock-free, defers building the entry until the reservation succeeds, and auto-cancels an unconsumed slot on close() - collapsing the racy hand-rolled dedup pattern LogCollector used to need into a single try-with-resources call. Overloaded up to 4 key components (Function..Function4) so callers can pass a non-capturing constructor reference instead of a capturing lambda. LogCollector is ported onto this API illustratively, to see the resulting ergonomics against a real caller. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…k-trace re-allocation in LogCollector Reverts D1/D2 back to the self-bounded Entry<K, TEntry extends Entry<K, TEntry>> shape per PR review, so entry payloads live in the subclass rather than a boxed value field. Adds ConcurrentHashtable.hashIterator as a public, reusable bucket-chain iterator, replacing LogCollector's private bucketIterator copy. Also stops matchesKey from calling the incoming throwable's getStackTrace() (a defensive-copy allocation) on every candidate scanned in addLogMessage's bucket chain -- it's now memoized once per call. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

hashIterator's bucket chain can contain entries whose keyHash differs (hash collisions on bucketIndex), so it now skips those internally rather than making every caller repeat curEntry.keyHash == keyHash -- consistent with how D1/D2's own lookups already pre-filter by keyHash before calling matches(). Drops the now-redundant keyHash check from LogCollector.addLogMessage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…loop var Reservation.tryGetOrInsertOrNull/tryGetOrInsert took their Strategy factory first, ahead of the key components -- inconsistent with D1/D2's own tryGetOrCreate(key, creator) convention of putting the functional-interface parameter last. Also adds hashIterable, a thin Iterable wrapper around hashIterator so callers can write a plain for-each loop, and renames LogCollector's loop variable from `it` to `iter`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

setNext/next used their own generic type parameter with an unchecked cast, but every caller already assigns the result to a TEntry-typed variable -- the field can just be declared as TEntry directly, since a bucket chain's entries always share the same concrete TEntry as the array that holds them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…to isReserved tryGetOrInsertOrNull(TEntry)/tryGetOrInsert(TEntry) expose finish() directly for callers with more than 4 key components or that want to avoid boxing primitives into a Function's type argument. isPresent() read like Optional.isPresent() (an entry already exists), but it actually means the reservation succeeded / the table had room -- true even before any entry is created. isReserved() matches that meaning. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

D1.Entry<K, TEntry> and D2.Entry<K1, K2, TEntry> imposed the same self-bound generic pattern as the base ConcurrentHashtable.Entry, but D1/D2 only need it to implement matches() -- the concrete subclass never needs to flow through to callers of D1/D2's own generic surface. D1.Entry<K> and D2.Entry<K1, K2> now extend ConcurrentHashtable.Entry<Entry<K>> (their own base type) directly, so subclasses are declared as e.g. `class MyEntry extends D1.Entry<String>` rather than `D1.Entry<String, MyEntry>`. D1/D2 store and link entries internally as that base Entry type and cast back to TEntry at their public API boundary -- sound because the table only ever holds instances its own caller-supplied creator produced. matches(Entry) is now final since subclasses only need to implement the key-based matches(Object) overload. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Now that D2.Entry no longer requires a self-type parameter, D2Entry doesn't need the extra value constructor param or lambda call sites that were introduced when self-bound generics were restored. Revert to the original D2Entry class and method references to keep the diff minimal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

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

Superseded by #12462, which rebuilds this API on top of the merged LogCollector change (#12367) for a cleaner diff. Closing in favor of that.

dougqh · 2026-09-11T12:25:22Z

+
+    // Lock-free scan first: most calls are re-observations of an already-seen message, so this
+    // avoids paying for a reservation and a RawLogMessage allocation on the common path.
+    for (RawLogMessage existing : ConcurrentHashtable.hashIterable(rawLogMessages, keyHash)) {


I fixed an oversight in hashIterator. It was suppose to filter down just to matching hashes like the one in Hashtable, but that got lost in the translation to ConcurrentHashtable.

Then I added hashIterable just to make the for-loop a little nicer still. The Iterators are trivial, so they should play nice with escape analysis. I'll probably add some benchmarks to prove that.

dougqh and others added 2 commits September 10, 2026 14:45

dougqh added type: feature Enhancements and improvements comp: telemetry Telemetry tag: ai generated Largely based on code generated by an AI or LLM labels Sep 10, 2026

dougqh commented Sep 10, 2026

View reviewed changes

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