From a0629a1b22afd86d80495f06879d5b3f75b69a80 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 28 Aug 2026 14:01:20 -0400 Subject: [PATCH 01/13] Carry Maybe forward pending #12328 merge Maybe/MaybeTest/EscapeShapeBenchmark/MaybeUsagePatternsBenchmark, copied verbatim from the merge-queued PR #12328, so this branch (stacked on #12101) can add Maybe-returning Hashtable/FlatHashtable methods without waiting on the queue. Drop this commit's contents in favor of master's copy once this branch rebases past #12328 landing. --- .../util/MaybeUsagePatternsBenchmark.java | 191 +++++++++ .../util/escape/EscapeShapeBenchmark.java | 404 ++++++++++++++++++ .../main/java/datadog/trace/util/Maybe.java | 156 +++++++ .../java/datadog/trace/util/MaybeTest.java | 114 +++++ 4 files changed, 865 insertions(+) create mode 100644 internal-api/src/jmh/java/datadog/trace/util/MaybeUsagePatternsBenchmark.java create mode 100644 internal-api/src/jmh/java/datadog/trace/util/escape/EscapeShapeBenchmark.java create mode 100644 internal-api/src/main/java/datadog/trace/util/Maybe.java create mode 100644 internal-api/src/test/java/datadog/trace/util/MaybeTest.java diff --git a/internal-api/src/jmh/java/datadog/trace/util/MaybeUsagePatternsBenchmark.java b/internal-api/src/jmh/java/datadog/trace/util/MaybeUsagePatternsBenchmark.java new file mode 100644 index 00000000000..07a05f69627 --- /dev/null +++ b/internal-api/src/jmh/java/datadog/trace/util/MaybeUsagePatternsBenchmark.java @@ -0,0 +1,191 @@ +package datadog.trace.util; + +import java.util.function.BiConsumer; +import java.util.function.ObjLongConsumer; +import javax.annotation.Nullable; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.infra.Blackhole; + +/** + * A do/don't guide for using {@link Maybe}, not a research instrument like {@code + * datadog.trace.util.escape.EscapeShapeBenchmark} (which this class's arms are built on top of). + * Read {@code gc.alloc.rate.norm} — the "good" arm in each pair is expected to read 0 B/op on every + * JDK the way {@code EscapeShapeBenchmark}'s {@code singleSite}/{@code passedToInlinedStrategy} + * arms do; the paired "bad" arm exists to make the regression visible rather than theoretical. Run + * as + * + * 
+ * ./gradlew :internal-api:jmh -Pjmh.includes=MaybeUsagePatternsBenchmark -Pjmh.profilers=gc -PtestJvm=17
+ *
+ * + * This is the intended backing example for a perf-review check like "EA-dependent elision on a hot + * path where a structural alternative exists at parity → prefer the deterministic form": both pairs + * below have a same-cost deterministic form available, so reviewing a real diff against these arms + * is a matter of asking "which arm does this call site look like," not re-deriving the + * escape-analysis argument each time. + * + *

The boxed-context pair is the sharper illustration of that phrase than it first looks + * like. {@code badBoxedContextUpdateInlined} was expected to allocate the boxed {@code Long} + * and, measured here, does not -- with the whole {@code update} call inlined, C2 scalar-replaces + * the box the same as it would any other short-lived object. That is exactly the "EA-dependent" + * half of that phrase: {@link Maybe#update(long, ObjLongConsumer)} has no box to eliminate + * in the first place, so it reads 0 B/op regardless of whether the mutator lambda's own inlining + * holds; the generic-context form's 0 B/op is contingent on that specific inlining, which {@code + * badBoxedContextUpdateUninlined} demonstrates by taking it away via the same {@code + * -XX:CompileCommand=dontinline} technique {@code EscapeShapeBenchmark} uses for its {@code + * UninlinedStrategy} arm. This is narrower than immunity to every inlining failure: if the + * producing method or the {@code update} call itself fails to inline -- a different boundary, + * exercised by {@code EscapeShapeBenchmark}'s {@code passedToUninlinedStrategy} arm (24 B/op) -- + * the {@code Maybe} wrapper itself becomes a real allocation for either overload. + */ +@Fork( + value = 2, + jvmArgsAppend = { + "-XX:CompileCommand=dontinline,datadog.trace.util.MaybeUsagePatternsBenchmark$UninlinedBoxedAdder::accept" + }) +@Warmup(iterations = 3, time = 1) +@Measurement(iterations = 5, time = 1) +@Threads(1) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(java.util.concurrent.TimeUnit.NANOSECONDS) +@State(Scope.Thread) +public class MaybeUsagePatternsBenchmark { + + static final class Widget { + long count; + } + + /** A non-capturing updater, as {@link Maybe#update(long, ObjLongConsumer)} expects. */ + static final ObjLongConsumer ADD_PRIMITIVE = (w, delta) -> w.count += delta; + + /** + * The same update expressed through the generic-context overload instead. {@code Long} is not + * assignable from {@code long} without boxing, so calling {@link Maybe#update(Object, + * BiConsumer)} with a {@code long} argument boxes it every time — the exact per-call allocation + * {@link Maybe#update(long, ObjLongConsumer)} exists to avoid. Kept as a {@code + * BiConsumer} rather than inlined at the call site so the two arms below differ + * only in which overload is selected, not in lambda shape. + */ + static final BiConsumer ADD_BOXED_INLINED = (w, delta) -> w.count += delta; + + /** + * Same logic as {@link #ADD_BOXED_INLINED}, but as a named class rather than a lambda so {@code + * -XX:CompileCommand=dontinline} (see this class's {@link Fork} annotation) has a concrete method + * to target -- kept out of line the same way {@code EscapeShapeBenchmark}'s {@code + * UninlinedStrategy} is, by the {@code CompileCommand} rather than {@code CompilerControl}, since + * JMH's processor only reads that annotation from {@code @Benchmark} methods. + */ + static final class UninlinedBoxedAdder implements BiConsumer { + @Override + public void accept(Widget w, Long delta) { + w.count += delta; + } + } + + static final BiConsumer ADD_BOXED_UNINLINED = new UninlinedBoxedAdder(); + + /** + * Deliberately outside {@code Long}'s [-128, 127] cache range -- a cached delta like {@code 1L} + * would make {@link #badBoxedContextUpdateUninlined} read 0 B/op too, for a reason with nothing + * to do with which overload got picked. + */ + static final long DELTA = 1_000L; + + private final Widget[] table = new Widget[8]; + private int counter; + + public MaybeUsagePatternsBenchmark() { + for (int i = 0; i < table.length; i++) { + // Half the slots stay null so every arm below actually exercises the refused/empty path, + // not just the present one -- see EscapeShapeBenchmark's `alternate()` javadoc for why an + // always-taken branch would quietly turn these into single-site arms and lie. + if ((i & 1) == 0) { + table[i] = new Widget(); + } + } + } + + private int nextKey() { + return (counter++) & (table.length - 1); + } + + @Nullable + private Widget lookup(int key) { + return table[key]; + } + + /** + * GOOD: exactly one {@code Maybe.of(...)} call site, fed by delegating to the existing nullable + * method. See {@link Maybe}'s class javadoc for why this is the recommended shape. + */ + private Maybe tryLookupDelegating(int key) { + return Maybe.of(lookup(key)); + } + + /** + * BAD: a {@code Maybe.of(...)} call site per branch. Both branches return the same wrapper type, + * so this looks equivalent to {@link #tryLookupDelegating} at every call site that uses it — the + * difference only shows up here, in the allocation profile of the method that builds the {@code + * Maybe}, which is exactly why it is easy to introduce by accident. + */ + private Maybe tryLookupMultiSite(int key) { + Widget w = lookup(key); + if (w != null) { + return Maybe.of(w); + } else { + return Maybe.of(null); + } + } + + @Benchmark + public void goodSingleConstructionSite(Blackhole bh) { + Maybe t = tryLookupDelegating(nextKey()); + bh.consume(t.isPresent()); + } + + @Benchmark + public void badMultiConstructionSite(Blackhole bh) { + Maybe t = tryLookupMultiSite(nextKey()); + bh.consume(t.isPresent()); + } + + @Benchmark + public void goodPrimitiveContextUpdate(Blackhole bh) { + Maybe t = tryLookupDelegating(nextKey()); + t.update(DELTA, ADD_PRIMITIVE); + bh.consume(t.isPresent()); + } + + /** + * Reads 0 B/op here despite boxing {@link #DELTA} on every call -- this call site stays inlined, + * so C2 scalar-replaces the {@code Long} the same as any other non-escaping object. See {@link + * #badBoxedContextUpdateUninlined} for what that 0 is actually contingent on. + */ + @Benchmark + public void badBoxedContextUpdateInlined(Blackhole bh) { + Maybe t = tryLookupDelegating(nextKey()); + t.update(DELTA, ADD_BOXED_INLINED); + bh.consume(t.isPresent()); + } + + /** + * The same boxing, with only the inlining taken away (via {@link UninlinedBoxedAdder} and this + * class's {@code CompileCommand}). Whatever this costs above {@link #goodPrimitiveContextUpdate} + * is the box {@link #badBoxedContextUpdateInlined} was quietly relying on EA to remove. + */ + @Benchmark + public void badBoxedContextUpdateUninlined(Blackhole bh) { + Maybe t = tryLookupDelegating(nextKey()); + t.update(DELTA, ADD_BOXED_UNINLINED); + bh.consume(t.isPresent()); + } +} diff --git a/internal-api/src/jmh/java/datadog/trace/util/escape/EscapeShapeBenchmark.java b/internal-api/src/jmh/java/datadog/trace/util/escape/EscapeShapeBenchmark.java new file mode 100644 index 00000000000..8f9ab1c63fe --- /dev/null +++ b/internal-api/src/jmh/java/datadog/trace/util/escape/EscapeShapeBenchmark.java @@ -0,0 +1,404 @@ +package datadog.trace.util.escape; + +import java.util.concurrent.TimeUnit; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.infra.Blackhole; + +/** + * Minimal code shapes, each isolating one thing that is believed to decide whether C2 can delete a + * short-lived object. Read {@code gc.alloc.rate.norm} — bytes per operation — not the timings: a + * shape that scalar-replaces reports 0, and one that does not reports the object's real size. Run + * it as + * + * 

./gradlew :internal-api:jmh -Pjmh.includes=EscapeShape -Pjmh.profilers=gc -PtestJvm=17
+ *
+ * + * and read the same rows across {@code -PtestJvm} 8, 11, 17, 21 and 25. The point is the matrix of + * shape against JDK, so that "will this allocate" stops being a question two people answer from + * memory. Nothing here is specific to any one caller: the arms model a generic two-outcome wrapper + * (something that is either present with a value or absent) and carry over unchanged to {@code + * Maybe}, because the shapes under test are about the compiler's allocation behavior, not about + * what the wrapped value represents. + * + *

The underlying idea, before the terminology: the compiler can sometimes prove a short-lived + * object never needs to outlive the method that created it, and when it can, it skips putting that + * object on the heap at all -- it keeps the object's fields as plain local values instead. Three + * terms for that recur below. Escape analysis (EA) is the compiler's proof step -- showing + * an allocated object's lifetime is confined to the method (or thread) that created it, i.e. it + * never escapes into a field, a return value visible outside, or a call the compiler cannot see + * into. Scalar replacement is what C2 (HotSpot's JIT) does once that proof holds: the object + * itself disappears, and its individual fields live in registers or on the stack instead, so no + * heap allocation happens -- the arms below that read 0 B/op are exactly the ones EA proved safe. + * {@code ReduceAllocationMerges} (JDK-8287061) extends that same proof to one harder case: + * an if/else (or similar branch) where each side allocates its own object -- say {@code x = new + * Foo()} in one branch and {@code x = new Bar()} in the other -- and the code after the branch + * reads {@code x} without knowing which allocation actually ran. Before JDK 21, C2 could not + * scalar-replace either allocation once they were merged like this, even if each individually would + * have qualified on its own; {@code ReduceAllocationMerges} is what lets it do so starting at JDK + * 21, which is why a few rows below only drop to 0 starting at JDK 21/25 rather than on every JDK. + * All of this is specific to HotSpot's C2 JIT; none of it has been checked against OpenJ9 or + * GraalVM, which use different compilers with different heuristics and may not scalar-replace the + * same shapes. + * + *

Every arm consumes the object's fields rather than the object. Handing the reference + * to a {@link Blackhole} would make it escape by construction and every row would read the same. + * + *

Bytes per operation, one machine, {@code -Pjmh.forks=1}. A 16-byte object allocated on half + * the operations reads as 8. Columns are the JDK the fork ran on, which is not necessarily + * the JDK on the shell's path — take it from JMH's own {@code # VM version} line. + * + * 

+ * shape                                 JDK 8  JDK 11  JDK 17  JDK 21  JDK 25   what it isolates
+ * singleSite                                0       ?       0       ?       0   the floor
+ * flagOnOneAllocation                       0       ?       0       ?       0   outcome in a field
+ * closedInFinally                           0       ?       0       ?       0   try/finally
+ * closedInFinallyWithThrow                  0       ?       0       ?       ?   ... with the handler taken
+ * flagOnOneAllocationClosedInFinally        0       ?       0       ?       0   flag field, whole
+ * passedToInlinedStrategy                   0       ?       0       ?       0   @Strategy boundary
+ * backingMonomorphic                        0       ?       0       ?       0   one backing
+ * backingBimorphic                          0       ?       0       ?       0   two backings
+ * mergeWithNull                             8       ?       8       ?       0   merge with null
+ * mergeWithStatic                           8       ?       8       ?       8   merge with a singleton
+ * mergeWithStaticClosedInFinally            8       ?       8       ?       8   ... the same, whole
+ * mergeOfTwoAllocations                    16       ?      16       ?      16   merge of two allocations
+ * passedToUninlinedStrategy                24       ?      24       ?      24   the same boundary, uninlined
+ * backingMegamorphic                       24       ?      24       ?      24   three backings
+ *
+ * + *

JDK 8 column measured 2026-08-27 (Zulu 8.72.0.17, this machine, {@code -Pjmh.fork=1}): every + * arm lands on the same B/op as the 17/25 columns it was checked against, including {@code + * mergeWithNull} staying at 8 rather than following JDK 25's drop to 0 — the {@code + * ReduceAllocationMerges} relaxation is JDK 21+ only, so 8's floor for this shape is the older, + * unconditional one. + * + *

What the two measured columns say so far: + * + *

+ */ +@Fork( + value = 2, + jvmArgsAppend = { + "-XX:CompileCommand=dontinline,datadog.trace.util.escape.EscapeShapeBenchmark$UninlinedStrategy::apply" + }) +@Warmup(iterations = 3, time = 1) +@Measurement(iterations = 5, time = 1) +@Threads(1) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@State(Scope.Thread) +public class EscapeShapeBenchmark { + + /** + * Minimal two-method interface -- a value to read and a close to call -- standing in for any + * short-lived object more complex than a single field. + */ + interface Outcome { + int value(); + + void close(); + } + + static final class SingleAllocation implements Outcome { + private final int seed; + + SingleAllocation(int seed) { + this.seed = seed; + } + + @Override + public int value() { + return seed + 1; + } + + @Override + public void close() {} + } + + /** A second allocation site, for the merge that C2 has some chance with. */ + static final class AlternateAllocation implements Outcome { + private final int seed; + + AlternateAllocation(int seed) { + this.seed = seed; + } + + @Override + public int value() { + return seed + 2; + } + + @Override + public void close() {} + } + + /** The absent outcome, reachable from a static, so the merge it takes part in is not local. */ + static final Outcome STATIC_SINGLETON = + new Outcome() { + @Override + public int value() { + return 0; + } + + @Override + public void close() {} + }; + + /** One allocation site carrying the outcome in a field: the shape that survives. */ + static final class FlaggedAllocation { + private final boolean present; + private final int seed; + + FlaggedAllocation(boolean present, int seed) { + this.present = present; + this.seed = seed; + } + + int value() { + return present ? seed + 1 : 0; + } + + void close() {} + } + + /** + * A non-capturing strategy held in a static final field of concrete type, as {@code @Strategy} + * requires. + */ + interface OutcomeStrategy { + int apply(FlaggedAllocation cell); + } + + static final OutcomeStrategy INLINED = FlaggedAllocation::value; + + /** + * Kept out of line by the {@code CompileCommand} in {@link Fork}, not by {@link CompilerControl}: + * JMH's processor only collects that annotation from {@code @Benchmark} methods, so putting it + * here emits no hint at all and the arm silently becomes a duplicate of the inlined one. Check + * the timing against {@code passedToInlinedStrategy} before believing this row — a call that + * really did not inline cannot cost the same as no call. + */ + static final class UninlinedStrategy implements OutcomeStrategy { + @Override + public int apply(FlaggedAllocation cell) { + return cell.value(); + } + } + + static final OutcomeStrategy UNINLINED = new UninlinedStrategy(); + + /** + * The template-method shape: a final method on a base type calling out to an abstract one, with + * the object under test riding along as the argument. How many concrete subclasses are loaded is + * the whole experiment — C2 inlines a monomorphic call outright and a bimorphic one behind a type + * guard, but gives up at three, and a call it does not inline turns its argument into an escape. + */ + abstract static class Backing { + final int admit(FlaggedAllocation cell) { + return store(cell); + } + + abstract int store(FlaggedAllocation cell); + } + + static final class ArrayBacking extends Backing { + @Override + int store(FlaggedAllocation cell) { + return cell.value(); + } + } + + static final class LinkedBacking extends Backing { + @Override + int store(FlaggedAllocation cell) { + return cell.value() + 1; + } + } + + static final class ThirdBacking extends Backing { + @Override + int store(FlaggedAllocation cell) { + return cell.value() + 2; + } + } + + // All three the same length, so the index arithmetic and the bounds check are identical and the + // only difference between the arms is how many types reach the call site. + // + // Unexplained: the monomorphic arm times slower than the bimorphic one (2.14 against 1.26 ns on + // 17), and equalising the lengths did not change it, so it is not the index arithmetic. Both + // eliminate their allocation, which is what this matrix is for, so the timing oddity does not + // touch any conclusion drawn here — but do not quote these two timings against each other until + // someone has read the assembly. + private final Backing[] one = {new ArrayBacking(), new ArrayBacking(), new ArrayBacking()}; + private final Backing[] two = {new ArrayBacking(), new LinkedBacking(), new ArrayBacking()}; + private final Backing[] three = {new ArrayBacking(), new LinkedBacking(), new ThirdBacking()}; + + // The three arms below are deliberately copy-pasted rather than sharing a helper. A shared helper + // would carry one profile for all three call sites, so the megamorphic arm would poison the other + // two and the matrix would report the same answer three times. + + @Benchmark + public void backingMonomorphic(Blackhole bh) { + Backing backing = one[(counter++ & 0x7fffffff) % one.length]; + FlaggedAllocation cell = new FlaggedAllocation(true, counter); + bh.consume(backing.admit(cell)); + } + + @Benchmark + public void backingBimorphic(Blackhole bh) { + Backing backing = two[(counter++ & 0x7fffffff) % two.length]; + FlaggedAllocation cell = new FlaggedAllocation(true, counter); + bh.consume(backing.admit(cell)); + } + + @Benchmark + public void backingMegamorphic(Blackhole bh) { + Backing backing = three[(counter++ & 0x7fffffff) % three.length]; + FlaggedAllocation cell = new FlaggedAllocation(true, counter); + bh.consume(backing.admit(cell)); + } + + /** + * Alternates so both sides of every branch are taken and the profile is honest. A branch C2 never + * sees taken becomes an uncommon trap, which would quietly turn the merge arms into single-site + * arms and make the whole matrix a lie. + */ + private int counter; + + private boolean alternate() { + return (counter++ & 1) == 0; + } + + @Benchmark + public void singleSite(Blackhole bh) { + SingleAllocation cell = new SingleAllocation(counter++); + bh.consume(cell.value()); + } + + @Benchmark + public void mergeOfTwoAllocations(Blackhole bh) { + Outcome cell = alternate() ? new SingleAllocation(counter) : new AlternateAllocation(counter); + bh.consume(cell.value()); + } + + @Benchmark + public void mergeWithStatic(Blackhole bh) { + Outcome cell = alternate() ? new SingleAllocation(counter) : STATIC_SINGLETON; + bh.consume(cell.value()); + } + + @Benchmark + public void mergeWithNull(Blackhole bh) { + SingleAllocation cell = alternate() ? new SingleAllocation(counter) : null; + bh.consume(cell == null ? 0 : cell.value()); + } + + @Benchmark + public void flagOnOneAllocation(Blackhole bh) { + FlaggedAllocation cell = new FlaggedAllocation(alternate(), counter); + bh.consume(cell.value()); + } + + @Benchmark + public void closedInFinally(Blackhole bh) { + SingleAllocation cell = new SingleAllocation(counter++); + try { + bh.consume(cell.value()); + } finally { + cell.close(); + } + } + + /** Preallocated and stackless, so the arm measures control flow rather than fillInStackTrace. */ + static final class Failure extends RuntimeException { + static final Failure INSTANCE = new Failure(); + + private Failure() { + super("failure", null, false, false); + } + } + + /** + * The same try/finally, with the handler actually taken often enough to be compiled rather than + * left as an uncommon trap. This is the case {@link #closedInFinally} does not cover: there, C2 + * has never seen the exception path, so there is no code for the object to be live into. + */ + @Benchmark + public void closedInFinallyWithThrow(Blackhole bh) { + SingleAllocation cell = new SingleAllocation(counter++); + try { + if ((counter & 15) == 0) { + throw Failure.INSTANCE; + } + bh.consume(cell.value()); + } catch (Failure failure) { + bh.consume(cell.value() + 1); + } finally { + cell.close(); + } + } + + /** The Optional-style shape, whole: a singleton for one outcome, under try/finally. */ + @Benchmark + public void mergeWithStaticClosedInFinally(Blackhole bh) { + Outcome cell = alternate() ? new SingleAllocation(counter) : STATIC_SINGLETON; + try { + bh.consume(cell.value()); + } finally { + cell.close(); + } + } + + /** The single-site shape, whole: one allocation carrying a flag, under try/finally. */ + @Benchmark + public void flagOnOneAllocationClosedInFinally(Blackhole bh) { + FlaggedAllocation cell = new FlaggedAllocation(alternate(), counter); + try { + bh.consume(cell.value()); + } finally { + cell.close(); + } + } + + /** + * A non-escaping object handed across a call boundary the strategy discipline keeps inlinable. + */ + @Benchmark + public void passedToInlinedStrategy(Blackhole bh) { + FlaggedAllocation cell = new FlaggedAllocation(alternate(), counter); + bh.consume(INLINED.apply(cell)); + } + + /** + * The same, with only the inlining taken away. Whatever this costs is what the discipline buys. + */ + @Benchmark + public void passedToUninlinedStrategy(Blackhole bh) { + FlaggedAllocation cell = new FlaggedAllocation(alternate(), counter); + bh.consume(UNINLINED.apply(cell)); + } +} diff --git a/internal-api/src/main/java/datadog/trace/util/Maybe.java b/internal-api/src/main/java/datadog/trace/util/Maybe.java new file mode 100644 index 00000000000..f69bc9784ca --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/util/Maybe.java @@ -0,0 +1,156 @@ +package datadog.trace.util; + +import java.util.function.BiConsumer; +import java.util.function.Consumer; +import java.util.function.Function; +import java.util.function.ObjDoubleConsumer; +import java.util.function.ObjIntConsumer; +import java.util.function.ObjLongConsumer; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; + +/** + * Candidate return shape for fallible operations (e.g. a table's capacity-refusing {@code + * tryGetOrCreate}), evaluating whether it can be allocation-free under escape analysis. + * + *

Deliberately shaped against the {@code Optional}-style merge-with-singleton pitfall: {@link + * #of} is the only allocation site, always allocates (never returns a shared instance), and holds a + * plain nullable field. See {@code EscapeShapeBenchmark}'s {@code phiWithStatic} arm for why an + * {@code EMPTY} singleton would cost 8 B/op on every JDK measured, including 25. + * + *

This shape -- one allocation site, a plain nullable field, no singleton merge -- scalar- + * replaces on ordinary escape analysis, on JDK 8/11/17/25, no JDK-21+ {@code + * ReduceAllocationMerges} needed (see {@code EscapeShapeBenchmark}). The discipline required of a + * caller is that the wrapping method itself construct a {@code Maybe} at exactly one call site (fed + * by a plain nullable local merged through ordinary branches, or by delegating to an + * already-nullable-returning method) rather than once per {@code return} statement -- multiple + * construction sites inline into a multi-producer phi that fails scalar replacement on JDK + * 8/11/17/21 (measured 16 B/op, {@code MaybeUsagePatternsBenchmark#badMultiConstructionSite}) once + * the refusal branch is reachable. On JDK 25, {@code ReduceAllocationMerges} collapses this + * specific shape -- two branches allocating the same final type with identical field layout -- back + * down to 0 B/op; do not rely on that JDK-25-only behavior, since it is exactly the kind of + * EA-dependent elision that can regress silently the moment the two branches stop being trivially + * mergeable (e.g. one branch gains extra state). See {@code EscapeShapeBenchmark}'s {@code + * phiOfTwoAllocations} arm, which uses two distinct interface implementations rather than one + * concrete type and therefore fails to scalar-replace on every JDK including 25 -- a different, + * stronger failure mode than the one demonstrated here. + */ +public final class Maybe { + @Nullable private final T value; + + private Maybe(@Nullable T value) { + this.value = value; + } + + @Nonnull + public static Maybe of(@Nullable T value) { + return new Maybe<>(value); + } + + /** + * Convenience form for the common shape {@code Maybe.of(receiver.someNullableMethod(args))}: + * {@code Maybe.of(receiver, r -> r.someNullableMethod(args))}. Useful when {@code receiver} would + * otherwise have to be re-evaluated or named twice at the call site. + * + *

Unlike the single-arg {@link #of}, {@code fn} here is typically a capturing lambda + * -- it closes over whatever local arguments the caller's method has in scope, so a fresh lambda + * instance is created on every invocation (capturing lambdas are never cached the way a + * non-capturing lambda's singleton instance commonly is) -- which makes it a second heap-object + * candidate distinct from the {@code Maybe} itself. That freshly-allocated capturing lambda still + * scalar-replaces as reliably as a plain delegating method call does, for the shape actually + * measured (JDK 8/11/17/25): a monomorphic receiver and a {@code fn} that is applied exactly once + * and does not itself escape (e.g. by being stored or passed further). If {@code fn} itself + * captures something that must be freshly allocated per call (e.g. a non-singleton creator), that + * allocation is real regardless of what happens to the lambda wrapping it. + */ + @Nonnull + public static Maybe of(R receiver, @Nonnull Function fn) { + return new Maybe<>(fn.apply(receiver)); + } + + public boolean isPresent() { + return value != null; + } + + /** + * Raw accessor -- named to make the null case unmissable at the call site, rather than {@code + * orElse}/{@code get}, neither of which says so on its own. + */ + @Nullable + public T getOrNull() { + return value; + } + + /** + * Primary intended usage: a guard in front of mutation, e.g. {@code + * table.tryGetOrCreateAsTry(key, FooEntry::new).update(FooEntry::inc)}. No-op if the operation + * was refused (table full) rather than throwing or requiring the caller to branch on {@link + * #isPresent()} first. + */ + public void update(Consumer mutator) { + if (value != null) { + mutator.accept(value); + } + } + + /** + * Generic-context form of {@link #update(Consumer)}, for callers that already have a reusable, + * non-capturing {@code BiConsumer} (typically a {@code static final}) plus whatever context it + * needs -- {@code (value, context)} to stay consistent with the primitive-context overloads + * below, at the cost of departing from {@code Hashtable#forEach}'s {@code (context, entry)} + * convention. + */ + public void update(C context, BiConsumer mutator) { + if (value != null) { + mutator.accept(value, context); + } + } + + /** + * Primitive-context form of {@link #update(Consumer)}, for the common case where the mutation + * needs one caller-supplied number (e.g. a duration or count) and boxing it into a captured + * {@code Long}/generic-context object would be the actual per-call allocation. This exists so a + * table wrapping a fallible lookup in {@code Maybe} pays for this shape once, here, instead of + * once per mutator-flavor per table type -- see {@code Hashtable#tryGetOrUpdate}'s {@code + * ObjLongConsumer} overload for the caller-side problem this replaces. + * + *

Deliberately the only primitive-context overload of {@code update}. An {@code + * int}/{@code boolean} sibling was tried and reverted: Java's overload resolution can pick + * cleanly between a primitive overload and the generic {@link #update(Object, BiConsumer)} form + * for a reference-typed argument (boxing is only considered once no non-boxing candidate + * applies), but that guarantee does not extend to a second primitive overload -- {@code update(1, + * lambda)} is ambiguous between {@code int} and {@code long} even with no {@code double} overload + * in the picture, because {@link ObjIntConsumer} and {@link ObjLongConsumer} are unrelated + * interfaces and JLS 15.12.2.5's most-specific-method rule requires every parameter position to + * agree, not just the numeric one. Confirmed by direct compilation, not just JLS reading: an + * inline lambda call breaks as soon as a second primitive overload exists. A plain {@code int} + * argument still widens to {@code long} for free at this single overload -- callers are not + * required to have a {@code long} in hand. {@code double} context is rare enough not to bother + * keeping pretty -- see {@link #updateDouble} for that case, given its own name to sidestep the + * ambiguity rather than trying to squeeze it into an overload. + */ + public void update(long context, ObjLongConsumer mutator) { + if (value != null) { + mutator.accept(value, context); + } + } + + /** + * {@code double}-context sibling of {@link #update(long, ObjLongConsumer)}, given a distinct name + * rather than a second primitive overload -- see that method's javadoc for why overloading {@code + * update} a second time breaks inline-lambda call sites. + */ + public void updateDouble(double context, ObjDoubleConsumer mutator) { + if (value != null) { + mutator.accept(value, context); + } + } + + public void ifPresentOrElse(Consumer action, Runnable emptyAction) { + if (value != null) { + action.accept(value); + } else { + emptyAction.run(); + } + } +} diff --git a/internal-api/src/test/java/datadog/trace/util/MaybeTest.java b/internal-api/src/test/java/datadog/trace/util/MaybeTest.java new file mode 100644 index 00000000000..2420d530bbe --- /dev/null +++ b/internal-api/src/test/java/datadog/trace/util/MaybeTest.java @@ -0,0 +1,114 @@ +package datadog.trace.util; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +public class MaybeTest { + + static final class Widget { + long count; + double total; + } + + @Test + public void ofPresent() { + Maybe maybe = Maybe.of("value"); + assertTrue(maybe.isPresent()); + assertEquals("value", maybe.getOrNull()); + } + + @Test + public void ofAbsent() { + Maybe maybe = Maybe.of(null); + assertFalse(maybe.isPresent()); + assertNull(maybe.getOrNull()); + } + + @Test + public void ofReceiverFunctionPresent() { + Maybe maybe = Maybe.of("value", String::length); + assertTrue(maybe.isPresent()); + assertEquals(5, maybe.getOrNull()); + } + + @Test + public void ofReceiverFunctionAbsent() { + Maybe maybe = Maybe.of("value", r -> null); + assertFalse(maybe.isPresent()); + assertNull(maybe.getOrNull()); + } + + @Test + public void updateConsumerRunsWhenPresent() { + Widget w = new Widget(); + Maybe.of(w).update(widget -> widget.count = 42); + assertEquals(42, w.count); + } + + @Test + public void updateConsumerNoOpWhenAbsent() { + Maybe maybe = Maybe.of(null); + maybe.update(widget -> widget.count = 42); + assertFalse(maybe.isPresent()); + } + + @Test + public void updateBiConsumerRunsWhenPresent() { + Widget w = new Widget(); + Maybe.of(w).update("context", (widget, ctx) -> widget.count = ctx.length()); + assertEquals(7, w.count); + } + + @Test + public void updateBiConsumerNoOpWhenAbsent() { + Maybe maybe = Maybe.of(null); + maybe.update("context", (widget, ctx) -> widget.count = ctx.length()); + assertFalse(maybe.isPresent()); + } + + @Test + public void updateLongRunsWhenPresent() { + Widget w = new Widget(); + Maybe.of(w).update(5L, (widget, delta) -> widget.count += delta); + assertEquals(5, w.count); + } + + @Test + public void updateLongNoOpWhenAbsent() { + Maybe maybe = Maybe.of(null); + maybe.update(5L, (widget, delta) -> widget.count += delta); + assertFalse(maybe.isPresent()); + } + + @Test + public void updateDoubleRunsWhenPresent() { + Widget w = new Widget(); + Maybe.of(w).updateDouble(2.5, (widget, delta) -> widget.total += delta); + assertEquals(2.5, w.total); + } + + @Test + public void updateDoubleNoOpWhenAbsent() { + Maybe maybe = Maybe.of(null); + maybe.updateDouble(2.5, (widget, delta) -> widget.total += delta); + assertFalse(maybe.isPresent()); + } + + @Test + public void ifPresentOrElseRunsActionWhenPresent() { + StringBuilder sb = new StringBuilder(); + Maybe.of("value").ifPresentOrElse(sb::append, () -> sb.append("empty")); + assertEquals("value", sb.toString()); + } + + @Test + public void ifPresentOrElseRunsEmptyActionWhenAbsent() { + StringBuilder sb = new StringBuilder(); + Maybe.of(null).ifPresentOrElse(sb::append, () -> sb.append("empty")); + assertEquals("empty", sb.toString()); + } +} From effe9ea3032793d92e4ac471500c69f69ca29319 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 28 Aug 2026 14:07:52 -0400 Subject: [PATCH 02/13] Add Maybe-returning tryGetOrCreateAsMaybe to Hashtable and FlatHashtable Additive siblings to tryGetOrCreate on Hashtable.D1/D2 and FlatHashtable.D1/D2, wrapping the existing @Nullable-returning method in a Maybe rather than changing its signature. Each delegates to the existing tryGetOrCreate as its sole Maybe#of call site, keeping the allocation-free shape Maybe's class javadoc requires. Validates Maybe against a real caller: the client-side-stats PR (#12312) stacked on top of this one converts CardinalityLimitReporter to tryGetOrCreateAsMaybe(...).update(...). --- .../datadog/trace/util/FlatHashtable.java | 23 ++++++++++++ .../java/datadog/trace/util/Hashtable.java | 29 +++++++++++++++ .../trace/util/FlatHashtableD1Test.java | 21 +++++++++++ .../trace/util/FlatHashtableD2Test.java | 25 +++++++++++++ .../datadog/trace/util/HashtableD1Test.java | 36 +++++++++++++++++++ .../datadog/trace/util/HashtableD2Test.java | 15 ++++++++ 6 files changed, 149 insertions(+) diff --git a/internal-api/src/main/java/datadog/trace/util/FlatHashtable.java b/internal-api/src/main/java/datadog/trace/util/FlatHashtable.java index f8f6731d9a7..4018aec72c6 100644 --- a/internal-api/src/main/java/datadog/trace/util/FlatHashtable.java +++ b/internal-api/src/main/java/datadog/trace/util/FlatHashtable.java @@ -285,6 +285,17 @@ public TEntry tryGetOrCreate(@Nullable K key, @Nonnull CreateStrategy return created; } + /** + * {@link Maybe}-wrapped form of {@link #tryGetOrCreate}, delegating to it as the sole {@link + * Maybe#of} call site -- see {@link Maybe}'s class javadoc for why that shape is required to + * stay allocation-free. A growable table's {@link Maybe} is always present. + */ + @Nonnull + public Maybe tryGetOrCreateAsMaybe( + @Nullable K key, @Nonnull CreateStrategy createStrat) { + return Maybe.of(tryGetOrCreate(key, createStrat)); + } + /** * Unconditionally adds {@code entry} ({@code true}), or {@code false} if a fixed table is full. * Comparison-free and caller-responsible (same contract as {@link FlatHashtable#insert}): the @@ -476,6 +487,18 @@ public TEntry tryGetOrCreate( return created; } + /** + * Two-key analogue of {@link D1#tryGetOrCreateAsMaybe}: {@link Maybe}-wrapped form of {@link + * #tryGetOrCreate}, delegating to it as the sole {@link Maybe#of} call site. + */ + @Nonnull + public Maybe tryGetOrCreateAsMaybe( + @Nullable K1 key1, + @Nullable K2 key2, + @Nonnull CreateStrategy2 createStrat) { + return Maybe.of(tryGetOrCreate(key1, key2, createStrat)); + } + /** * Unconditionally adds {@code entry} ({@code true}), or {@code false} if a fixed table is full. * Comparison-free and caller-responsible: the caller must ensure {@code (key1, key2)} is diff --git a/internal-api/src/main/java/datadog/trace/util/Hashtable.java b/internal-api/src/main/java/datadog/trace/util/Hashtable.java index 3692a365f76..9fad036fd3e 100644 --- a/internal-api/src/main/java/datadog/trace/util/Hashtable.java +++ b/internal-api/src/main/java/datadog/trace/util/Hashtable.java @@ -336,6 +336,23 @@ public TEntry tryGetOrCreate( return newEntry; } + /** + * {@link Maybe}-wrapped form of {@link #tryGetOrCreate}, for callers that want to guard the + * refused-create case with {@link Maybe#update} rather than a manual null check: + * + * 

{@code
+     * table.tryGetOrCreateAsMaybe(key, Counter::new).update(n, ADD);
+     * }
+ * + *

Exactly one {@link Maybe#of} call site, fed by delegating to {@link #tryGetOrCreate} -- + * see {@link Maybe}'s class javadoc for why that shape is required to stay allocation-free. + */ + @Nonnull + public Maybe tryGetOrCreateAsMaybe( + @Nullable K key, @Nonnull Function creator) { + return Maybe.of(tryGetOrCreate(key, creator)); + } + /** * {@link #tryGetOrCreate} followed by {@code updater}, returning whether the update happened. * @@ -650,6 +667,18 @@ public TEntry tryGetOrCreate( return newEntry; } + /** + * Two-key analogue of {@link D1#tryGetOrCreateAsMaybe}: {@link Maybe}-wrapped form of {@link + * #tryGetOrCreate}, delegating to it as the sole {@link Maybe#of} call site. + */ + @Nonnull + public Maybe tryGetOrCreateAsMaybe( + @Nullable K1 key1, + @Nullable K2 key2, + @Nonnull BiFunction creator) { + return Maybe.of(tryGetOrCreate(key1, key2, creator)); + } + /** * Two-key analogue of {@link D1#tryGetOrUpdate(Object, Function, Consumer)}: applies {@code * updater} to the entry for {@code (key1, key2)}, creating one if absent, and returns whether diff --git a/internal-api/src/test/java/datadog/trace/util/FlatHashtableD1Test.java b/internal-api/src/test/java/datadog/trace/util/FlatHashtableD1Test.java index e51462451aa..b8ec29db704 100644 --- a/internal-api/src/test/java/datadog/trace/util/FlatHashtableD1Test.java +++ b/internal-api/src/test/java/datadog/trace/util/FlatHashtableD1Test.java @@ -257,6 +257,27 @@ void fixedGetOrCreateCapsWhenFull() { assertSame(a, table.tryGetOrCreate("a", k -> new StringIntEntry(k, 99))); } + @Test + void fixedGetOrCreateAsMaybeIsAbsentWhenFullButStillReturnsHits() { + FlatHashtable.D1 table = fixed(2); + table.tryGetOrCreate("a", k -> new StringIntEntry(k, 1)); + table.tryGetOrCreate("b", k -> new StringIntEntry(k, 2)); + + assertFalse(table.tryGetOrCreateAsMaybe("c", k -> new StringIntEntry(k, 3)).isPresent()); + + Maybe hit = table.tryGetOrCreateAsMaybe("a", k -> new StringIntEntry(k, 99)); + assertEquals(1, hit.getOrNull().value, "existing entry is still returned even at capacity"); + } + + @Test + void growableGetOrCreateAsMaybeIsAlwaysPresent() { + FlatHashtable.D1 table = growable(1); + for (int i = 0; i < 50; i++) { + assertTrue(table.tryGetOrCreateAsMaybe("k" + i, k -> new StringIntEntry(k, 0)).isPresent()); + } + assertEquals(50, table.size()); + } + @Test void fixedInsertReturnsFalseWhenFull() { FlatHashtable.D1 table = fixed(2); diff --git a/internal-api/src/test/java/datadog/trace/util/FlatHashtableD2Test.java b/internal-api/src/test/java/datadog/trace/util/FlatHashtableD2Test.java index 0900617035e..289d564fe59 100644 --- a/internal-api/src/test/java/datadog/trace/util/FlatHashtableD2Test.java +++ b/internal-api/src/test/java/datadog/trace/util/FlatHashtableD2Test.java @@ -226,6 +226,31 @@ void fixedGetOrCreateCapsWhenFull() { assertSame(a, table.tryGetOrCreate("a", 1, (k1, k2) -> new PairEntry(k1, k2, 99))); } + @Test + void fixedGetOrCreateAsMaybeIsAbsentWhenFullButStillReturnsHits() { + FlatHashtable.D2 table = fixed(2); + table.tryGetOrCreate("a", 1, (k1, k2) -> new PairEntry(k1, k2, 1)); + table.tryGetOrCreate("b", 2, (k1, k2) -> new PairEntry(k1, k2, 2)); + + assertFalse( + table.tryGetOrCreateAsMaybe("c", 3, (k1, k2) -> new PairEntry(k1, k2, 3)).isPresent()); + + Maybe hit = + table.tryGetOrCreateAsMaybe("a", 1, (k1, k2) -> new PairEntry(k1, k2, 99)); + assertEquals(1, hit.getOrNull().value, "existing entry is still returned even at capacity"); + } + + @Test + void growableGetOrCreateAsMaybeIsAlwaysPresent() { + FlatHashtable.D2 table = growable(1); + for (int i = 0; i < 50; i++) { + Maybe maybe = + table.tryGetOrCreateAsMaybe("k", i, (k1, k2) -> new PairEntry(k1, k2, k2)); + assertTrue(maybe.isPresent()); + } + assertEquals(50, table.size()); + } + @Test void fixedInsertReturnsFalseWhenFull() { FlatHashtable.D2 table = fixed(2); diff --git a/internal-api/src/test/java/datadog/trace/util/HashtableD1Test.java b/internal-api/src/test/java/datadog/trace/util/HashtableD1Test.java index 2c1a28b4bf4..24c95b7fb3e 100644 --- a/internal-api/src/test/java/datadog/trace/util/HashtableD1Test.java +++ b/internal-api/src/test/java/datadog/trace/util/HashtableD1Test.java @@ -245,6 +245,42 @@ void getOrCreateNullKeyIsPermitted() { assertEquals(1, table.size()); } + @Test + void getOrCreateAsMaybeOnMissBuildsEntryViaCreator() { + Hashtable.D1 table = Hashtable.D1.createCapped(StringIntEntry.class, 8); + Maybe maybe = + table.tryGetOrCreateAsMaybe("foo", k -> new StringIntEntry(k, 42)); + assertTrue(maybe.isPresent()); + assertEquals(42, maybe.getOrNull().value); + assertSame(table.get("foo"), maybe.getOrNull()); + } + + @Test + void getOrCreateAsMaybeReturnsAbsentOnceAtCapacityButStillReturnsHits() { + Hashtable.D1 table = Hashtable.D1.createCapped(StringIntEntry.class, 2); + table.insert(new StringIntEntry("a", 1)); + table.insert(new StringIntEntry("b", 2)); + + assertFalse(table.tryGetOrCreateAsMaybe("c", k -> new StringIntEntry(k, 3)).isPresent()); + assertEquals(2, table.size()); + + Maybe hit = table.tryGetOrCreateAsMaybe("a", k -> new StringIntEntry(k, 999)); + assertEquals(1, hit.getOrNull().value, "existing entry is still returned even at capacity"); + } + + @Test + void getOrCreateAsMaybeUpdateAppliesOnlyWhenPresent() { + Hashtable.D1 table = Hashtable.D1.createCapped(StringIntEntry.class, 1); + table.insert(new StringIntEntry("a", 1)); + + ObjLongConsumer add = (e, n) -> e.value += n; + table.tryGetOrCreateAsMaybe("a", k -> new StringIntEntry(k, 0)).update(5L, add); + assertEquals(6, table.get("a").value); + + table.tryGetOrCreateAsMaybe("b", k -> new StringIntEntry(k, 0)).update(5L, add); + assertNull(table.get("b"), "refused create at capacity leaves nothing to update"); + } + @Test void insertReturnsFalseOnceAtCapacity() { Hashtable.D1 table = Hashtable.D1.createCapped(StringIntEntry.class, 2); diff --git a/internal-api/src/test/java/datadog/trace/util/HashtableD2Test.java b/internal-api/src/test/java/datadog/trace/util/HashtableD2Test.java index 16739c4a9a5..8af22c9256a 100644 --- a/internal-api/src/test/java/datadog/trace/util/HashtableD2Test.java +++ b/internal-api/src/test/java/datadog/trace/util/HashtableD2Test.java @@ -206,6 +206,21 @@ void getOrCreateReturnsNullOnceAtCapacityButStillReturnsHits() { assertEquals(100, hit.value, "existing entry is still returned even at capacity"); } + @Test + void getOrCreateAsMaybeReturnsAbsentOnceAtCapacityButStillReturnsHits() { + Hashtable.D2 table = Hashtable.D2.createCapped(PairEntry.class, 2); + table.insert(new PairEntry("a", 1, 100)); + table.insert(new PairEntry("b", 2, 200)); + + assertFalse( + table.tryGetOrCreateAsMaybe("c", 3, (k1, k2) -> new PairEntry(k1, k2, 300)).isPresent()); + assertEquals(2, table.size()); + + Maybe hit = + table.tryGetOrCreateAsMaybe("a", 1, (k1, k2) -> new PairEntry(k1, k2, 999)); + assertEquals(100, hit.getOrNull().value, "existing entry is still returned even at capacity"); + } + @Test void tryInsertOrReplaceStillReplacesAtCapacityButRefusesFreshInsert() { Hashtable.D2 table = Hashtable.D2.createCapped(PairEntry.class, 2); From ccf2f1b578d709c9ddd83bf4b2fcfcc0b8ed3275 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 28 Aug 2026 14:27:03 -0400 Subject: [PATCH 03/13] Promote tryGetOrCreateAsMaybe to tryGetOrCreate, demote nullable form to tryGetOrCreateOrNull Maybe becomes the primary get-or-create contract on Hashtable.D1/D2 and FlatHashtable.D1/D2; the raw nullable form survives as an escape hatch under a less-prominent name. Breaking change is affordable now: CardinalityLimitReporter is the only production caller and is updated to the renamed OrNull method here (the fused Maybe-based conversion lands separately in #12312). --- .../metrics/CardinalityLimitReporter.java | 2 +- .../datadog/trace/util/FlatHashtable.java | 88 +++++++------ .../java/datadog/trace/util/Hashtable.java | 124 +++++++++--------- .../main/java/datadog/trace/util/Maybe.java | 7 +- .../trace/util/FlatHashtableD1Test.java | 20 +-- .../trace/util/FlatHashtableD2Test.java | 23 ++-- .../datadog/trace/util/HashtableD1Test.java | 23 ++-- .../datadog/trace/util/HashtableD2Test.java | 14 +- 8 files changed, 151 insertions(+), 150 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/common/metrics/CardinalityLimitReporter.java b/dd-trace-core/src/main/java/datadog/trace/common/metrics/CardinalityLimitReporter.java index fc64b9015d7..3b13a8800bd 100644 --- a/dd-trace-core/src/main/java/datadog/trace/common/metrics/CardinalityLimitReporter.java +++ b/dd-trace-core/src/main/java/datadog/trace/common/metrics/CardinalityLimitReporter.java @@ -58,7 +58,7 @@ final class CardinalityLimitReporter { /** Records {@code count} values blocked for {@code tag} in the current reporting cycle. */ void record(String tag, long count) { if (count > 0) { - TagBlockEntry entry = blockedByTag.tryGetOrCreate(tag, TagBlockEntry::new); + TagBlockEntry entry = blockedByTag.tryGetOrCreateOrNull(tag, TagBlockEntry::new); if (entry != null) { entry.count += count; } diff --git a/internal-api/src/main/java/datadog/trace/util/FlatHashtable.java b/internal-api/src/main/java/datadog/trace/util/FlatHashtable.java index 4018aec72c6..6b78c1c4539 100644 --- a/internal-api/src/main/java/datadog/trace/util/FlatHashtable.java +++ b/internal-api/src/main/java/datadog/trace/util/FlatHashtable.java @@ -123,12 +123,12 @@ protected Entry(long hash) { * *

Fixed or growable, chosen at construction. {@link #createFixed} keeps the raw core's * bounded posture — the table holds up to {@code maxCapacity} entries, then {@link - * #tryGetOrCreate} caps and returns {@code null} (the caller supplies the overflow default). - * {@link #createGrowable} trades that for {@code HashMap}-like ergonomics — its {@code + * #tryGetOrCreateOrNull} caps and returns {@code null} (the caller supplies the overflow + * default). {@link #createGrowable} trades that for {@code HashMap}-like ergonomics — its {@code * initialCapacity} is a sizing hint, not a cap, the table doubles when it fills past its load - * factor, and {@code tryGetOrCreate} never returns {@code null}. The distinct factory names make - * the choice explicit at the call site (there's no ambiguous {@code (Class, int)} constructor); - * {@code Capacity} always counts entries, matching the chained {@code + * factor, and {@code tryGetOrCreateOrNull} never returns {@code null}. The distinct factory names + * make the choice explicit at the call site (there's no ambiguous {@code (Class, int)} + * constructor); {@code Capacity} always counts entries, matching the chained {@code * Hashtable.D1.createCapped}. Across the family a table factory's number is always entries — only * the low-level array allocators take a bucket count. * @@ -197,7 +197,7 @@ private D1(TEntry[] table, float loadFactor, boolean growable, int limit) { /** * A bounded {@link D1} holding up to {@code maxCapacity} entries at the {@link - * #DEFAULT_LOAD_FACTOR}, then capping ({@link #tryGetOrCreate} returns {@code null}). + * #DEFAULT_LOAD_FACTOR}, then capping ({@link #tryGetOrCreateOrNull} returns {@code null}). */ @Nonnull public static > D1 createFixed( @@ -257,18 +257,33 @@ public TEntry get(@Nullable K key) { /** * Existing entry for {@code key}, or a freshly {@link CreateStrategy#create created} + inserted - * one. A growable table never returns {@code null}; a fixed one returns {@code null} when full - * and {@code key} is absent (the caller supplies the overflow default). A hit is always - * returned even at capacity — the cap blocks only creation, not lookup. + * one, wrapped in a {@link Maybe}. A growable table's {@link Maybe} is always present; a fixed + * one's is absent when full and {@code key} is absent (the caller supplies the overflow + * default). A hit is always returned even at capacity — the cap blocks only creation, not + * lookup. * *

The {@code try} prefix marks "this may refuse" — a growable table simply never exercises - * it. The name has to serve both postures, since 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 it costs a redundant null - * check. So it errs toward {@code try}. + * it. + * + *

Delegates to {@link #tryGetOrCreateOrNull} as the sole {@link Maybe#of} call site -- see + * {@link Maybe}'s class javadoc for why that shape is required to stay allocation-free. + */ + @Nonnull + public Maybe tryGetOrCreate( + @Nullable K key, @Nonnull CreateStrategy createStrat) { + return Maybe.of(tryGetOrCreateOrNull(key, createStrat)); + } + + /** + * Low-level, {@code null}-returning form of {@link #tryGetOrCreate}. Prefer the {@link Maybe} + * form above for new call sites; this one remains as an escape hatch for callers where the + * {@link Maybe} allocation-free contract doesn't fit or that pre-date it. Under-promising + * refusal here costs an NPE at the cap; over-promising it costs a redundant null check on a + * growable table -- so it errs toward {@code try}. */ @Nullable - public TEntry tryGetOrCreate(@Nullable K key, @Nonnull CreateStrategy createStrat) { + public TEntry tryGetOrCreateOrNull( + @Nullable K key, @Nonnull CreateStrategy createStrat) { final TEntry existing = get(key); if (existing != null) { return existing; @@ -285,17 +300,6 @@ public TEntry tryGetOrCreate(@Nullable K key, @Nonnull CreateStrategy return created; } - /** - * {@link Maybe}-wrapped form of {@link #tryGetOrCreate}, delegating to it as the sole {@link - * Maybe#of} call site -- see {@link Maybe}'s class javadoc for why that shape is required to - * stay allocation-free. A growable table's {@link Maybe} is always present. - */ - @Nonnull - public Maybe tryGetOrCreateAsMaybe( - @Nullable K key, @Nonnull CreateStrategy createStrat) { - return Maybe.of(tryGetOrCreate(key, createStrat)); - } - /** * Unconditionally adds {@code entry} ({@code true}), or {@code false} if a fixed table is full. * Comparison-free and caller-responsible (same contract as {@link FlatHashtable#insert}): the @@ -404,7 +408,7 @@ private D2(TEntry[] table, float loadFactor, boolean growable, int limit) { /** * A bounded {@link D2} holding up to {@code maxCapacity} entries at the {@link - * #DEFAULT_LOAD_FACTOR}, then capping ({@link #tryGetOrCreate} returns {@code null}). + * #DEFAULT_LOAD_FACTOR}, then capping ({@link #tryGetOrCreateOrNull} returns {@code null}). */ @Nonnull public static > D2 createFixed( @@ -463,11 +467,25 @@ public TEntry get(@Nullable K1 key1, @Nullable K2 key2) { } /** - * Two-key analogue of {@link D1#tryGetOrCreate}: growable never returns {@code null}; fixed - * returns {@code null} when full and {@code (key1, key2)} is absent. + * Two-key analogue of {@link D1#tryGetOrCreate}: {@link Maybe}-wrapped form, delegating to + * {@link #tryGetOrCreateOrNull} as the sole {@link Maybe#of} call site. Growable's {@link + * Maybe} is always present; fixed's is absent when full and {@code (key1, key2)} is absent. + */ + @Nonnull + public Maybe tryGetOrCreate( + @Nullable K1 key1, + @Nullable K2 key2, + @Nonnull CreateStrategy2 createStrat) { + return Maybe.of(tryGetOrCreateOrNull(key1, key2, createStrat)); + } + + /** + * Two-key analogue of {@link D1#tryGetOrCreateOrNull}: low-level, {@code null}-returning form + * of {@link #tryGetOrCreate}. Growable never returns {@code null}; fixed returns {@code null} + * when full and {@code (key1, key2)} is absent. */ @Nullable - public TEntry tryGetOrCreate( + public TEntry tryGetOrCreateOrNull( @Nullable K1 key1, @Nullable K2 key2, @Nonnull CreateStrategy2 createStrat) { @@ -487,18 +505,6 @@ public TEntry tryGetOrCreate( return created; } - /** - * Two-key analogue of {@link D1#tryGetOrCreateAsMaybe}: {@link Maybe}-wrapped form of {@link - * #tryGetOrCreate}, delegating to it as the sole {@link Maybe#of} call site. - */ - @Nonnull - public Maybe tryGetOrCreateAsMaybe( - @Nullable K1 key1, - @Nullable K2 key2, - @Nonnull CreateStrategy2 createStrat) { - return Maybe.of(tryGetOrCreate(key1, key2, createStrat)); - } - /** * Unconditionally adds {@code entry} ({@code true}), or {@code false} if a fixed table is full. * Comparison-free and caller-responsible: the caller must ensure {@code (key1, key2)} is diff --git a/internal-api/src/main/java/datadog/trace/util/Hashtable.java b/internal-api/src/main/java/datadog/trace/util/Hashtable.java index 9fad036fd3e..2e0793afe42 100644 --- a/internal-api/src/main/java/datadog/trace/util/Hashtable.java +++ b/internal-api/src/main/java/datadog/trace/util/Hashtable.java @@ -99,8 +99,8 @@ public final TEntry next() { * *

Capacity is fixed at construction. The table does not resize, so the caller is responsible * for choosing a capacity appropriate to the working set. Once {@link #size()} reaches that - * capacity, {@link #insert} returns {@code false} and {@link #tryGetOrCreate} returns {@code - * null} rather than adding more entries -- a lookup hit is still always returned even at + * capacity, {@link #insert} returns {@code false} and {@link #tryGetOrCreateOrNull} returns + * {@code null} rather than adding more entries -- a lookup hit is still always returned even at * capacity, the cap only blocks new entries. Want your own eviction policy instead of a hard cap? * Drop down to the static building blocks and drive the bucket array yourself -- {@link * Hashtable#createCapped(int)} hands you a spine and a {@link SizeManager} already matched to @@ -172,8 +172,8 @@ private D1(int maxCapacity) { /** * A capped single-key table: it holds at most {@code maxCapacity} live entries, after - * which {@link #insert} returns {@code false} and {@link #tryGetOrCreate} returns {@code null}. - * A lookup hit is still always returned at capacity -- the cap only blocks new entries. + * which {@link #insert} returns {@code false} and {@link #tryGetOrCreateOrNull} returns {@code + * null}. A lookup hit is still always returned at capacity -- the cap only blocks new entries. * *

"Capped" names the promise, not the mechanism: the bucket array is sized once from {@code * maxCapacity} via {@link Hashtable#capacityFor(int)} and never resized, but that is an @@ -292,17 +292,16 @@ public boolean tryInsertOrReplace(@Nonnull TEntry newEntry) { } /** - * Returns the entry for {@code key}, building one via {@code creator} if absent -- or {@code - * null} if the key is absent and the table is at capacity. This method can refuse: - * despite the name it is not total, and a caller that dereferences the result without a null - * check will NPE the first time the cap is reached. A lookup hit is always returned even at - * capacity, so only the create half can fail. Check {@link #isFull()} beforehand if you want to - * distinguish "refused" from "created" without inspecting the result. + * Returns the entry for {@code key}, building one via {@code creator} if absent -- wrapped in a + * {@link Maybe} that is absent if the key is absent and the table is at capacity. A + * lookup hit is always returned even at capacity, so only the create half can fail. Check + * {@link #isFull()} beforehand if you want to distinguish "refused" from "created" without + * inspecting the result. * *

Refusal is a designed steady state for a capped table, not an exceptional condition -- see * {@link #createCapped}. Decide deliberately what a refused create should do (drop the sample, - * fall back, make room); silently ignoring the {@code null} turns the cap into data loss you - * cannot see. + * fall back, make room); silently ignoring an absent {@link Maybe} turns the cap into data loss + * you cannot see. * *

Computes the hash once and reuses it for both the lookup and (on miss) the insert -- * avoids the double-hash that "{@code get}; if null then {@code insert}" would incur. @@ -311,9 +310,26 @@ public boolean tryInsertOrReplace(@Nonnull TEntry newEntry) { * Entry#hash(Object) D1.Entry.hash(key)} -- typically by passing {@code key} to a constructor * that calls {@code super(key)}. A mismatched hash will leave the new entry inserted at a * bucket that future {@link #get} calls won't probe. + * + *

Exactly one {@link Maybe#of} call site, fed by delegating to {@link #tryGetOrCreateOrNull} + * -- see {@link Maybe}'s class javadoc for why that shape is required to stay allocation-free. + * Use {@link #tryGetOrCreateOrNull} directly only when a manual null check is genuinely more + * convenient than {@link Maybe#update}/{@link Maybe#getOrNull}. + */ + @Nonnull + public Maybe tryGetOrCreate( + @Nullable K key, @Nonnull Function creator) { + return Maybe.of(tryGetOrCreateOrNull(key, creator)); + } + + /** + * Low-level, {@code null}-returning form of {@link #tryGetOrCreate}. Prefer the {@link Maybe} + * form above for new call sites; this one remains as an escape hatch for callers where the + * {@link Maybe} allocation-free contract doesn't fit (e.g. storing the result past the current + * stack frame) or that pre-date it. */ @Nullable - public TEntry tryGetOrCreate( + public TEntry tryGetOrCreateOrNull( @Nullable K key, @Nonnull Function creator) { long keyHash = D1.Entry.hash(key); for (TEntry curEntry = bucketFor(this.buckets, keyHash); @@ -337,24 +353,8 @@ public TEntry tryGetOrCreate( } /** - * {@link Maybe}-wrapped form of {@link #tryGetOrCreate}, for callers that want to guard the - * refused-create case with {@link Maybe#update} rather than a manual null check: - * - * 

{@code
-     * table.tryGetOrCreateAsMaybe(key, Counter::new).update(n, ADD);
-     * }
- * - *

Exactly one {@link Maybe#of} call site, fed by delegating to {@link #tryGetOrCreate} -- - * see {@link Maybe}'s class javadoc for why that shape is required to stay allocation-free. - */ - @Nonnull - public Maybe tryGetOrCreateAsMaybe( - @Nullable K key, @Nonnull Function creator) { - return Maybe.of(tryGetOrCreate(key, creator)); - } - - /** - * {@link #tryGetOrCreate} followed by {@code updater}, returning whether the update happened. + * {@link #tryGetOrCreateOrNull} followed by {@code updater}, returning whether the update + * happened. * *

Prefer this over the two-call form for the common read-modify-write shape -- a counter * bump, a max, a timestamp refresh: @@ -365,19 +365,19 @@ public Maybe tryGetOrCreateAsMaybe( * *

The two-call form leaves a {@code null} on the caller's happy path, and the {@code null} * only ever appears once the table is at capacity -- so {@code - * tryGetOrCreate(...).inc()} reads fine, tests fine, and throws in production under cardinality - * pressure. Fusing the update keeps that reference inside the table: at capacity the update is - * skipped and {@code false} is returned, which a counter caller can safely ignore or check - * deliberately. + * tryGetOrCreateOrNull(...).inc()} reads fine, tests fine, and throws in production under + * cardinality pressure. Fusing the update keeps that reference inside the table: at capacity + * the update is skipped and {@code false} is returned, which a counter caller can safely ignore + * or check deliberately. * *

No extra work versus doing it by hand -- the hash is still computed once, by the delegated - * {@link #tryGetOrCreate}. + * {@link #tryGetOrCreateOrNull}. */ public boolean tryGetOrUpdate( @Nullable K key, @Nonnull Function creator, @Nonnull Consumer updater) { - TEntry entry = tryGetOrCreate(key, creator); + TEntry entry = tryGetOrCreateOrNull(key, creator); if (entry == null) { return false; } @@ -396,7 +396,7 @@ public boolean tryGetOrUpdate( @Nonnull Function creator, C context, @Nonnull BiConsumer updater) { - TEntry entry = tryGetOrCreate(key, creator); + TEntry entry = tryGetOrCreateOrNull(key, creator); if (entry == null) { return false; } @@ -421,7 +421,7 @@ public boolean tryGetOrUpdate( @Nonnull Function creator, long context, @Nonnull ObjLongConsumer updater) { - TEntry entry = tryGetOrCreate(key, creator); + TEntry entry = tryGetOrCreateOrNull(key, creator); if (entry == null) { return false; } @@ -550,8 +550,8 @@ private D2(int maxCapacity) { /** * Composite-key analogue of {@link D1#createCapped}: a capped table holding at most * {@code maxCapacity} live entries, after which {@link #insert} returns {@code false} and - * {@link #tryGetOrCreate} returns {@code null}, with lookup hits still always returned. See - * {@link D1#createCapped} for what "capped" promises and why it is the default posture. + * {@link #tryGetOrCreateOrNull} returns {@code null}, with lookup hits still always returned. + * See {@link D1#createCapped} for what "capped" promises and why it is the default posture. * *

{@code entryClass} is a type token only -- it pins the concrete entry type so the compiler * infers {@code K1}, {@code K2}, and {@code TEntry} at the call site (e.g. {@code @@ -632,17 +632,31 @@ public boolean tryInsertOrReplace(@Nonnull TEntry newEntry) { /** * Two-key analogue of {@link D1#tryGetOrCreate}: returns the entry for {@code (key1, key2)}, - * building one via {@code creator} if absent -- or {@code null} if the pair is absent and the - * table is at capacity. Like the single-key form it is not total despite the name, and - * refusal is a designed steady state rather than an exceptional one; see {@link - * D1#tryGetOrCreate} for the full contract and what to do about a refused create. + * building one via {@code creator} if absent -- wrapped in a {@link Maybe} that is absent if + * the pair is absent and the table is at capacity. Refusal is a designed steady state + * rather than an exceptional one; see {@link D1#tryGetOrCreate} for the full contract and what + * to do about a refused create. * *

Computes the combined hash once and reuses it for both lookup and (on miss) insert. The * {@code creator} is expected to build an entry whose {@code keyHash} equals {@link * Entry#hash(Object, Object) D2.Entry.hash(key1, key2)}. + * + *

Delegates to {@link #tryGetOrCreateOrNull} as the sole {@link Maybe#of} call site. + */ + @Nonnull + public Maybe tryGetOrCreate( + @Nullable K1 key1, + @Nullable K2 key2, + @Nonnull BiFunction creator) { + return Maybe.of(tryGetOrCreateOrNull(key1, key2, creator)); + } + + /** + * Two-key analogue of {@link D1#tryGetOrCreateOrNull}: low-level, {@code null}-returning form + * of {@link #tryGetOrCreate}. Prefer the {@link Maybe} form above for new call sites. */ @Nullable - public TEntry tryGetOrCreate( + public TEntry tryGetOrCreateOrNull( @Nullable K1 key1, @Nullable K2 key2, @Nonnull BiFunction creator) { @@ -667,31 +681,19 @@ public TEntry tryGetOrCreate( return newEntry; } - /** - * Two-key analogue of {@link D1#tryGetOrCreateAsMaybe}: {@link Maybe}-wrapped form of {@link - * #tryGetOrCreate}, delegating to it as the sole {@link Maybe#of} call site. - */ - @Nonnull - public Maybe tryGetOrCreateAsMaybe( - @Nullable K1 key1, - @Nullable K2 key2, - @Nonnull BiFunction creator) { - return Maybe.of(tryGetOrCreate(key1, key2, creator)); - } - /** * Two-key analogue of {@link D1#tryGetOrUpdate(Object, Function, Consumer)}: applies {@code * updater} to the entry for {@code (key1, key2)}, creating one if absent, and returns whether * the update happened. Returns {@code false} without updating when the pair is absent and the * table is at capacity. See the single-key form for why fusing the update is preferred over - * {@code tryGetOrCreate(...)} followed by a dereference. + * {@code tryGetOrCreateOrNull(...)} followed by a dereference. */ public boolean tryGetOrUpdate( @Nullable K1 key1, @Nullable K2 key2, @Nonnull BiFunction creator, @Nonnull Consumer updater) { - TEntry entry = tryGetOrCreate(key1, key2, creator); + TEntry entry = tryGetOrCreateOrNull(key1, key2, creator); if (entry == null) { return false; } @@ -711,7 +713,7 @@ public boolean tryGetOrUpdate( @Nonnull BiFunction creator, C context, @Nonnull BiConsumer updater) { - TEntry entry = tryGetOrCreate(key1, key2, creator); + TEntry entry = tryGetOrCreateOrNull(key1, key2, creator); if (entry == null) { return false; } diff --git a/internal-api/src/main/java/datadog/trace/util/Maybe.java b/internal-api/src/main/java/datadog/trace/util/Maybe.java index f69bc9784ca..a3e986174cf 100644 --- a/internal-api/src/main/java/datadog/trace/util/Maybe.java +++ b/internal-api/src/main/java/datadog/trace/util/Maybe.java @@ -82,10 +82,9 @@ public T getOrNull() { } /** - * Primary intended usage: a guard in front of mutation, e.g. {@code - * table.tryGetOrCreateAsTry(key, FooEntry::new).update(FooEntry::inc)}. No-op if the operation - * was refused (table full) rather than throwing or requiring the caller to branch on {@link - * #isPresent()} first. + * Primary intended usage: a guard in front of mutation, e.g. {@code table.tryGetOrCreate(key, + * FooEntry::new).update(FooEntry::inc)}. No-op if the operation was refused (table full) rather + * than throwing or requiring the caller to branch on {@link #isPresent()} first. */ public void update(Consumer mutator) { if (value != null) { diff --git a/internal-api/src/test/java/datadog/trace/util/FlatHashtableD1Test.java b/internal-api/src/test/java/datadog/trace/util/FlatHashtableD1Test.java index b8ec29db704..56594c292fc 100644 --- a/internal-api/src/test/java/datadog/trace/util/FlatHashtableD1Test.java +++ b/internal-api/src/test/java/datadog/trace/util/FlatHashtableD1Test.java @@ -188,7 +188,7 @@ void getOrCreateOnMissBuildsEntryViaCreator() { FlatHashtable.D1 table = growable(8); int[] createCount = {0}; StringIntEntry created = - table.tryGetOrCreate( + table.tryGetOrCreateOrNull( "foo", k -> { createCount[0]++; @@ -209,7 +209,7 @@ void getOrCreateOnHitSkipsCreator() { table.insert(seeded); int[] createCount = {0}; StringIntEntry got = - table.tryGetOrCreate( + table.tryGetOrCreateOrNull( "foo", k -> { createCount[0]++; @@ -237,7 +237,7 @@ void growableGrowsPastInitialCapacity() { void growableGetOrCreateNeverReturnsNull() { FlatHashtable.D1 table = growable(1); for (int i = 0; i < 50; i++) { - StringIntEntry e = table.tryGetOrCreate("k" + i, k -> new StringIntEntry(k, 0)); + StringIntEntry e = table.tryGetOrCreateOrNull("k" + i, k -> new StringIntEntry(k, 0)); assertNotNull(e); } assertEquals(50, table.size()); @@ -246,15 +246,15 @@ void growableGetOrCreateNeverReturnsNull() { @Test void fixedGetOrCreateCapsWhenFull() { FlatHashtable.D1 table = fixed(2); - assertNotNull(table.tryGetOrCreate("a", k -> new StringIntEntry(k, 1))); - assertNotNull(table.tryGetOrCreate("b", k -> new StringIntEntry(k, 2))); + assertNotNull(table.tryGetOrCreateOrNull("a", k -> new StringIntEntry(k, 1))); + assertNotNull(table.tryGetOrCreateOrNull("b", k -> new StringIntEntry(k, 2))); assertEquals(2, table.size()); // At capacity, a new key can't be created -> null (caller's overflow default). - assertNull(table.tryGetOrCreate("c", k -> new StringIntEntry(k, 3))); + assertNull(table.tryGetOrCreateOrNull("c", k -> new StringIntEntry(k, 3))); assertEquals(2, table.size()); // ...but an existing key still resolves even at capacity (cap blocks creation, not lookup). StringIntEntry a = table.get("a"); - assertSame(a, table.tryGetOrCreate("a", k -> new StringIntEntry(k, 99))); + assertSame(a, table.tryGetOrCreateOrNull("a", k -> new StringIntEntry(k, 99))); } @Test @@ -263,9 +263,9 @@ void fixedGetOrCreateAsMaybeIsAbsentWhenFullButStillReturnsHits() { table.tryGetOrCreate("a", k -> new StringIntEntry(k, 1)); table.tryGetOrCreate("b", k -> new StringIntEntry(k, 2)); - assertFalse(table.tryGetOrCreateAsMaybe("c", k -> new StringIntEntry(k, 3)).isPresent()); + assertFalse(table.tryGetOrCreate("c", k -> new StringIntEntry(k, 3)).isPresent()); - Maybe hit = table.tryGetOrCreateAsMaybe("a", k -> new StringIntEntry(k, 99)); + Maybe hit = table.tryGetOrCreate("a", k -> new StringIntEntry(k, 99)); assertEquals(1, hit.getOrNull().value, "existing entry is still returned even at capacity"); } @@ -273,7 +273,7 @@ void fixedGetOrCreateAsMaybeIsAbsentWhenFullButStillReturnsHits() { void growableGetOrCreateAsMaybeIsAlwaysPresent() { FlatHashtable.D1 table = growable(1); for (int i = 0; i < 50; i++) { - assertTrue(table.tryGetOrCreateAsMaybe("k" + i, k -> new StringIntEntry(k, 0)).isPresent()); + assertTrue(table.tryGetOrCreate("k" + i, k -> new StringIntEntry(k, 0)).isPresent()); } assertEquals(50, table.size()); } diff --git a/internal-api/src/test/java/datadog/trace/util/FlatHashtableD2Test.java b/internal-api/src/test/java/datadog/trace/util/FlatHashtableD2Test.java index 289d564fe59..948501185cd 100644 --- a/internal-api/src/test/java/datadog/trace/util/FlatHashtableD2Test.java +++ b/internal-api/src/test/java/datadog/trace/util/FlatHashtableD2Test.java @@ -167,7 +167,7 @@ void getOrCreateOnMissBuildsEntryViaCreator() { FlatHashtable.D2 table = growable(8); int[] createCount = {0}; PairEntry created = - table.tryGetOrCreate( + table.tryGetOrCreateOrNull( "foo", 1, (k1, k2) -> { @@ -190,7 +190,7 @@ void getOrCreateOnHitSkipsCreator() { table.insert(seeded); int[] createCount = {0}; PairEntry got = - table.tryGetOrCreate( + table.tryGetOrCreateOrNull( "foo", 1, (k1, k2) -> { @@ -217,13 +217,13 @@ void growableGrowsPastInitialCapacity() { @Test void fixedGetOrCreateCapsWhenFull() { FlatHashtable.D2 table = fixed(2); - assertNotNull(table.tryGetOrCreate("a", 1, (k1, k2) -> new PairEntry(k1, k2, 1))); - assertNotNull(table.tryGetOrCreate("b", 2, (k1, k2) -> new PairEntry(k1, k2, 2))); + assertNotNull(table.tryGetOrCreateOrNull("a", 1, (k1, k2) -> new PairEntry(k1, k2, 1))); + assertNotNull(table.tryGetOrCreateOrNull("b", 2, (k1, k2) -> new PairEntry(k1, k2, 2))); assertEquals(2, table.size()); - assertNull(table.tryGetOrCreate("c", 3, (k1, k2) -> new PairEntry(k1, k2, 3))); + assertNull(table.tryGetOrCreateOrNull("c", 3, (k1, k2) -> new PairEntry(k1, k2, 3))); assertEquals(2, table.size()); PairEntry a = table.get("a", 1); - assertSame(a, table.tryGetOrCreate("a", 1, (k1, k2) -> new PairEntry(k1, k2, 99))); + assertSame(a, table.tryGetOrCreateOrNull("a", 1, (k1, k2) -> new PairEntry(k1, k2, 99))); } @Test @@ -232,11 +232,9 @@ void fixedGetOrCreateAsMaybeIsAbsentWhenFullButStillReturnsHits() { table.tryGetOrCreate("a", 1, (k1, k2) -> new PairEntry(k1, k2, 1)); table.tryGetOrCreate("b", 2, (k1, k2) -> new PairEntry(k1, k2, 2)); - assertFalse( - table.tryGetOrCreateAsMaybe("c", 3, (k1, k2) -> new PairEntry(k1, k2, 3)).isPresent()); + assertFalse(table.tryGetOrCreate("c", 3, (k1, k2) -> new PairEntry(k1, k2, 3)).isPresent()); - Maybe hit = - table.tryGetOrCreateAsMaybe("a", 1, (k1, k2) -> new PairEntry(k1, k2, 99)); + Maybe hit = table.tryGetOrCreate("a", 1, (k1, k2) -> new PairEntry(k1, k2, 99)); assertEquals(1, hit.getOrNull().value, "existing entry is still returned even at capacity"); } @@ -244,8 +242,7 @@ void fixedGetOrCreateAsMaybeIsAbsentWhenFullButStillReturnsHits() { void growableGetOrCreateAsMaybeIsAlwaysPresent() { FlatHashtable.D2 table = growable(1); for (int i = 0; i < 50; i++) { - Maybe maybe = - table.tryGetOrCreateAsMaybe("k", i, (k1, k2) -> new PairEntry(k1, k2, k2)); + Maybe maybe = table.tryGetOrCreate("k", i, (k1, k2) -> new PairEntry(k1, k2, k2)); assertTrue(maybe.isPresent()); } assertEquals(50, table.size()); @@ -283,7 +280,7 @@ void hashCollisionsResolveByKeyEquality() { void growableGetOrCreateGrowsPastInitialCapacity() { FlatHashtable.D2 table = growable(1); for (int i = 0; i < 50; i++) { - PairEntry e = table.tryGetOrCreate("k", i, (k1, k2) -> new PairEntry(k1, k2, k2)); + PairEntry e = table.tryGetOrCreateOrNull("k", i, (k1, k2) -> new PairEntry(k1, k2, k2)); assertNotNull(e); } assertEquals(50, table.size()); diff --git a/internal-api/src/test/java/datadog/trace/util/HashtableD1Test.java b/internal-api/src/test/java/datadog/trace/util/HashtableD1Test.java index 24c95b7fb3e..9905642fa92 100644 --- a/internal-api/src/test/java/datadog/trace/util/HashtableD1Test.java +++ b/internal-api/src/test/java/datadog/trace/util/HashtableD1Test.java @@ -202,7 +202,7 @@ void getOrCreateOnMissBuildsEntryViaCreator() { Hashtable.D1 table = Hashtable.D1.createCapped(StringIntEntry.class, 8); int[] createCount = {0}; StringIntEntry created = - table.tryGetOrCreate( + table.tryGetOrCreateOrNull( "foo", k -> { createCount[0]++; @@ -223,7 +223,7 @@ void getOrCreateOnHitSkipsCreator() { table.insert(seeded); int[] createCount = {0}; StringIntEntry got = - table.tryGetOrCreate( + table.tryGetOrCreateOrNull( "foo", k -> { createCount[0]++; @@ -237,19 +237,18 @@ void getOrCreateOnHitSkipsCreator() { @Test void getOrCreateNullKeyIsPermitted() { Hashtable.D1 table = Hashtable.D1.createCapped(StringIntEntry.class, 8); - StringIntEntry created = table.tryGetOrCreate(null, k -> new StringIntEntry(k, 7)); + StringIntEntry created = table.tryGetOrCreateOrNull(null, k -> new StringIntEntry(k, 7)); assertNotNull(created); assertNull(created.key); assertEquals(7, created.value); - assertSame(created, table.tryGetOrCreate(null, k -> new StringIntEntry(k, 999))); + assertSame(created, table.tryGetOrCreateOrNull(null, k -> new StringIntEntry(k, 999))); assertEquals(1, table.size()); } @Test void getOrCreateAsMaybeOnMissBuildsEntryViaCreator() { Hashtable.D1 table = Hashtable.D1.createCapped(StringIntEntry.class, 8); - Maybe maybe = - table.tryGetOrCreateAsMaybe("foo", k -> new StringIntEntry(k, 42)); + Maybe maybe = table.tryGetOrCreate("foo", k -> new StringIntEntry(k, 42)); assertTrue(maybe.isPresent()); assertEquals(42, maybe.getOrNull().value); assertSame(table.get("foo"), maybe.getOrNull()); @@ -261,10 +260,10 @@ void getOrCreateAsMaybeReturnsAbsentOnceAtCapacityButStillReturnsHits() { table.insert(new StringIntEntry("a", 1)); table.insert(new StringIntEntry("b", 2)); - assertFalse(table.tryGetOrCreateAsMaybe("c", k -> new StringIntEntry(k, 3)).isPresent()); + assertFalse(table.tryGetOrCreate("c", k -> new StringIntEntry(k, 3)).isPresent()); assertEquals(2, table.size()); - Maybe hit = table.tryGetOrCreateAsMaybe("a", k -> new StringIntEntry(k, 999)); + Maybe hit = table.tryGetOrCreate("a", k -> new StringIntEntry(k, 999)); assertEquals(1, hit.getOrNull().value, "existing entry is still returned even at capacity"); } @@ -274,10 +273,10 @@ void getOrCreateAsMaybeUpdateAppliesOnlyWhenPresent() { table.insert(new StringIntEntry("a", 1)); ObjLongConsumer add = (e, n) -> e.value += n; - table.tryGetOrCreateAsMaybe("a", k -> new StringIntEntry(k, 0)).update(5L, add); + table.tryGetOrCreate("a", k -> new StringIntEntry(k, 0)).update(5L, add); assertEquals(6, table.get("a").value); - table.tryGetOrCreateAsMaybe("b", k -> new StringIntEntry(k, 0)).update(5L, add); + table.tryGetOrCreate("b", k -> new StringIntEntry(k, 0)).update(5L, add); assertNull(table.get("b"), "refused create at capacity leaves nothing to update"); } @@ -297,10 +296,10 @@ void getOrCreateReturnsNullOnceAtCapacityButStillReturnsHits() { table.insert(new StringIntEntry("a", 1)); table.insert(new StringIntEntry("b", 2)); - assertNull(table.tryGetOrCreate("c", k -> new StringIntEntry(k, 3))); + assertNull(table.tryGetOrCreateOrNull("c", k -> new StringIntEntry(k, 3))); assertEquals(2, table.size()); - StringIntEntry hit = table.tryGetOrCreate("a", k -> new StringIntEntry(k, 999)); + StringIntEntry hit = table.tryGetOrCreateOrNull("a", k -> new StringIntEntry(k, 999)); assertEquals(1, hit.value, "existing entry is still returned even at capacity"); } diff --git a/internal-api/src/test/java/datadog/trace/util/HashtableD2Test.java b/internal-api/src/test/java/datadog/trace/util/HashtableD2Test.java index 8af22c9256a..a7378f55904 100644 --- a/internal-api/src/test/java/datadog/trace/util/HashtableD2Test.java +++ b/internal-api/src/test/java/datadog/trace/util/HashtableD2Test.java @@ -85,7 +85,7 @@ void getOrCreateOnMissBuildsEntryViaCreator() { Hashtable.D2 table = Hashtable.D2.createCapped(PairEntry.class, 8); int[] createCount = {0}; PairEntry created = - table.tryGetOrCreate( + table.tryGetOrCreateOrNull( "a", 1, (k1, k2) -> { @@ -108,7 +108,7 @@ void getOrCreateOnHitSkipsCreator() { table.insert(seeded); int[] createCount = {0}; PairEntry got = - table.tryGetOrCreate( + table.tryGetOrCreateOrNull( "a", 1, (k1, k2) -> { @@ -199,10 +199,10 @@ void getOrCreateReturnsNullOnceAtCapacityButStillReturnsHits() { table.insert(new PairEntry("a", 1, 100)); table.insert(new PairEntry("b", 2, 200)); - assertNull(table.tryGetOrCreate("c", 3, (k1, k2) -> new PairEntry(k1, k2, 300))); + assertNull(table.tryGetOrCreateOrNull("c", 3, (k1, k2) -> new PairEntry(k1, k2, 300))); assertEquals(2, table.size()); - PairEntry hit = table.tryGetOrCreate("a", 1, (k1, k2) -> new PairEntry(k1, k2, 999)); + PairEntry hit = table.tryGetOrCreateOrNull("a", 1, (k1, k2) -> new PairEntry(k1, k2, 999)); assertEquals(100, hit.value, "existing entry is still returned even at capacity"); } @@ -212,12 +212,10 @@ void getOrCreateAsMaybeReturnsAbsentOnceAtCapacityButStillReturnsHits() { table.insert(new PairEntry("a", 1, 100)); table.insert(new PairEntry("b", 2, 200)); - assertFalse( - table.tryGetOrCreateAsMaybe("c", 3, (k1, k2) -> new PairEntry(k1, k2, 300)).isPresent()); + assertFalse(table.tryGetOrCreate("c", 3, (k1, k2) -> new PairEntry(k1, k2, 300)).isPresent()); assertEquals(2, table.size()); - Maybe hit = - table.tryGetOrCreateAsMaybe("a", 1, (k1, k2) -> new PairEntry(k1, k2, 999)); + Maybe hit = table.tryGetOrCreate("a", 1, (k1, k2) -> new PairEntry(k1, k2, 999)); assertEquals(100, hit.getOrNull().value, "existing entry is still returned even at capacity"); } From 1d9c2c00110d04b013387cba48b2cd01acffbf3d Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Wed, 26 Aug 2026 18:09:40 -0400 Subject: [PATCH 04/13] Migrate AggregateTable to the blessed Hashtable API Replaces the deprecated Support facade, and the hand-rolled bookkeeping, with Hashtable.State: - four fields (buckets, maxAggregates, size, evictCursor) become one Hashtable.State - 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, 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) --- .../trace/common/metrics/AggregateTable.java | 82 ++++++------------- 1 file changed, 25 insertions(+), 57 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java index b120cb8b915..57297f35ec3 100644 --- a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java +++ b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java @@ -2,9 +2,9 @@ import datadog.trace.core.monitor.HealthMetrics; import datadog.trace.util.Hashtable; -import datadog.trace.util.Hashtable.MutatingTableIterator; import java.util.function.BiConsumer; import java.util.function.Consumer; +import java.util.function.Predicate; /** * The {@link AggregateEntry} store of the consuming aggregator thread, keyed on the canonical @@ -25,17 +25,20 @@ */ final class AggregateTable { - private final Hashtable.Entry[] buckets; - private final int maxAggregates; - private final AggregateEntry.Canonical canonical; - private int size; + /** + * Stale means "not used in this reporting cycle". Held as a {@code static final} so it is a + * non-capturing singleton rather than a fresh lambda per eviction. + */ + private static final Predicate STALE = entry -> entry.getHitCount() == 0; /** - * Bucket index where the last {@link #evictOneStale} successfully removed an entry. The next call - * resumes from this bucket so a fast-evicting workload doesn't repeatedly re-walk the same hot - * entries clustered near bucket 0. Reset to {@code 0} by {@link #clear}. + * Bucket spine plus the manager that keeps it within {@code maxAggregates} -- the manager also + * owns the resumable eviction scan, so consecutive evictions don't re-walk the same hot entries + * clustered near bucket 0. */ - private int evictCursor; + private final Hashtable.State state; + + private final AggregateEntry.Canonical canonical; AggregateTable(int maxAggregates) { this(maxAggregates, AdditionalTagsSchema.EMPTY); @@ -47,8 +50,7 @@ final class AggregateTable { AggregateTable( int maxAggregates, CoreHandlers handlers, AdditionalTagsSchema additionalTagsSchema) { - this.buckets = Hashtable.Support.create(maxAggregates, Hashtable.Support.MAX_RATIO); - this.maxAggregates = maxAggregates; + this.state = Hashtable.createCapped(maxAggregates); this.canonical = new AggregateEntry.Canonical(handlers, additionalTagsSchema); } @@ -57,11 +59,11 @@ void resetCoreHandlers(HealthMetrics healthMetrics, CardinalityLimitReporter rep } int size() { - return size; + return state.sizeManager.size(); } boolean isEmpty() { - return size == 0; + return state.sizeManager.size() == 0; } /** @@ -72,20 +74,20 @@ boolean isEmpty() { AggregateEntry findOrInsert(SpanSnapshot snapshot) { canonical.populateFrom(snapshot); long keyHash = canonical.keyHash; - for (AggregateEntry candidate = Hashtable.Support.bucket(buckets, keyHash); + for (AggregateEntry candidate = Hashtable.bucketFor(state.buckets, keyHash); candidate != null; candidate = candidate.next()) { if (candidate.keyHash == keyHash && canonical.matches(candidate)) { return candidate; } } - // Miss path. - if (size >= maxAggregates && !evictOneStale()) { + // Miss path. Reserve before building the entry so a refused insert costs no allocation; the + // reservation evicts a stale entry to make room if the table is already full. + if (!Hashtable.tryReserveOrEvict(state, STALE)) { return null; } AggregateEntry entry = canonical.createEntry(); - Hashtable.Support.insertHeadEntry(buckets, keyHash, entry); - size++; + Hashtable.insertHeadEntryFor(state.buckets, keyHash, entry); return entry; } @@ -106,32 +108,8 @@ AggregateEntry findOrInsert(SpanSnapshot snapshot) { * still drive the entry count to {@code maxAggregates}, so this cursor-resumed scan remains the * backstop. */ - private boolean evictOneStale() { - // Two passes -- [cursor, length) then [0, cursor) -- using the half-open-range iterator. The - // second pass is naturally empty when cursor==0, so no extra check needed. - return evictOneStaleInRange(evictCursor, buckets.length) - || evictOneStaleInRange(0, evictCursor); - } - - /** Scans {@code [startBucket, endBucket)} for the first stale entry and unlinks it. */ - private boolean evictOneStaleInRange(int startBucket, int endBucket) { - MutatingTableIterator iter = - Hashtable.Support.mutatingTableIterator(buckets, startBucket, endBucket); - while (iter.hasNext()) { - AggregateEntry e = iter.next(); - if (e.getHitCount() == 0) { - int bucket = iter.currentBucket(); - iter.remove(); - size--; - evictCursor = bucket; - return true; - } - } - return false; - } - void forEach(Consumer consumer) { - Hashtable.Support.forEach(buckets, consumer); + Hashtable.forEach(state.buckets, consumer); } /** @@ -139,26 +117,16 @@ void forEach(Consumer consumer) { * each invocation -- pass a non-capturing {@link BiConsumer} (typically a {@code static final}) * plus whatever side-band state it needs as {@code context}. */ - void forEach(T context, BiConsumer consumer) { - Hashtable.Support.forEach(buckets, context, consumer); + void forEach(C context, BiConsumer consumer) { + Hashtable.forEach(state.buckets, context, consumer); } /** Removes entries whose {@code getHitCount() == 0}. */ void expungeStaleAggregates() { - for (MutatingTableIterator iter = - Hashtable.Support.mutatingTableIterator(buckets); - iter.hasNext(); ) { - AggregateEntry e = iter.next(); - if (e.getHitCount() == 0) { - iter.remove(); - size--; - } - } + Hashtable.evictAll(state, STALE); } void clear() { - Hashtable.Support.clear(buckets); - size = 0; - evictCursor = 0; + Hashtable.clear(state); } } From ee128fb1b92becbd94212b66631677ba8b17f115 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Wed, 26 Aug 2026 18:51:05 -0400 Subject: [PATCH 05/13] Encapsulate the staleness rule as AggregateEntry.isStale 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 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) --- .../datadog/trace/common/metrics/AggregateEntry.java | 11 +++++++++++ .../datadog/trace/common/metrics/AggregateTable.java | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java index 1214b246470..646725636ec 100644 --- a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java +++ b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java @@ -244,6 +244,17 @@ public int getHitCount() { return hitCount; } + /** + * {@code true} if nothing hit this entry in the current reporting cycle, making it the first + * thing worth evicting when the table is full. Encapsulates the staleness rule on the entry so + * the table doesn't have to know it is spelled {@code hitCount == 0}, and reads as {@code + * AggregateEntry::isStale} at an eviction call site -- an unbound method reference, so it is + * non-capturing and costs no allocation. + */ + public boolean isStale() { + return hitCount == 0; + } + public int getErrorCount() { return errorCount; } diff --git a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java index 57297f35ec3..cac8d539655 100644 --- a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java +++ b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java @@ -29,7 +29,7 @@ final class AggregateTable { * Stale means "not used in this reporting cycle". Held as a {@code static final} so it is a * non-capturing singleton rather than a fresh lambda per eviction. */ - private static final Predicate STALE = entry -> entry.getHitCount() == 0; + private static final Predicate STALE = AggregateEntry::isStale; /** * Bucket spine plus the manager that keeps it within {@code maxAggregates} -- the manager also From 6461012631077cc47173ef5941bbfae7da57e68d Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Wed, 26 Aug 2026 18:57:01 -0400 Subject: [PATCH 06/13] Reach State only through the statics in AggregateTable 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) --- .../trace/common/metrics/AggregateTable.java | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java index cac8d539655..5eb5ac9269a 100644 --- a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java +++ b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java @@ -31,11 +31,6 @@ final class AggregateTable { */ private static final Predicate STALE = AggregateEntry::isStale; - /** - * Bucket spine plus the manager that keeps it within {@code maxAggregates} -- the manager also - * owns the resumable eviction scan, so consecutive evictions don't re-walk the same hot entries - * clustered near bucket 0. - */ private final Hashtable.State state; private final AggregateEntry.Canonical canonical; @@ -59,11 +54,11 @@ void resetCoreHandlers(HealthMetrics healthMetrics, CardinalityLimitReporter rep } int size() { - return state.sizeManager.size(); + return Hashtable.size(state); } boolean isEmpty() { - return state.sizeManager.size() == 0; + return Hashtable.isEmpty(state); } /** @@ -74,7 +69,7 @@ boolean isEmpty() { AggregateEntry findOrInsert(SpanSnapshot snapshot) { canonical.populateFrom(snapshot); long keyHash = canonical.keyHash; - for (AggregateEntry candidate = Hashtable.bucketFor(state.buckets, keyHash); + for (AggregateEntry candidate = Hashtable.bucketFor(state, keyHash); candidate != null; candidate = candidate.next()) { if (candidate.keyHash == keyHash && canonical.matches(candidate)) { @@ -87,7 +82,7 @@ AggregateEntry findOrInsert(SpanSnapshot snapshot) { return null; } AggregateEntry entry = canonical.createEntry(); - Hashtable.insertHeadEntryFor(state.buckets, keyHash, entry); + Hashtable.insertReserved(state, keyHash, entry); return entry; } @@ -109,7 +104,7 @@ AggregateEntry findOrInsert(SpanSnapshot snapshot) { * backstop. */ void forEach(Consumer consumer) { - Hashtable.forEach(state.buckets, consumer); + Hashtable.forEach(state, consumer); } /** @@ -118,7 +113,7 @@ void forEach(Consumer consumer) { * plus whatever side-band state it needs as {@code context}. */ void forEach(C context, BiConsumer consumer) { - Hashtable.forEach(state.buckets, context, consumer); + Hashtable.forEach(state, context, consumer); } /** Removes entries whose {@code getHitCount() == 0}. */ From e565370949c2ec0e3a477d4b1b27e743b545ac01 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Wed, 26 Aug 2026 20:07:28 -0400 Subject: [PATCH 07/13] Follow the estimateSize/isLikelyEmpty rename in AggregateTable 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) --- .../datadog/trace/common/metrics/AggregateTable.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java index 5eb5ac9269a..2e6a959f139 100644 --- a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java +++ b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java @@ -53,12 +53,17 @@ void resetCoreHandlers(HealthMetrics healthMetrics, CardinalityLimitReporter rep canonical.handlers.reset(healthMetrics, reporter); } + /** + * Live aggregate count. Exact from this class's point of view: {@link Hashtable#estimateSize} is + * an estimate only across a reservation window, and {@link #findOrInsert} reserves and links + * without yielding, so no caller can observe one. + */ int size() { - return Hashtable.size(state); + return Hashtable.estimateSize(state); } boolean isEmpty() { - return Hashtable.isEmpty(state); + return Hashtable.isLikelyEmpty(state); } /** From 8ad26429f1825cc6ff43cc9b386ef5de8e88dcd6 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Wed, 26 Aug 2026 20:21:26 -0400 Subject: [PATCH 08/13] Rehome the eviction rationale after deleting evictOneStale 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) --- .../trace/common/metrics/AggregateTable.java | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java index 2e6a959f139..dd03187ad55 100644 --- a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java +++ b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java @@ -69,7 +69,19 @@ boolean isEmpty() { /** * Returns the {@link AggregateEntry} to update for {@code snapshot}, lazily creating one on miss. * Returns {@code null} when the table is at capacity and no stale entry can be evicted -- the - * caller should drop the data point in that case. + * caller should drop the data point in that case (reported via {@code + * onStatsAggregateDropped}). Dropping the new key rather than evicting an established one is + * deliberate: the cap is sized to the steady-state working set, so a full table of entries that + * were all used this cycle means the new key is the outlier. + * + *

Cardinality limiting (see {@link MetricCardinalityLimits#USE_BLOCKED_SENTINEL}) reduces how + * often eviction fires but doesn't eliminate it. Over-cap values for a single field collapse into + * the shared {@code tracer_blocked_value} sentinel, so no one field can fill the table on its + * own. But distinct in-budget combinations across fields (resource x service x operation x ...) + * can still drive the entry count to {@code maxAggregates}, so eviction remains the backstop. + * + *

The scan that finds a stale entry, and its resume-where-it-left-off amortization, live in + * {@link Hashtable#tryReserveOrEvict} -- this class only supplies {@link #STALE}. */ AggregateEntry findOrInsert(SpanSnapshot snapshot) { canonical.populateFrom(snapshot); @@ -91,23 +103,6 @@ AggregateEntry findOrInsert(SpanSnapshot snapshot) { return entry; } - /** - * Unlinks the first entry whose {@code getHitCount() == 0}, resuming the scan from {@link - * #evictCursor} so consecutive evictions amortize to O(1) per call. Worst case for a single call - * is still O(N) when nearly every entry is hot, but a sustained eviction stream never re-scans - * the hot prefix more than twice across N evictions. - * - *

If the table is full and every entry was used in this cycle, drop the new key (reported via - * {@code onStatsAggregateDropped}) rather than evicting an established one. Cap is sized to the - * steady-state working set, so eviction is rare in the common case. - * - *

Cardinality limiting (see {@link MetricCardinalityLimits#USE_BLOCKED_SENTINEL}) reduces how - * often this fires but doesn't eliminate it. Over-cap values for a single field collapse into the - * shared {@code tracer_blocked_value} sentinel, so no one field can fill the table on its own. - * But distinct in-budget combinations across fields (resource x service x operation x ...) can - * still drive the entry count to {@code maxAggregates}, so this cursor-resumed scan remains the - * backstop. - */ void forEach(Consumer consumer) { Hashtable.forEach(state, consumer); } From b4af2d95c6fefe3948a67fca9a6cb580b19994c3 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Wed, 26 Aug 2026 20:28:57 -0400 Subject: [PATCH 09/13] Delete the deprecated Hashtable.Support facade 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) --- .../java/datadog/trace/util/Hashtable.java | 174 ------------------ .../datadog/trace/util/HashtableTest.java | 138 -------------- 2 files changed, 312 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/util/Hashtable.java b/internal-api/src/main/java/datadog/trace/util/Hashtable.java index 2e0793afe42..81e5daa7a0f 100644 --- a/internal-api/src/main/java/datadog/trace/util/Hashtable.java +++ b/internal-api/src/main/java/datadog/trace/util/Hashtable.java @@ -1497,180 +1497,6 @@ public static State createCapped(int maxCapacity) return new State<>(buckets, maxCapacity); } - /** - * Deprecated facade over the static building blocks that are now methods on {@link Hashtable} - * itself. Every member here delegates to its {@code Hashtable.*} counterpart -- no real logic - * lives in this class, so it can be deleted outright once the last caller migrates. - * - *

Retained only for source compatibility with existing callers. New code should call the - * {@code Hashtable.*} statics directly. - * - * @deprecated use the static building blocks on {@link Hashtable} directly. - */ - @Deprecated - public static final class Support { - private Support() {} - - /** - * @deprecated use {@link Hashtable#create(int)} (or {@link Hashtable#create(Class, int)} for a - * typed spine). - */ - @Deprecated - @Nonnull - public static Hashtable.Entry[] create(int requestedSize) { - return Hashtable.create(requestedSize); - } - - /** - * Scales the requested working-set size before sizing the bucket array. Pair with {@link - * #MAX_RATIO} to leave headroom over the working set for a desired load factor; the canonical - * call is {@code create(n, MAX_RATIO)}. - * - *

The scaled size is truncated to {@code int} before going through {@link - * Hashtable#sizeFor(int)}. Truncation rather than {@code ceil} is intentional: {@code sizeFor} - * rounds up to the next power of two anyway, so the fractional part would only matter when - * float fuzz pushes the result across a power-of-two boundary -- {@code ceil} would then double - * the array size for no reason (e.g. {@code 12 * 4/3 = 16.0...0005f -> ceil 17 -> sizeFor 32}). - * - * @deprecated use {@link Hashtable#capacityFor(int)} (or {@link Hashtable#capacityFor(int, - * float)} for a load factor other than {@link Hashtable#DEFAULT_LOAD_FACTOR}), then {@link - * Hashtable#create(Class, int)} with the result. - */ - @Deprecated - @Nonnull - public static Hashtable.Entry[] create(int requestedSize, float scale) { - // Deliberately multiplies by `scale` rather than routing through - // Hashtable#capacityFor(int, float), which divides by a load factor: `n * MAX_RATIO` and - // `n / DEFAULT_LOAD_FACTOR` are not bit-identical in float, and this deprecated path keeps - // its exact legacy sizing. Only the allocation itself is inverted onto the blessed API. - return Hashtable.create((int) (requestedSize * scale)); - } - - /** - * Inverse of a 75% load factor. Callers that size their bucket array from a target working-set - * size {@code n} should pass {@code create(n, MAX_RATIO)} to leave ~25% headroom in the array. - * - * @deprecated equivalent to {@code 1f / Hashtable#DEFAULT_LOAD_FACTOR}; prefer {@link - * Hashtable#capacityFor(int)}, which applies that load factor directly. - */ - @Deprecated public static final float MAX_RATIO = 1.0f / Hashtable.DEFAULT_LOAD_FACTOR; - - /** - * @deprecated use {@link Hashtable#sizeFor(int)}. - */ - @Deprecated - static int sizeFor(int requestedSize) { - return Hashtable.sizeFor(requestedSize); - } - - /** - * @deprecated use {@link Hashtable#clear(Hashtable.Entry[])}. - */ - @Deprecated - public static void clear(@Nonnull Hashtable.Entry[] buckets) { - Hashtable.clear(buckets); - } - - /** - * @deprecated use {@link Hashtable#bucketIterator(Hashtable.Entry[], long)}. - */ - @Deprecated - @Nonnull - public static BucketIterator bucketIterator( - @Nonnull Hashtable.Entry[] buckets, long keyHash) { - return Hashtable.bucketIterator(buckets, keyHash); - } - - /** - * @deprecated use {@link Hashtable#mutatingBucketIterator(Hashtable.Entry[], long)}. - */ - @Deprecated - @Nonnull - public static - MutatingBucketIterator mutatingBucketIterator( - @Nonnull Hashtable.Entry[] buckets, long keyHash) { - return Hashtable.mutatingBucketIterator(buckets, keyHash); - } - - /** - * @deprecated use {@link Hashtable#mutatingTableIterator(Hashtable.Entry[])}. - */ - @Deprecated - @Nonnull - public static - MutatingTableIterator mutatingTableIterator(@Nonnull Hashtable.Entry[] buckets) { - return Hashtable.mutatingTableIterator(buckets); - } - - /** - * @deprecated use {@link Hashtable#mutatingTableIterator(Hashtable.Entry[], int, int)}. - */ - @Deprecated - @Nonnull - public static - MutatingTableIterator mutatingTableIterator( - @Nonnull Hashtable.Entry[] buckets, int startBucket, int endBucket) { - return Hashtable.mutatingTableIterator(buckets, startBucket, endBucket); - } - - /** - * @deprecated use {@link Hashtable#bucketIndex(Object[], long)}. - */ - @Deprecated - public static int bucketIndex(@Nonnull Object[] buckets, long keyHash) { - return Hashtable.bucketIndex(buckets, keyHash); - } - - /** - * @deprecated use {@link Hashtable#insertHeadEntryAt(Hashtable.Entry[], int, Hashtable.Entry)}. - */ - @Deprecated - public static void insertHeadEntry( - @Nonnull Hashtable.Entry[] buckets, int bucketIndex, @Nonnull Hashtable.Entry entry) { - Hashtable.insertHeadEntryAt(buckets, bucketIndex, entry); - } - - /** - * @deprecated use {@link Hashtable#insertHeadEntryFor(Hashtable.Entry[], long, - * Hashtable.Entry)}. - */ - @Deprecated - public static void insertHeadEntry( - @Nonnull Hashtable.Entry[] buckets, long keyHash, @Nonnull Hashtable.Entry entry) { - Hashtable.insertHeadEntryFor(buckets, keyHash, entry); - } - - /** - * @deprecated use {@link Hashtable#bucketFor(Hashtable.Entry[], long)}. - */ - @Deprecated - @Nullable - public static TEntry bucket( - @Nonnull Hashtable.Entry[] buckets, long keyHash) { - return Hashtable.bucketFor(buckets, keyHash); - } - - /** - * @deprecated use {@link Hashtable#forEach(Hashtable.Entry[], Consumer)}. - */ - @Deprecated - public static void forEach( - @Nonnull Hashtable.Entry[] buckets, @Nonnull Consumer consumer) { - Hashtable.forEach(buckets, consumer); - } - - /** - * @deprecated use {@link Hashtable#forEach(Hashtable.Entry[], Object, BiConsumer)}. - */ - @Deprecated - public static void forEach( - @Nonnull Hashtable.Entry[] buckets, - C context, - @Nonnull BiConsumer consumer) { - Hashtable.forEach(buckets, context, consumer); - } - } - /** * Read-only iterator over entries in a single bucket whose {@code keyHash} matches a specific * search hash. Cheaper than {@link MutatingBucketIterator} because it does not track the diff --git a/internal-api/src/test/java/datadog/trace/util/HashtableTest.java b/internal-api/src/test/java/datadog/trace/util/HashtableTest.java index 91bead646fd..04b579a3c7f 100644 --- a/internal-api/src/test/java/datadog/trace/util/HashtableTest.java +++ b/internal-api/src/test/java/datadog/trace/util/HashtableTest.java @@ -14,7 +14,6 @@ import datadog.trace.util.Hashtable.BucketIterator; import datadog.trace.util.Hashtable.MutatingBucketIterator; import datadog.trace.util.Hashtable.MutatingTableIterator; -import datadog.trace.util.Hashtable.Support; import java.util.ArrayList; import java.util.HashSet; import java.util.List; @@ -202,143 +201,6 @@ void insertHeadEntrySplicesAsNewHead() { } } - // ============ Deprecated Support facade ============ - - /** - * The scaled {@code create(int, float)} factory and {@code MAX_RATIO} are deprecated-only: they - * have no blessed equivalent on {@link Hashtable} but remain in use by client-side statistics, so - * they keep dedicated coverage here. - */ - @Nested - @SuppressWarnings("deprecation") - class DeprecatedSupportTests { - - @Test - void maxRatioScalesTargetForLoadFactor() { - // 75% load factor => bucket array sized at requestedSize * 4/3, rounded up to power of 2. - // 12 * (4/3) = 16 entries, rounded up to power-of-2 length = 16. - assertEquals(4.0f / 3.0f, Support.MAX_RATIO); - Hashtable.Entry[] buckets = Support.create(12, Support.MAX_RATIO); - assertEquals(16, buckets.length); - } - - @Test - void createWithScaleRoundsUpToPowerOfTwo() { - // 7 * 1.5 = 10.5 -> (int) 10 -> sizeFor rounds up to next power-of-two = 16 - Hashtable.Entry[] buckets = Support.create(7, 1.5f); - assertEquals(16, buckets.length); - } - - @Test - void createWithoutScaleDelegatesToHashtableSizeFor() { - Hashtable.Entry[] buckets = Support.create(5); - assertEquals(Hashtable.create(StringIntEntry.class, 5).length, buckets.length); - } - - @Test - void clearDelegatesToHashtableClear() { - Hashtable.Entry[] buckets = Support.create(4); - buckets[0] = new StringIntEntry("a", 1); - Support.clear(buckets); - for (Hashtable.Entry b : buckets) { - assertNull(b); - } - } - - @Test - void bucketIndexDelegatesToHashtableBucketIndex() { - Hashtable.Entry[] buckets = Support.create(4); - long hash = StringIntEntry.hash("a"); - assertEquals(Hashtable.bucketIndex(buckets, hash), Support.bucketIndex(buckets, hash)); - } - - @Test - void insertHeadEntryByIndexDelegatesToHashtableInsertHeadEntryAt() { - Hashtable.Entry[] buckets = Support.create(4); - StringIntEntry entry = new StringIntEntry("a", 1); - Support.insertHeadEntry(buckets, 0, entry); - assertSame(entry, buckets[0]); - } - - @Test - void insertHeadEntryByHashDelegatesToHashtableInsertHeadEntryFor() { - Hashtable.Entry[] buckets = Support.create(4); - StringIntEntry entry = new StringIntEntry("a", 1); - Support.insertHeadEntry(buckets, entry.keyHash, entry); - assertSame(entry, Support.bucket(buckets, entry.keyHash)); - } - - @Test - void bucketDelegatesToHashtableBucketFor() { - Hashtable.Entry[] buckets = Support.create(4); - StringIntEntry entry = new StringIntEntry("a", 1); - Support.insertHeadEntry(buckets, entry.keyHash, entry); - assertSame(entry, Support.bucket(buckets, entry.keyHash)); - } - - @Test - void bucketIteratorDelegatesToHashtableBucketIterator() { - Hashtable.Entry[] buckets = Support.create(4); - StringIntEntry entry = new StringIntEntry("a", 1); - Support.insertHeadEntry(buckets, entry.keyHash, entry); - BucketIterator it = Support.bucketIterator(buckets, entry.keyHash); - assertTrue(it.hasNext()); - assertSame(entry, it.next()); - } - - @Test - void mutatingBucketIteratorDelegatesToHashtableMutatingBucketIterator() { - Hashtable.Entry[] buckets = Support.create(4); - StringIntEntry entry = new StringIntEntry("a", 1); - Support.insertHeadEntry(buckets, entry.keyHash, entry); - MutatingBucketIterator it = - Support.mutatingBucketIterator(buckets, entry.keyHash); - assertTrue(it.hasNext()); - assertSame(entry, it.next()); - it.remove(); - assertNull(Support.bucket(buckets, entry.keyHash)); - } - - @Test - void mutatingTableIteratorOverFullTableDelegatesToHashtable() { - Hashtable.Entry[] buckets = Support.create(4); - buckets[0] = new StringIntEntry("a", 1); - MutatingTableIterator it = Support.mutatingTableIterator(buckets); - assertTrue(it.hasNext()); - assertEquals("a", it.next().key); - } - - @Test - void mutatingTableIteratorOverRangeDelegatesToHashtable() { - Hashtable.Entry[] buckets = Support.create(4); - buckets[0] = new StringIntEntry("a", 1); - buckets[2] = new StringIntEntry("b", 2); - MutatingTableIterator it = Support.mutatingTableIterator(buckets, 0, 2); - assertTrue(it.hasNext()); - assertEquals("a", it.next().key); - assertFalse(it.hasNext(), "range end is exclusive"); - } - - @Test - void forEachDelegatesToHashtableForEach() { - Hashtable.Entry[] buckets = Support.create(4); - buckets[0] = new StringIntEntry("a", 1); - buckets[1] = new StringIntEntry("b", 2); - Set seen = new HashSet<>(); - Support.forEach(buckets, e -> seen.add(e.key)); - assertEquals(2, seen.size()); - } - - @Test - void forEachWithContextDelegatesToHashtableForEach() { - Hashtable.Entry[] buckets = Support.create(4); - buckets[0] = new StringIntEntry("a", 1); - Set seen = new HashSet<>(); - Support., StringIntEntry>forEach(buckets, seen, (ctx, e) -> ctx.add(e.key)); - assertEquals(1, seen.size()); - } - } - // ============ BucketIterator ============ @Nested From fa402d643e59b922b4f560eec0627e1aac405f2a Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Thu, 27 Aug 2026 13:09:20 -0400 Subject: [PATCH 10/13] Fuse the CardinalityLimitReporter counter bump into tryGetOrUpdate 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 --- .../common/metrics/CardinalityLimitReporter.java | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/common/metrics/CardinalityLimitReporter.java b/dd-trace-core/src/main/java/datadog/trace/common/metrics/CardinalityLimitReporter.java index 3b13a8800bd..ee16129a0a8 100644 --- a/dd-trace-core/src/main/java/datadog/trace/common/metrics/CardinalityLimitReporter.java +++ b/dd-trace-core/src/main/java/datadog/trace/common/metrics/CardinalityLimitReporter.java @@ -4,6 +4,7 @@ import datadog.logging.RatelimitedLogger; import datadog.trace.util.Hashtable; +import java.util.function.ObjLongConsumer; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -55,13 +56,15 @@ final class CardinalityLimitReporter { this.rlLog = rlLog; } - /** Records {@code count} values blocked for {@code tag} in the current reporting cycle. */ + /** + * Records {@code count} values blocked for {@code tag} in the current reporting cycle. + * + *

A {@code false} return -- the tag table is itself at capacity -- is deliberately ignored: + * this is a log sink, and the durable counts still reach {@code onTagCardinalityBlocked}. + */ void record(String tag, long count) { if (count > 0) { - TagBlockEntry entry = blockedByTag.tryGetOrCreateOrNull(tag, TagBlockEntry::new); - if (entry != null) { - entry.count += count; - } + blockedByTag.tryGetOrUpdate(tag, TagBlockEntry::new, count, ADD_BLOCKED); } } @@ -100,6 +103,9 @@ private String summarize() { /** * Single-key counter entry: the tag name (via {@link #key()}) plus its in-place-mutated count. */ + /** Non-capturing, so {@link #record} allocates nothing per call. */ + private static final ObjLongConsumer ADD_BLOCKED = (entry, n) -> entry.count += n; + private static final class TagBlockEntry extends Hashtable.D1.Entry { long count; From 7a615358f04c15251b2c2af18ee7a0d83e6c9409 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Thu, 27 Aug 2026 13:09:22 -0400 Subject: [PATCH 11/13] Rewrap an AggregateTable javadoc paragraph per spotless Co-Authored-By: Claude Opus 5 --- .../java/datadog/trace/common/metrics/AggregateTable.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java index dd03187ad55..d7d726935f3 100644 --- a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java +++ b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java @@ -69,10 +69,10 @@ boolean isEmpty() { /** * Returns the {@link AggregateEntry} to update for {@code snapshot}, lazily creating one on miss. * Returns {@code null} when the table is at capacity and no stale entry can be evicted -- the - * caller should drop the data point in that case (reported via {@code - * onStatsAggregateDropped}). Dropping the new key rather than evicting an established one is - * deliberate: the cap is sized to the steady-state working set, so a full table of entries that - * were all used this cycle means the new key is the outlier. + * caller should drop the data point in that case (reported via {@code onStatsAggregateDropped}). + * Dropping the new key rather than evicting an established one is deliberate: the cap is sized to + * the steady-state working set, so a full table of entries that were all used this cycle means + * the new key is the outlier. * *

Cardinality limiting (see {@link MetricCardinalityLimits#USE_BLOCKED_SENTINEL}) reduces how * often eviction fires but doesn't eliminate it. Over-cap values for a single field collapse into From 4d41f3f220c11267cd397d2bd72c276a6b4b5da1 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 28 Aug 2026 14:49:30 -0400 Subject: [PATCH 12/13] Fuse CardinalityLimitReporter.record via Maybe, inline the count-bump as a method reference record() now uses tryGetOrCreate(...).update(...) instead of the older tryGetOrUpdate helper, and the mutator is TagBlockEntry::inc -- an unbound method reference, non-capturing like the static-final lambda it replaces -- so nothing changes on the allocation front. Added a JMH benchmark as the acceptance check that steady-state record() stays allocation-free. --- .../CardinalityLimitReporterBenchmark.java | 61 +++++++++++++++++++ .../metrics/CardinalityLimitReporter.java | 14 ++--- 2 files changed, 68 insertions(+), 7 deletions(-) create mode 100644 dd-trace-core/src/jmh/java/datadog/trace/common/metrics/CardinalityLimitReporterBenchmark.java diff --git a/dd-trace-core/src/jmh/java/datadog/trace/common/metrics/CardinalityLimitReporterBenchmark.java b/dd-trace-core/src/jmh/java/datadog/trace/common/metrics/CardinalityLimitReporterBenchmark.java new file mode 100644 index 00000000000..6566216153f --- /dev/null +++ b/dd-trace-core/src/jmh/java/datadog/trace/common/metrics/CardinalityLimitReporterBenchmark.java @@ -0,0 +1,61 @@ +package datadog.trace.common.metrics; + +import java.util.concurrent.ThreadLocalRandom; +import java.util.concurrent.TimeUnit; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Level; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +/** + * Steady-state {@code record()} acceptance check: once every tag in the working set has an entry, + * every call should be a lookup + in-place count bump through the {@link + * datadog.trace.util.Hashtable.D1#tryGetOrCreate} {@code Maybe}, with no per-call allocation. Run + * with {@code -prof gc} -- B/op should read ~0. + * + *

Not thread-safe by design (see {@link CardinalityLimitReporter}'s class javadoc), so each + * thread gets its own reporter and tag pool rather than sharing one instance. + */ +@State(Scope.Thread) +@BenchmarkMode(Mode.Throughput) +@OutputTimeUnit(TimeUnit.SECONDS) +@Warmup(iterations = 5, time = 2) +@Measurement(iterations = 5, time = 2) +@Fork(3) +@Threads(8) +public class CardinalityLimitReporterBenchmark { + + private static final int DISTINCT_TAGS = 32; + + private CardinalityLimitReporter reporter; + private String[] tags; + private int cursor; + + @Setup(Level.Trial) + public void setup() { + this.reporter = new CardinalityLimitReporter(); + this.tags = new String[DISTINCT_TAGS]; + for (int i = 0; i < DISTINCT_TAGS; i++) { + tags[i] = "tag-" + i; + } + // Pre-populate every entry so the measured path is pure lookup + update, not creation. + for (String tag : tags) { + reporter.record(tag, 1); + } + } + + @Benchmark + public void record() { + String tag = tags[cursor++ & (DISTINCT_TAGS - 1)]; + long count = 1L + (ThreadLocalRandom.current().nextLong() & 0xFF); + reporter.record(tag, count); + } +} diff --git a/dd-trace-core/src/main/java/datadog/trace/common/metrics/CardinalityLimitReporter.java b/dd-trace-core/src/main/java/datadog/trace/common/metrics/CardinalityLimitReporter.java index ee16129a0a8..307f6c5492c 100644 --- a/dd-trace-core/src/main/java/datadog/trace/common/metrics/CardinalityLimitReporter.java +++ b/dd-trace-core/src/main/java/datadog/trace/common/metrics/CardinalityLimitReporter.java @@ -4,7 +4,6 @@ import datadog.logging.RatelimitedLogger; import datadog.trace.util.Hashtable; -import java.util.function.ObjLongConsumer; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -59,12 +58,12 @@ final class CardinalityLimitReporter { /** * Records {@code count} values blocked for {@code tag} in the current reporting cycle. * - *

A {@code false} return -- the tag table is itself at capacity -- is deliberately ignored: - * this is a log sink, and the durable counts still reach {@code onTagCardinalityBlocked}. + *

A refused create -- the tag table is itself at capacity -- is deliberately ignored: this is + * a log sink, and the durable counts still reach {@code onTagCardinalityBlocked}. */ void record(String tag, long count) { if (count > 0) { - blockedByTag.tryGetOrUpdate(tag, TagBlockEntry::new, count, ADD_BLOCKED); + blockedByTag.tryGetOrCreate(tag, TagBlockEntry::new).update(count, TagBlockEntry::inc); } } @@ -103,14 +102,15 @@ private String summarize() { /** * Single-key counter entry: the tag name (via {@link #key()}) plus its in-place-mutated count. */ - /** Non-capturing, so {@link #record} allocates nothing per call. */ - private static final ObjLongConsumer ADD_BLOCKED = (entry, n) -> entry.count += n; - private static final class TagBlockEntry extends Hashtable.D1.Entry { long count; TagBlockEntry(String tag) { super(tag); } + + void inc(long n) { + count += n; + } } } From ab7ec5369b69086f86b839d239d5ee4594476cb6 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 28 Aug 2026 14:49:54 -0400 Subject: [PATCH 13/13] Inline AggregateEntry::isStale at its call sites, drop the STALE field Same non-capturing-method-reference reasoning as CardinalityLimitReporter's ADD_BLOCKED: an unbound method reference is cached the same way a static final field would be, so the STALE field bought nothing. Also trims isStale's javadoc now that it no longer needs to justify a static-field pattern that's gone. --- .../trace/common/metrics/AggregateEntry.java | 5 +---- .../trace/common/metrics/AggregateTable.java | 13 +++---------- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java index 646725636ec..41f280faee7 100644 --- a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java +++ b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateEntry.java @@ -246,10 +246,7 @@ public int getHitCount() { /** * {@code true} if nothing hit this entry in the current reporting cycle, making it the first - * thing worth evicting when the table is full. Encapsulates the staleness rule on the entry so - * the table doesn't have to know it is spelled {@code hitCount == 0}, and reads as {@code - * AggregateEntry::isStale} at an eviction call site -- an unbound method reference, so it is - * non-capturing and costs no allocation. + * thing worth evicting when the table is full. */ public boolean isStale() { return hitCount == 0; diff --git a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java index d7d726935f3..1984e8f4f20 100644 --- a/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java +++ b/dd-trace-core/src/main/java/datadog/trace/common/metrics/AggregateTable.java @@ -4,7 +4,6 @@ import datadog.trace.util.Hashtable; import java.util.function.BiConsumer; import java.util.function.Consumer; -import java.util.function.Predicate; /** * The {@link AggregateEntry} store of the consuming aggregator thread, keyed on the canonical @@ -25,12 +24,6 @@ */ final class AggregateTable { - /** - * Stale means "not used in this reporting cycle". Held as a {@code static final} so it is a - * non-capturing singleton rather than a fresh lambda per eviction. - */ - private static final Predicate STALE = AggregateEntry::isStale; - private final Hashtable.State state; private final AggregateEntry.Canonical canonical; @@ -81,7 +74,7 @@ boolean isEmpty() { * can still drive the entry count to {@code maxAggregates}, so eviction remains the backstop. * *

The scan that finds a stale entry, and its resume-where-it-left-off amortization, live in - * {@link Hashtable#tryReserveOrEvict} -- this class only supplies {@link #STALE}. + * {@link Hashtable#tryReserveOrEvict} -- this class only supplies {@link AggregateEntry#isStale}. */ AggregateEntry findOrInsert(SpanSnapshot snapshot) { canonical.populateFrom(snapshot); @@ -95,7 +88,7 @@ AggregateEntry findOrInsert(SpanSnapshot snapshot) { } // Miss path. Reserve before building the entry so a refused insert costs no allocation; the // reservation evicts a stale entry to make room if the table is already full. - if (!Hashtable.tryReserveOrEvict(state, STALE)) { + if (!Hashtable.tryReserveOrEvict(state, AggregateEntry::isStale)) { return null; } AggregateEntry entry = canonical.createEntry(); @@ -118,7 +111,7 @@ void forEach(C context, BiConsumer consumer) { /** Removes entries whose {@code getHitCount() == 0}. */ void expungeStaleAggregates() { - Hashtable.evictAll(state, STALE); + Hashtable.evictAll(state, AggregateEntry::isStale); } void clear() {