Skip to content

Use the unified Hashtable API in client-side stats - #12312

Draft
dougqh wants to merge 8 commits into
feat/hashtable-api-unificationfrom
feat/client-side-stats-hashtable-api
Draft

Use the unified Hashtable API in client-side stats#12312
dougqh wants to merge 8 commits into
feat/hashtable-api-unificationfrom
feat/client-side-stats-hashtable-api

Conversation

@dougqh

@dougqh dougqh commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Stacked on #12101. Review that first — this PR's diff against it is just AggregateTable.

What Does This Do?

Migrates client-side statistics off the deprecated Hashtable.Support facade and onto the API from #12101, as the first real consumer of it.

This is the "use it to review it" half of that PR. The API was shaped in the abstract; this is what it looks like from the outside.

Result

AggregateTable goes from 164 to 132 lines, and from four pieces of hand-managed state to one:

// before
private final Hashtable.Entry[] buckets;
private final int maxAggregates;
private int size;
private int evictCursor;

// after
private final Hashtable.State<AggregateEntry> state;

The biggest deletion is evictOneStale and its helper — a cursor-resumed two-pass scan over [cursor, length) then [0, cursor), ~25 lines — which collapses into one call that reserves a slot and evicts only if the table is actually full:

if (!Hashtable.tryReserveOrEvict(state, STALE)) {
  return null;                        // full, nothing stale -- drop the datum
}

The caller no longer knows a cursor exists, but still gets its amortization: a sustained eviction stream never re-scans the hot prefix more than twice across N evictions.

Also gone: the manual size-- after every unlink (the manager owns the count now), the three paired resets in clear, and the iterator loop in expungeStaleAggregates.

STALE is a static final Predicate<AggregateEntry> — non-capturing, so eviction allocates nothing, and typed, so it needs no cast.

What this exercised in the API

Two warts found by writing this, both fixed in #12101 rather than worked around here:

  • removeMatching needed an explicit Hashtable.<AggregateEntry> type witness, because TEntry had nothing to infer from.
  • Eviction predicates took Predicate<? super Entry>, forcing entry -> ((AggregateEntry) entry).getHitCount() == 0.

Parameterizing State<TEntry> and having the statics take it fixed both, and made a spine/manager mismatch unconstructible rather than merely discouraged.

Behavior

Unchanged. Same cap, same evict-a-stale-entry-or-drop policy on the miss path, same amortized resumable scan — the scan just lives in the primitive now.

Follow-up this unblocks

No production code references Hashtable.Support any more. The facade holds no logic and can be deleted outright — deliberately left for its own PR so this one stays a behaviour-neutral migration.

Test plan

  • ./gradlew :dd-trace-core:test --tests "datadog.trace.common.metrics.*" — passes unchanged
  • ./gradlew :dd-trace-core:compileJava — no deprecation warnings from AggregateTable any more

🤖 Generated with Claude Code

@dougqh dougqh added tag: ai generated Largely based on code generated by an AI or LLM comp: core Tracer core type: refactoring tag: no release notes Changes to exclude from release notes labels Aug 26, 2026
Comment thread dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java Outdated
Comment thread dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java Outdated
Comment thread dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java Outdated
Comment thread dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java Outdated
Comment thread dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java Outdated
dougqh added a commit that referenced this pull request Aug 26, 2026
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>
dougqh added a commit that referenced this pull request Aug 26, 2026
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>
@dougqh
dougqh force-pushed the feat/client-side-stats-hashtable-api branch from 3d295de to 4790ecf Compare August 26, 2026 22:57
@datadog-datadog-prod-us1

This comment has been minimized.

@dd-octo-sts

dd-octo-sts Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

🟢 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.80 s 14.72 s [-0.3%; +1.3%] (no difference)
startup:insecure-bank:tracing:Agent 13.64 s 13.69 s [-1.1%; +0.4%] (no difference)
startup:petclinic:appsec:Agent 17.47 s 17.38 s [-0.4%; +1.4%] (no difference)
startup:petclinic:iast:Agent 17.52 s 17.61 s [-1.4%; +0.4%] (no difference)
startup:petclinic:profiling:Agent 16.80 s 17.24 s [-6.8%; +1.7%] (no difference)
startup:petclinic:sca:Agent 17.54 s 17.39 s [-0.0%; +1.8%] (no difference)
startup:petclinic:tracing:Agent 16.67 s 16.79 s [-1.8%; +0.4%] (no difference)

Commit: 8435b639 · 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 added a commit that referenced this pull request Aug 27, 2026
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>
@dougqh
dougqh force-pushed the feat/client-side-stats-hashtable-api branch from 4790ecf to c5d4d06 Compare August 27, 2026 00:07
dougqh added a commit that referenced this pull request Aug 27, 2026
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>
@dougqh
dougqh force-pushed the feat/client-side-stats-hashtable-api branch from c5d4d06 to 00f8ad1 Compare August 27, 2026 00:16
dougqh and others added 6 commits August 26, 2026 20:47
Replaces the deprecated Support facade, and the hand-rolled bookkeeping,
with Hashtable.State:

- four fields (buckets, maxAggregates, size, evictCursor) become one
  Hashtable.State<AggregateEntry>
- evictOneStale's cursor-resumed two-pass scan -- the [cursor, length)
  then [0, cursor) walk, plus its helper, ~25 lines -- disappears into
  Hashtable.tryReserveOrEvict, which reserves a slot and only evicts if
  the table is actually full
- expungeStaleAggregates' manual iterator loop becomes evictAll
- clear stops pairing three resets by hand
- the stale test is a static final Predicate<AggregateEntry>, so eviction
  allocates no lambda and needs no cast

Behaviour is unchanged: same cap, same evict-a-stale-entry-or-drop policy
on the miss path, same amortized resumable scan -- that scan just lives
in the primitive now instead of here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The eviction predicate spelled the rule out as hitCount == 0, so the
table had to know how staleness is defined. Moving it onto the entry
leaves the call site reading AggregateEntry::isStale.

That is an unbound instance-method reference, so it still coerces to
Predicate<AggregateEntry> and is still non-capturing -- LambdaMetafactory
links it to one cached instance, same as the lambda it replaces.

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>
Notes why AggregateTable.size() stays exact despite delegating to an
estimate: findOrInsert reserves and links without yielding, so the
reservation window is never observable from outside this class.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deleting evictOneStale left its javadoc behind, where it silently
attached to forEach -- so forEach claimed to unlink stale entries and
linked #evictCursor, a field that no longer exists.

The mechanical half of that text (cursor-resumed two-pass scan, its
amortization) now belongs to Hashtable.tryReserveOrEvict, so it goes.
The domain half is knowledge this class still owns and nothing else
records: why a full table drops the new key instead of evicting an
established one, and why cardinality limiting reduces but does not
eliminate eviction. That moves onto findOrInsert, where the decision is
actually made.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing references it any more: this PR moved the last production caller
(AggregateTable) onto the blessed statics, and the facade held no logic
of its own -- every member was a one-line delegate.

Removes 174 lines from Hashtable and the 135-line DeprecatedSupportTests
group, most of which asserted only that a one-liner forwards. The two
members that did have unique behaviour, create(int, float) and
MAX_RATIO, are covered by capacityFor(int, float), which has its own
tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dougqh and others added 2 commits August 27, 2026 14:46
Removes the nullable that only ever appears once the tag table is at
capacity -- the shape most likely to ship as a rare production NPE.
The primitive-long overload keeps record() allocation-free.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp: core Tracer core tag: ai generated Largely based on code generated by an AI or LLM tag: no release notes Changes to exclude from release notes type: refactoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant