Add ConcurrentHashtable (perf toolbox) - #11675
Conversation
…y tables Mirrors Hashtable's D1/D2 API with concurrent access guarantees: lock-free get via AtomicReferenceArray volatile reads, synchronized getOrCreate with double-checked re-read on miss. Eliminates composite key object allocation on hot read paths — the same structural advantage Hashtable.D2 has over HashMap<Pair<K1,K2>,V>, but thread-safe. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t class; add D2 benchmark Extract bucketIndex and forEach into ConcurrentHashtable.Support, mirroring the Hashtable.Support pattern. Add ConcurrentHashtableD2Benchmark comparing get and getOrCreate throughput against ConcurrentHashMap and ConcurrentSkipListMap. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
🟢 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. |
… ConcurrentHashtable Two gaps filled per-dimension (D1 and D2): - Chain collision: force multiple entries into the same bucket (CollidingKey with fixed hashCode for D1; pigeonhole via 2-bucket table for D2) and verify all entries are reachable after concurrent inserts. - Concurrent distinct keys: 16 threads each insert a unique key simultaneously, verifying final size and that every key is retrievable — exercises concurrent inserts to different buckets, which the single-shared-key test does not cover. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…htable Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ThreadSafeCounterBenchmarks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…enchmark Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ectly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…adSafeMapD2Benchmark Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaced by the ThreadSafeMap{D1,D2,Counter}Benchmark split.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Give ConcurrentHashtable its own entry hierarchy (Entry / D1.Entry / D2.Entry)
with a volatile next pointer, independent of the single-threaded Hashtable. The
volatile chain pointer lets a chain splice under the write lock be observed by
lock-free readers, which makes removal safe:
- remove(key) unlink a single entry
- removeIf(predicate) sweep the whole table under one lock
- drain(sink) read-and-reset: remove every entry, handing each to a
caller-supplied accumulator (Consumer + context-passing
BiConsumer overload) -- the flush/publish primitive
- clear() empty the table
Removed entries keep their own next pointer intact so an in-flight reader can
still traverse forward. Migrates the ThreadSafeMap* benchmarks to the new
entry base. Adds single-threaded and concurrent removal tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Lookups reuse the interned KEYS/SOURCE_* instances used to populate the table, so they exercise the == identity fast path — deliberate and realistic for the tracer (keys are typically interned tag-name constants), not an oversight. Clarifies so it isn't misread against the equals()-path numbers elsewhere. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… shape Flatten the nested Support class onto the ConcurrentHashtable namespace (static fns over a caller-owned AtomicReferenceArray, mirroring FlatHashtable) and type the bucket arrays AtomicReferenceArray<TEntry> so the unchecked casts on the bucket read paths disappear. - createFixedBuckets(entryClass, capacity) factories on ConcurrentHashtable (returns the raw spine), D1, and D2 (return a D1/D2); D1(int)/D2(int) ctors are now private. entryClass is a symmetry + type-inference anchor here (the AtomicReferenceArray spine is erased, so it isn't consumed for allocation the way FlatHashtable's E[] is). - key()/key1()/key2() accessors on D1.Entry/D2.Entry to match Hashtable post-#12044. - Context-passing forEach/drain overloads use <C> for the context type param. - Double-checked-locking + lock-striping recipes moved to the class Javadoc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Move D1/D2 tests and the ThreadSafeMap{Counter,D1,D2} benchmarks off the
removed public ctors / Support class onto createFixedBuckets and the flattened
ConcurrentHashtable.* static fns. The D2 benchmark's raw-array custom-entry arm
now drives a typed AtomicReferenceArray<SupportEntry>.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5829238fde
ℹ️ 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".
The bucket AtomicReferenceArray is the per-table write monitor, obtained via getWriteLock(buckets) (opaque accessor, single source of truth) so callers never hardcode what to synchronize on. Reads (bucket/forEach) stay lock-free; whole-table mutators (removeIf/drain/clear) self-lock; the single-slot write primitives (insertHeadEntry/unlink) are caller-locked and assert Thread.holdsLock(getWriteLock(buckets)) under -ea. insertHeadEntry mirrors Hashtable's insert helper so custom tables publish entries without touching the chain pointer directly; Entry.setNext is demoted to package-private accordingly while next() stays public for lock-free chain walks. Adapts ThreadSafeMapD2Benchmark call sites to the new API. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…htable # Conflicts: # internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapBenchmark.java
Mirrors the same guard added to Hashtable.insertHeadEntryAt. Here it also catches reinserting an already-unlinked entry: unlink() deliberately leaves next intact so in-flight lock-free readers can keep traversing, so overwriting it via a reinsert would corrupt that traversal.
Bundles buckets + a cursor-based SizeManager into a State<TEntry>, threaded through D1/D2 as tryGetOrCreateOrEvict(OrNull) so callers can cap table size and evict on overflow. Renames createFixedBuckets -> createCapped and getOrCreate -> tryGetOrCreate(OrNull) to reflect the capacity-aware contract. Adds unit test coverage for SizeManager's reserve/evict/reset behavior and the D1/D2 eviction paths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Mirrors Hashtable.insertReserved: splices a fully-built entry into an already-reserved slot (from tryReserve()/tryReserveOrEvict) without double-counting. Not used by D1/D2, whose creator is fallible and so increments only after a successful link; documented as the contrast. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
tryReserveOrEvict is self-locking, so pairing it with insertReserved across two critical sections lets a drain or clear land in the gap, reset the SizeManager while the reservation is outstanding, and leave the insert linking an entry the count never learns about -- a capped table then drifts silently past its cap. Document the enclosing lock as part of the contract (class level, both tryReserveOrEvict javadocs, and insertReserved's example), fix the test that encoded the racy shape, and add a deterministic test that a concurrent clear cannot interleave. Also give evictOneInRange the @GuardedBy the other cursor writers carry, and suppress AT_STALE_THREAD_WRITE_OF_PRIMITIVE where SpotBugs cannot model the dynamic getWriteLock(buckets) guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
getWriteLock(buckets) had exactly one answer, so every caller asked for the whole table whether or not that was what it needed. That makes the locking granularity part of the API: a striped implementation would have no object to return. Replace it with three accessors that name a scope -- getWriteLock(state, keyHash), getWriteLockAt(state, bucketIndex), and getTableWriteLock(state) -- and point every @GuardedBy, assert, and call site at the one it actually needs. All three still return the same monitor, so behavior is unchanged; only the question each caller asks is different. Two consequences worth having: a caller that holds one key's monitor and mutates another is now visibly wrong rather than accidentally right, and every getTableWriteLock use marks a spot where striping would cost something. The class javadoc records what those spots are -- table-wide capacity accounting and a whole-table eviction scan -- so the analysis does not have to be redone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SizeManager treated the entry cap as strict, and paid for it twice: a check-then-increment reserve that needed the table write lock, and drain and clear zeroing the count so a concurrent reservation was silently discarded. The second of those was the P1: an undercount no later eviction repairs, since eviction decrements too. An approximate cap is fine here -- the bucket array is fixed-size with load-factor headroom and never rehashes, so overshoot lengthens chains and nothing else. Taking that latitude turns out to buy exactness where it is cheap and delete the locking where it is not: - tryReserve claims a slot and refunds on overshoot. Atomic on its own, so it needs no lock, and concurrent reservers still cannot both pass the cap. - drain and clear subtract what they actually removed (release(int), replacing reset()), so a reservation survives a sweep landing in the gap between reserve and insert. That removes the reason reserve-then-insert had to share one critical section, so insertReserved now documents the single-bucket lock instead. What remains lock-dependent is D1/D2's isFull-then-increment ordering, which exists so a fallible creator cannot leak a slot and can tolerate admitting slightly over the cap. Counting makes clear O(entries) rather than O(buckets); clear is a rare whole-table operation, so an honest count is worth the walk. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bric3
left a comment
There was a problem hiding this comment.
The code looks good to me, assuming I got the concurrent part well.
However, aong the thing that I noticed some lower level APIs, have a public API in the ConcurrentHashtable "namespace", which I belive confuses its basic usage. Maybe using a Support class like elsewhere could help distinguish public API, versus lower level APIs. That could come as a follow-up.
Additionaly, I wonder if tests on mixed clear/drain/eviction invocations could still be useful here.
I think the javadoc could have some hman refinement, but nothing blocking.
bric3
left a comment
There was a problem hiding this comment.
Suggested reworded comments without the verbiage of claude's inference.
… comment Condenses the class/method-level Javadoc across ConcurrentHashtable and the ThreadSafeMap* benchmarks down to the load-bearing points, and reworks the isFull()-before-creator comment in D1/D2.tryGetOrCreateOrNull to spell out the leaked-reservation failure mode instead of a terse arrow-notation summary (per bric3's PR review nitpick). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
bric3
left a comment
There was a problem hiding this comment.
OK to move forward with this. But I'm not convinced by the exposure of lower level API on the "main" class. I understand it has to be done in some way though to achieve the multi key mechanism.
@bric3 I understand. As I said, I've gone back and forth myself. I think I like this better than a Support class, but it is a close call. |
Inlines the power-of-two rounding and its MAX_BUCKETS cap directly into ConcurrentHashtable instead of delegating to Hashtable.Support.sizeFor, which is being removed as part of the Hashtable/ConcurrentHashtable API unification. Some duplication with Hashtable.sizeFor is accepted in exchange for removing the cross-PR coupling.
…vadoc Hashtable and FlatHashtable both carry a "Choosing between the three tables" section; ConcurrentHashtable lacked the reciprocal reference.
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What Does This Do?
Adds a concurrent version of Hashtable. ConcurrentHashtable (like Hashtable) is parameterized on its entry type.
Motivation
Parameterizing on entry type allows the dd-trace-java Hashtable-s to excel in use cases where regular Map-s don't fit or have high overhead including...
Additional Notes
ConcurrentHashtablewithD1(single-key) andD2(composite-key) inner classes, mirroring theHashtableAPI with concurrent access guaranteesget/getOrCreatefast path viaAtomicReferenceArrayvolatile bucket reads plus avolatilechain-nextpointer; synchronized only on miss, with a double-checked re-read under the locksizeFor,bucketIndex,bucket,unlink,removeIf,drain,clear,forEach) arepublic staticbuilding blocks over a caller-ownedAtomicReferenceArray— the same "static functions over a caller-owned array" shape asHashtable(see howAggregateTableusesHashtable). Callers that need primitive/higher-arity keys or their own lock strategy subclassEntrydirectly and drive the table with these;D1/D2are the batteries-included wrappers over that spinecreateFixedBuckets(Class<TEntry> entryClass, int capacity)on all three:ConcurrentHashtable.createFixedBucketshands back the rawAtomicReferenceArray<TEntry>for the caller-owned path, whileD1/D2.createFixedBucketsreturn aD1/D2instance. The entry class anchorsK/K1/K2/TEntryinference and keeps the family symmetric withHashtable/FlatHashtable— though theAtomicReferenceArrayspine is type-erased, so (unlikeFlatHashtable) the class isn't consumed for allocationD2.get(K1, K2)andD2.getOrCreate(K1, K2, creator)accept key parts directly — no composite key object allocated for the lookup, unlikeConcurrentHashMap<Pair<K1,K2>, V>where EA must conservatively treat the key as escaping even on hits (ownership-transfer contract)ConcurrentHashtableD1TestandConcurrentHashtableD2Test(JUnit 5), including a concurrency correctness test verifying exactly one entry is created under 16 racing threads@Threads(8), all threads hitting a shared table):ThreadSafeMapD1Benchmark(D1vsConcurrentHashMapvsConcurrentSkipListMap),ThreadSafeMapD2Benchmark(D2and a raw caller-owned-array arm vsConcurrentHashMapwith aKey2wrapper vsConcurrentSkipListMap), andThreadSafeMapCounterBenchmark(D1+AtomicLongFieldUpdaterinline counter vsConcurrentHashMap+AtomicLong/LongAdder)Test plan
./gradlew :internal-api:test --tests "datadog.trace.util.ConcurrentHashtable*"— all tests pass./gradlew :internal-api:jmhCompileGeneratedClasses— benchmarks compile cleanThreadSafeMap*benchmarks locally to validate get/getOrCreate throughput advantage over CHM and CSLM🤖 Generated with Claude Code