Unify Hashtable static API with ConcurrentHashtable; deprecate Support - #12101
Unify Hashtable static API with ConcurrentHashtable; deprecate Support#12101dougqh wants to merge 30 commits into
Conversation
dougqh
added
comp: core
|
@codex review |
|
🎯 Code Coverage (details) 🔗 Commit SHA: d813ca0 | Docs | View more details | Give us feedback! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
| */ | ||
| @Deprecated | ||
| public static final class Support { | ||
| private Support() {} |
There was a problem hiding this comment.
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 👍 / 👎.
| * 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; |
There was a problem hiding this comment.
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 👍 / 👎.
Move the static building blocks off the nested Support class onto Hashtable itself, mirroring ConcurrentHashtable's flat layout, and add createFixedBuckets(Class, int) factories on Hashtable/D1/D2 for family symmetry. Support becomes a thin @deprecated facade delegating to the new statics (retaining the scaled create(int, float)/MAX_RATIO helpers, which have no blessed equivalent), so client-side-statistics callers keep compiling untouched. Rename the context type parameter <T> -> <C> on the context-passing forEach overloads, and add D2.Entry.key1()/key2() accessors to match D1/the concurrent variant. No behavior change; pure API relocation + deprecation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Point the tests at the relocated static building blocks on Hashtable (createFixedBuckets, sizeFor, bucketIndex, clear, insertHeadEntry, and the iterator factories) instead of the now-deprecated Support facade. Keep a small DeprecatedSupportTests group covering the deprecated-only scaled create(int, float) + MAX_RATIO, which have no blessed equivalent and remain in use by client-side statistics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mirrors the ConcurrentHashtable fix: an int-typed key hash calling the overloaded 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 insertHeadEntryAt (index-based) and insertHeadEntryFor (hash-based). Also renames bucket to bucketFor for consistency with ConcurrentHashtable's naming, even though Hashtable has no competing int-index overload of bucket today. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Capacity is now enforced, not just used to size the bucket array: insert() returns false, getOrCreate() returns null, and insertOrReplace() throws once size() reaches the constructor capacity. A lookup hit is still always returned even at capacity -- only new entries are blocked. Callers wanting their own eviction policy can drop to Hashtable.Support directly.
getOrCreate() can now return null once TAG_CAPACITY distinct tags are blocked in a window; record() must null-check it rather than relying on the table's old unbounded-chaining behavior.
Composers driving the static building blocks directly (e.g. client-side stats' AggregateTable) currently hand-roll entry-count bookkeeping and cursor-resumed eviction scans themselves. These give them (and D1/D2, next) a shared, non-thread-safe primitive for both instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replaces the hand-rolled size/limit int fields with the new shared SizeTracker -- no behavior change, D1/D2's public API and semantics are unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
43fdfe2 to
c04c3de
Compare
Adds unconditional drain (forEach-then-clear-and-reset-size in one call, plus a context-passing overload) as a static building block on Hashtable and as instance methods on D1/D2, mirroring ConcurrentHashtable's drain(Consumer)/drain(context, BiConsumer). The single-threaded version needs no locking, just a size-tracker reset. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Delegates to the internal SizeTracker so callers can check capacity before calling insert/getOrCreate/insertOrReplace, instead of inferring it from a false/null/thrown result after the fact. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🟢 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. |
Both methods are annotated @nonnull but return null once the table is at capacity and the key is absent -- which their own javadoc documents. The annotation contradicted the contract, on the exact path a capped table takes under pressure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"Fixed" meant two contradictory things across the *Hashtable family: FlatHashtable.createFixed and Hashtable.D1 both cap entries and refuse past the cap, while ConcurrentHashtable's fixed tables have no cap at all and never refuse. Rename the capping factories to say what they promise rather than how they are built, so a caller reading the name gets a straight answer to "will this refuse my insert?". D1/D2.createFixed -> createCapped(entryClass, maxCapacity) Hashtable.createTable -> createCappedTable(maxCapacity) Table factories now always take a number of entries; only the low-level allocator takes buckets. Sizing a table from a bucket count is the HashMap(initialCapacity) footgun, and the load factor differs per class, so callers should never need to know it: Hashtable.create(int buckets) / create(Class, int buckets) -- low level Hashtable.capacityFor(cardinalityLimit[, loadFactor]) -- the bridge Hashtable.DEFAULT_LOAD_FACTOR -- 0.75, chained D1/D2 constructors become private so the factory always carries the posture choice, which also leaves room for a growable variant later without a second rename. The deprecated Support facade is inverted onto the blessed statics: the new untyped create(int) gives create(int)/create(int, float)/MAX_RATIO a real home, three inline `new Hashtable.Entry[...]` sites route through it, and the iterators stop calling Support.bucketIndex. Support now holds no logic and can be deleted outright once client-side stats migrates. FlatHashtable is unchanged -- it already used this shape, and keeps fixed/growable because for open addressing growth is a correctness requirement rather than a performance choice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
remove() delegated to removeMatching with `e -> e.matches(key)`, which captures `key` and so allocates a fresh Predicate on every call -- LambdaMetafactory can only cache non-capturing lambdas. insertOrReplace sits directly below it and walks the same chain with no lambda at all. Escape analysis often erases this, and remove() has no production caller today, so the argument is consistency rather than measured throughput: this class ships context-passing forEach/drain overloads specifically so callers can avoid capturing lambdas, and then captured one itself. removeMatching stays as a building block for composers that match on something other than the key, so it and the size-tracked insertHeadEntryFor get direct tests now that D1/D2 no longer cover them by delegation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Parameter order for the size-tracked static building blocks is now: mutated bookkeeping, then the spine, then the key, then callbacks. insertHeadEntryFor(sizeTracker, buckets, keyHash, entry) removeMatching(sizeTracker, buckets, keyHash, matches) Appending the tracker made the tracked and untracked forms differ only in a trailing argument, which is the wrong shape for a distinction that fails silently: a missed increment refuses inserts early and gets noticed, while a missed decrement leaks the cap until the table stops accepting anything. Leading with it puts the difference at the head of the call, where it is visible while reading and greppable in review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing outside Support mentions it now: the class javadoc no longer advertises the facade, D1's "roll your own eviction" pointer aims at createCappedTable/SizeTracker/EvictionCursor instead, and the historical note on the static-building-block section is gone. Support keeps its own @deprecated pointers saying what replaced each member -- that direction is the useful one. Since no production code references the facade any more, it can be deleted outright once client-side stats migrates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The nullable return was buried in the third paragraph, behind the hash-reuse and creator-contract notes, while the method name reads as total. That ordering is how the @nonnull annotation got there in the first place. Both D1 and D2 now state up front that a create can be refused at capacity, that a hit is still always returned, and that isFull() answers the question ahead of time. Also notes that refusal is a designed steady state for a capped table rather than an exceptional one, so callers should decide deliberately what a refused create does instead of letting the null fall through. Keeping the name getOrCreate rather than tryGetOrCreate: the posture is per-instance (capped vs uncapped) while the method name is per-class, so a try- prefix would over-promise failure on an uncapped table exactly as the current name under-promises it on a capped one. FlatHashtable already made this call explicitly -- the factory name carries the posture -- and ConcurrentHashtable's getOrCreate is correctly @nonnull because it never refuses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The name read as total while the method refuses at capacity, and the idiomatic two-liner everyone writes -- getOrCreate(key, ctor) then mutate the result -- NPEs the first time the cap is reached. That ordering is also how the @nonnull annotation got onto it originally. The prefix marks "this may refuse", matching SizeTracker.tryReserve in the same class. A growable FlatHashtable never exercises it, and that is deliberate: the posture is chosen per instance at the factory while the method name is per class, and the two mistakes are not symmetric -- under-promising refusal costs an NPE at the cap, over-promising costs a redundant null check. Better to over-warn. ConcurrentHashtable keeps the plain getOrCreate for now: it is uncapped, so the name is honest there. It renames when it gains a cap. Also fixes a FlatHashtable javadoc claim that Hashtable.D1's factory counts buckets -- it counts entries, as every table factory in the family now does; only the low-level array allocators take bucket counts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
Two doc-only changes; no behavior change.
Stop naming other classes. Hashtable referenced ConcurrentHashtable
(three of them {@link}s to a class that is not in this tree, so dangling)
and AggregateTable, a downstream consumer in dd-trace-core -- an inverted
dependency for a low-level util to document. FlatHashtable comparisons
went too: the three are related in design, not in any dependency sense,
and a reader of this class should not need the other two loaded to
understand it. The reasoning those references carried is kept, just
stated on its own terms -- why bucketFor is not called bucket, why
insertHeadEntryAt/For are not one overloaded name, why a chained table
can run a higher load factor than an open-addressed one.
Order members by expected use, leading with creation. D1, D2 and the
static building blocks now all read: create, then access (get / insert /
tryGetOrCreate / forEach), then the bulk clear / drain / eviction
routines, then supporting types. Previously the iterator factories sat
after drain, and clear came before the traversal methods.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reserving a slot and evicting to make room are two directions of the same
policy, so they belong on one object. Keeping them apart meant a caller
had to wire a cursor to a tracker, then remember to decrement after every
unlink -- a missed decrement leaks the cap silently until the table stops
accepting anything.
Folded, that whole class of mistake disappears: there is no second object
to mis-wire, and every eviction maintains the count because the count is
right there. It also lets the two halves compose into the call a
self-evicting table's miss path actually wants:
if (!sizeManager.tryReserveOrEvict(buckets, STALE)) {
return null; // full and nothing evictable
}
replacing an isFull() check followed by a hand-rolled evict-and-retry.
Also renames the cursor's full-pass drain to evictAll, so it no longer
collides with Hashtable.drain -- one removes what matches and returns a
count, the other empties the table into a sink. And adds the tracked
clear(sizeManager, buckets), which resets the count along with the
spine; D1/D2.clear now use it instead of pairing the two calls by hand.
Table drops to buckets + sizeManager.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Table said what it was made of, not what it is for, and it competed with D1/D2 -- which are also tables. State says the honest thing: both halves are mutable, the spine holds the entries and the SizeManager holds how many there are and where the last eviction looked. Its javadoc previously told callers to unpack it into their own fields and not retain it. That was backwards. An array and a manager stored separately can drift apart, which is exactly what this type exists to prevent, so holding the pair is now the documented usage. createCappedTable becomes createCapped, matching D1/D2.createCapped and sitting alongside the raw create(int buckets) -- create allocates an array by bucket count, createCapped builds capped state from an entry count, consistent with the entries-vs-buckets split elsewhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
State is parameterized on its entry type, so the statics that need both the spine and its manager take one argument instead of two: insertHeadEntryFor(state, keyHash, entry) removeMatching(state, keyHash, matches) clear(state) tryReserveOrEvict(state, evictable) evictOne(state, evictable) / evictAll(state, evictable) Two things this buys beyond brevity. A manager belonging to a different table is no longer passable -- the pairing is structural rather than a convention the caller upholds. And TEntry now has somewhere to be inferred from, which removes both warts the client-side-stats migration hit: the explicit Hashtable.<StringIntEntry>removeMatching witness, and the cast inside eviction predicates, which are now typed to the entry. Eviction stays static rather than moving onto State, even though State is the thing holding the cursor underneath. Composition through static functions over caller-owned data is the shape of this class, and keeping it means a caller gets the cursor-resumed scan -- and its amortization across a sustained eviction stream -- without knowing a cursor exists. State itself stays pure data: two final fields, no behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
From reviewing the first real consumer (#12312), where each of these was either reaching into state.buckets or reaching into state.sizeManager to do something the API should have offered directly: size(state) / isEmpty(state) bucketFor(state, keyHash) -- typed, so the chain walk needs no witness forEach(state, consumer) -- and the context-passing overload Also adds insertReserved(state, keyHash, entry), which links an entry without touching the count because the caller already holds a reservation. That is the other half of tryReserveOrEvict, and it is deliberately a different name from insertHeadEntryFor(State, ...) -- that one reserves as it inserts, so using it after a reservation would count the entry twice. Splitting them keeps the refuse-before-you- allocate shape available: reserve, and only build the entry once the slot is yours. 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>
|
@codex review |
There was a problem hiding this comment.
💡 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".
| Hashtable.<TEntry>forEach(buckets, sink); | ||
| clear(buckets); |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
| count++; | ||
| } | ||
| } | ||
| this.size -= count; |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
| public static <TEntry extends Entry> void drain( | ||
| @Nonnull Hashtable.Entry[] buckets, @Nonnull Consumer<? super TEntry> sink) { | ||
| Hashtable.<TEntry>forEach(buckets, sink); | ||
| clear(buckets); |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
From /techdebt and /perf-review over the branch. drain was the one size-tracked pair still left to the caller. clear gained a (sizeManager, buckets) form whose javadoc says emptying without resetting "leaves the cap permanently consumed, so the two belong in one call" -- and then D1.drain and D2.drain did exactly that pair by hand, and a composer calling the public static against a State would have leaked the cap silently. Adds drain(sizeManager, buckets, sink), the context-passing form, and both State overloads; D1/D2 route through them. Also repairs a comment in CaseInsensitiveMapBenchmark that a rename reflow had mangled mid-sentence, and records why the D1/D2 benchmarks use @setup(Level.Iteration) rather than Trial -- the setup rebuilds the table and the HashMap, so iterations must not inherit mutated counters; the pollution call merely rides along. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eviction only advanced the cursor on a successful match, so a table that
was full and entirely hot retried from the same origin on every miss,
re-testing the same entries in the same order. It now steps on regardless.
That does not shrink the per-attempt cost -- a scan that matches nothing
has by definition tested every live entry -- so the javadoc now says so.
It previously advertised only the amortized success case ("N evictions
never re-scan the hot prefix more than twice"), which is true of
successes and quietly untrue of refusals. Callers get told to size the
cap to the steady-state working set and keep the predicate cheap, since
it runs once per live entry on every refusal.
Renames the count to match what it can promise. SizeManager.estimateSize
is an estimate because reservations are counted the moment they are
taken: between reserving and linking it reads one high, and
insertReserved trusts the caller, so a link without a reservation reads
low. Hashtable.isEmpty becomes isLikelyEmpty for the same reason -- fine
for skipping work that would be wasted on an empty table, not for
establishing that the table is empty.
D1/D2 keep an exact size(): they reserve and link inside one call, so the
window is never observable from outside.
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>
evictAll subtracted its running count after the loop, so a predicate that threw part way through left the already-unlinked entries gone from the chains while the count kept counting them -- permanently high, which in a capped table means it eventually stops accepting anything. Now decrements per removal, matching evictOne. drain handed entries to the sink with their `next` links intact, since it was forEach followed by Arrays.fill. A sink that retained one entry of a chain pinned every entry behind it, including ones 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, because the sink may do anything with the entry once it has it. That also drops the second pass. Both come with regression tests, verified to fail against the pre-fix code: one drives evictAll with a throwing predicate and asserts the count matches the spine, the other drains a forced collision chain and asserts the drained entries are detached. 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>
|
@arp review |
Two questions, at the top of each class, written from that class's side:
1. Concurrent? -> ConcurrentHashtable, the only thread-safe one.
2. Otherwise, does the population reset wholesale or evolve? Cleared as
a unit (per cycle, per request, built-then-discarded) -> the
open-addressed FlatHashtable, which has no tombstones and so offers
no removal beyond clearing. Entries coming and going independently
-> the chained Hashtable, which removes and evicts in place.
Lifetime is the usual shorthand for the second question, and the guide
says where it mis-sorts: a long-lived table that resets on a cycle is a
sequence of short lives and belongs with the short-lived ones. That case
is real -- client-side stats has one table of each shape -- so the guide
describes the two shapes rather than leaving a dev to discover the
exception.
These are the only cross-class references in Hashtable's docs; selection
guidance is the one place a reader needs to know the siblings exist.
Written as {@code} rather than {@link} so nothing dangles at a class
outside this tree.
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>
There was a problem hiding this comment.
💡 Codex Review
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".
| Hashtable.<TEntry>drain(buckets, sink); | ||
| sizeManager.reset(); |
There was a problem hiding this comment.
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 👍 / 👎.
| int bucket = iter.currentBucket(); | ||
| iter.remove(); | ||
| this.cursor = bucket; |
There was a problem hiding this comment.
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 👍 / 👎.
![datadog-prod-us1-4[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
🤖 Datadog Autotest · Commit 2d6bdb9 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| @Nonnull Hashtable.Entry[] buckets, | ||
| @Nonnull Consumer<? super TEntry> sink) { | ||
| Hashtable.<TEntry>drain(buckets, sink); | ||
| sizeManager.reset(); |
There was a problem hiding this comment.
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
| if (evictable.test((TEntry) candidate)) { | ||
| int bucket = iter.currentBucket(); | ||
| iter.remove(); | ||
| this.cursor = bucket; |
There was a problem hiding this comment.
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
…ller's path
tryGetOrCreate returns null once the table is at capacity, so the natural
read-modify-write spelling
table.tryGetOrCreate(key, Counter::new).inc();
compiles, tests, and then throws in production under cardinality pressure --
the one condition no unit test covers. Fusing the update keeps that reference
inside the table: at capacity the update is skipped and false is returned.
Delegates to tryGetOrCreate, so the hash is still computed once and there is no
extra work versus doing it by hand. Context-passing overloads take the side-band
value as an argument against a non-capturing BiConsumer, so a counter add does
not allocate a capturing lambda per call.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The generic context overload boxes on every call, which would make the counter-accumulate shape allocate where the hand-rolled tryGetOrCreate + null-check + field-write it replaces did not. ObjLongConsumer closes that gap for the one shape that motivated tryGetOrUpdate in the first place. D1 only -- D2 has no caller for it yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds the 5-fork Java 17 numbers for the State-backed table alongside the existing tables, and notes that JMH's Blackhole auto-detect picked a different mode than the previous Java 17 run did on the same JVM build -- so absolute numbers are only comparable within a table. add_hashtable now loses to HashMap by ~19% rather than being roughly comparable; update (~3.2x) and iterate (~1.35x) still win. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
What Does This Do?
Reshapes
Hashtableso 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
Supportclass onHashtable, flat onConcurrentHashtable— so moving between the two meant relearning the surface.And "fixed" meant two contradictory things:
getOrCreatepast capFlatHashtable.createFixednullHashtable.D1(capacity)nullConcurrentHashtable.D1.createFixedBucketsTwo of three promised a cap, one promised the opposite. So
createFixedgave 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.FlatHashtablekeepscreateFixed/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:
Hashtable(mirrored inFlatHashtable) — two questions that decide which of the three you want. If that reads wrong, everything below it is wrong.D1end to end — the tier most callers touch:createCapped,get,insert,tryGetOrCreate,tryInsertOrReplace,remove,forEach,clear,drain.SizeManager— the one genuinely new concept.Stateand the statics that take it — the composer tier, for tablesD1/D2don't fit.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
capacityForas the sole bridge: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 addednewHashMapWithExpectedSize. 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/D2constructors are private —createCappedis the only way in, so the posture is explicit at the call site.SizeManager— the one new conceptReserving 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:
That replaces an
isFull()check plus a hand-rolled evict-and-retry. In #12312 it deletes ~25 lines of cursor-resumed scan fromAggregateTable, and the caller no longer knows a cursor exists while still getting its amortization.Parameter order for the size-tracked statics puts
sizeManager/statefirst. 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.maxCapacityis a hard cap on live entries, not a sizing hint:insert()returnsboolean—falseat capacity, instead of growing unboundedly.tryGetOrCreate()returnsnullat capacity when the key is absent; a hit is always returned.tryInsertOrReplace()returnsboolean—falseonly when the key is absent and the table is full; a replacement never grows the table so it always succeeds. It previously threwIllegalStateException, which turned a designed steady state into an exception and allocated a throwable exactly when the table was under most pressure.capacityFor.CardinalityLimitReporter— the one existing consumer — is updated for thebooleaninsert()and the new factory. Its table was previously unbounded; it is now capped at 64.getOrCreate→tryGetOrCreate, and@Nullable. It was annotated@Nonnullwhile returningnullat capacity, which its own javadoc documented — the name reading as total is how the wrong annotation got there.FlatHashtableis renamed too, since it refuses when fixed;ConcurrentHashtablekeeps the plain name because it is uncapped and genuinely cannot refuse.estimateSize/isLikelyEmptyon the composer tier. A reservation counts the moment it is taken, so between reserving and linking the count reads one high.D1/D2keep an exactsize()— they reserve and link inside one call, so the window is never observable from outside.D1/D2.removeno longer allocate a capturing predicate. They delegated toremoveMatchingwithe -> e.matches(key), which captures and so cannot be cached byLambdaMetafactory, whiletryInsertOrReplacebeside it walks the same chain with no lambda. Escape analysis often erases this andremovehas no production caller, so this is a consistency fix rather than a measured throughput win.Out of scope
Support. It is now a pure delegating facade holding no logic. Use the unified Hashtable API in client-side stats #12312 removes the last caller and deletes it.ConcurrentHashtable's port — the rename, the fuzzy cap, and adrainbuilt ongetAndSetrather than unhooking entries (unhooking would truncate an in-flight lock-free reader). Checklist on APMLP-1532.tryCreateOrUpdateand 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:evictAlldeferred its count subtraction past a predicate that could throw, anddrainhanded entries to the sink withnextintact 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:compileJavaand:dd-trace-core:test --tests "datadog.trace.common.metrics.*"D1andD2;tryReserveOrEvictcovered for reserve, evict-to-make-room, and refuse🤖 Generated with Claude Code