Use ConcurrentHashtable for telemetry log deduplication - #12367
Draft
bric3 wants to merge 44 commits into
Draft
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>
… 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>
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>
matches() now invokes equals() on the lookup parameter rather than the stored field (D1: key, D2: key1/key2). When matches() inlines into get/getOrCreate the caller's key type is known, so the JIT can devirtualize the equals() call; Objects.equals still short-circuits on == first, so interned keys keep the identity fast path. ThreadSafeMapD2Benchmark's Key2 dropped Objects.hash(...) — its varargs Object[] allocation penalized the map baselines with an alloc the wrapper itself doesn't need, overstating the ConcurrentHashtable advantage the benchmark measures. Uses a plain 31*h1 + h2 hash instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The D1/D2 wrappers were well covered but the caller-owned-array path — the static building blocks that back custom tables (primitive/higher-arity keys) — had no direct tests. Adds ConcurrentHashtableStaticsTest, which drives a hand-written primitive-int-key table (IntTable) through the documented lock-free-read / locked-write recipe. Covers sizeFor, createFixedBuckets, getWriteLock, bucketIndex, both bucket and insertHeadEntry overloads, unlink (head/middle/tail), and the static removeIf/drain/drain-with-context/clear/forEach primitives. Also asserts the Thread.holdsLock guards on insertHeadEntry/unlink fire when called without the write lock (guarded by an -ea check), plus exactly-once and lock-free reader-safety races driven entirely through the statics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codex flagged that if a drain sink throws part-way, already-detached entries are gone while size() still reports the pre-drain count. Rather than add per-entry size bookkeeping to a path that only matters when the caller is already in error (a throwing sink is a half-published flush with no rollback), document that the sink must not throw — on the D1/D2 drain wrappers and the static drain primitive. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…GuardedBy D1 and D2 are thread-safe (lock-free reads, locked writes), so mark them @threadsafe at the type level. The hand-written mutating building blocks insertHeadEntry and unlink require the caller to hold the table write monitor (they already assert Thread.holdsLock(getWriteLock(buckets))); make that precondition static/tooling-visible with @GuardedBy("getWriteLock(buckets)"). Deliberately leave the final, individually-thread-safe buckets/size fields unannotated: reads are lock-free by design, so @GuardedBy there would misdescribe the contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
An int-typed key hash calling the overloaded bucket(buckets, hash) or insertHeadEntry(buckets, hash, entry) binds to the int-index overload instead of widening to long, treating the raw hash as an array index. Split into distinct bucketAt/insertHeadEntryAt (index-based) and bucketFor/insertHeadEntryFor (hash-based) so there's no overload to mis-resolve. Co-Authored-By: Claude Sonnet 5 <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>
… 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>
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.
bric3
added
type: feature
Contributor
|
🎯 Code Coverage (details) 🔗 Commit SHA: 1367128 | Docs | View more details | Give us feedback! |
Contributor
🟢 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. |
Contributor
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This Do
Replaces LogCollector's ConcurrentHashMap dedup storage with a bounded ConcurrentHashtable entry that carries its own atomic count. Duplicate lookups no longer construct RawLogMessage.
Migrates the direct tests to JUnit 5 and strengthens concurrency, capacity, and drain coverage.
Motivation
Telemetry logging can be reached repeatedly from error paths. The previous duplicate-hit path allocated a RawLogMessage on every call and retained a separate AtomicInteger per distinct message.
Additional Notes
Stacked on #11675.
The eight-thread duplicate-hit benchmark reduced normalized allocation from 48.000 B/op to approximately 0 B/op (about 1e-4 B/op measured).
Contributor Checklist