From 9d2f878bdf6b2eed205cd4111f00b1b463d03dc6 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Thu, 10 Sep 2026 14:45:47 -0400 Subject: [PATCH 1/9] Encapsulate ConcurrentHashtable.SizeManager and relocate createBounded State.sizeManager was a public field, letting callers outside this class reach into SizeManager directly instead of going through the State-accepting static helpers. Narrow it to package-private and add the missing tryReserve(State) wrapper so external callers (e.g. an upcoming ConcurrentHashtable consumer) have a sanctioned lock-free reservation entry point instead of needing sizeManager exposed. Also move createBounded from the nested State class onto ConcurrentHashtable directly for a nicer call site, per review discussion on PR #12367. Co-Authored-By: Claude Sonnet 5 --- .../trace/util/ConcurrentHashtable.java | 41 +++++++++++++------ .../ConcurrentHashtableSizeManagerTest.java | 28 ++++++------- 2 files changed, 43 insertions(+), 26 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java b/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java index 693ca2a1b16..0be691a00a0 100644 --- a/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java +++ b/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java @@ -151,7 +151,7 @@ private D1(State state) { @Nonnull public static > D1 createBounded( @Nonnull Class entryClass, int maxCapacity) { - return new D1<>(State.createBounded(entryClass, maxCapacity)); + return new D1<>(ConcurrentHashtable.createBounded(entryClass, maxCapacity)); } public int size() { @@ -414,7 +414,7 @@ private D2(State state) { @Nonnull public static > D2 createBounded( @Nonnull Class entryClass, int maxCapacity) { - return new D2<>(State.createBounded(entryClass, maxCapacity)); + return new D2<>(ConcurrentHashtable.createBounded(entryClass, maxCapacity)); } public int size() { @@ -812,25 +812,30 @@ public int evictAll( /** * Bucket array and occupancy manager for a caller-defined capped table. Keep them paired and * prefer the {@code State}-accepting helpers so structural changes update the count consistently. + * + *

{@code sizeManager} is intentionally package-private: callers outside this class must go + * through the {@code State}-accepting static helpers ({@link #estimateSize}, {@link #isFull}, + * {@link #tryReserve}, {@link #tryReserveOrEvict}, {@link #evictOne}, {@link #evictAll}) rather + * than reach into the manager directly. */ public static final class State { public final AtomicReferenceArray buckets; - public final SizeManager sizeManager; + final SizeManager sizeManager; private State(AtomicReferenceArray buckets, int maxCapacity) { this.buckets = buckets; this.sizeManager = new SizeManager(maxCapacity); } + } - /** - * Creates a bucket array for {@code maxCapacity} entries and pairs it with a manager enforcing - * that cap. {@code entryClass} is used only to infer {@code TEntry}. - */ - @Nonnull - public static State createBounded( - @Nonnull Class entryClass, int maxCapacity) { - return new State<>(createFixedBuckets(entryClass, maxCapacity), maxCapacity); - } + /** + * Creates a bucket array for {@code maxCapacity} entries and pairs it with a manager enforcing + * that cap. {@code entryClass} is used only to infer {@code TEntry}. + */ + @Nonnull + public static State createBounded( + @Nonnull Class entryClass, int maxCapacity) { + return new State<>(createFixedBuckets(entryClass, maxCapacity), maxCapacity); } /** Live entries in {@code state}; see {@link SizeManager#estimateSize()}. Lock-free. */ @@ -845,6 +850,18 @@ public static boolean isFull(@Nonnull State state) { return state.sizeManager.isFull(); } + /** + * Reserves one slot in {@code state} without evicting; see {@link SizeManager#tryReserve()}. + * Lock-free — does not acquire the table write lock. Returns {@code false} with the table + * unchanged when it is full. + * + *

Build the entry before reserving: there is no cancellation operation, so abandoning a + * successful reservation permanently consumes capacity. Complete it with {@link #insertReserved}. + */ + public static boolean tryReserve(@Nonnull State state) { + return state.sizeManager.tryReserve(); + } + /** * Reserves one slot in {@code state}, evicting an entry matching {@code evictable} when * necessary. Returns {@code false} if the table is full and nothing can be evicted. This method diff --git a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableSizeManagerTest.java b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableSizeManagerTest.java index 24c581b2380..35bb9cc24de 100644 --- a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableSizeManagerTest.java +++ b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableSizeManagerTest.java @@ -40,7 +40,7 @@ void tryReserveSucceedsUnderCapacityAndFailsWhenFull() { @Test void tryReserveOrEvictReservesDirectlyWhenUnderCapacity() { ConcurrentHashtable.State state = - ConcurrentHashtable.State.createBounded(TestEntry.class, 2); + ConcurrentHashtable.createBounded(TestEntry.class, 2); boolean reserved = tryReserveOrEvict(state, e -> true); assertTrue(reserved); @@ -51,7 +51,7 @@ void tryReserveOrEvictReservesDirectlyWhenUnderCapacity() { @Test void tryReserveOrEvictEvictsWhenFullAndSomethingMatches() { ConcurrentHashtable.State state = - ConcurrentHashtable.State.createBounded(TestEntry.class, 1); + ConcurrentHashtable.createBounded(TestEntry.class, 1); TestEntry existing = insertAt(state, 0, "existing"); assertTrue(state.sizeManager.tryReserve()); assertTrue(state.sizeManager.isFull()); @@ -65,7 +65,7 @@ void tryReserveOrEvictEvictsWhenFullAndSomethingMatches() { @Test void tryReserveOrEvictFailsAndLeavesTableUntouchedWhenNothingEvictable() { ConcurrentHashtable.State state = - ConcurrentHashtable.State.createBounded(TestEntry.class, 1); + ConcurrentHashtable.createBounded(TestEntry.class, 1); TestEntry existing = insertAt(state, 0, "existing"); assertTrue(state.sizeManager.tryReserve()); @@ -78,7 +78,7 @@ void tryReserveOrEvictFailsAndLeavesTableUntouchedWhenNothingEvictable() { @Test void insertReservedSplicesWithoutTouchingTheCountAfterATryReserve() { ConcurrentHashtable.State state = - ConcurrentHashtable.State.createBounded(TestEntry.class, 2); + ConcurrentHashtable.createBounded(TestEntry.class, 2); assertTrue(state.sizeManager.tryReserve()); assertEquals(1, state.sizeManager.estimateSize()); @@ -95,7 +95,7 @@ void insertReservedSplicesWithoutTouchingTheCountAfterATryReserve() { @Test void evictOneReturnsNullAndLeavesCountUnchangedWhenNothingMatches() { ConcurrentHashtable.State state = - ConcurrentHashtable.State.createBounded(TestEntry.class, 4); + ConcurrentHashtable.createBounded(TestEntry.class, 4); insertAt(state, 0, "a"); state.sizeManager.increment(); @@ -107,7 +107,7 @@ void evictOneReturnsNullAndLeavesCountUnchangedWhenNothingMatches() { @Test void evictOneUnlinksMatchAndDecrementsCount() { ConcurrentHashtable.State state = - ConcurrentHashtable.State.createBounded(TestEntry.class, 4); + ConcurrentHashtable.createBounded(TestEntry.class, 4); TestEntry a = insertAt(state, 0, "a"); TestEntry b = insertAt(state, 1, "b"); state.sizeManager.increment(); @@ -130,7 +130,7 @@ void evictOneUnlinksMatchAndDecrementsCount() { void evictOneResumesFromLastEvictedBucketAndWrapsAround() { // Bucket-array length 4: keyHash i lands in bucket i. ConcurrentHashtable.State state = - ConcurrentHashtable.State.createBounded(TestEntry.class, 4); + ConcurrentHashtable.createBounded(TestEntry.class, 4); TestEntry e0 = insertAt(state, 0, "e0"); insertAt(state, 2, "e2"); TestEntry e3 = insertAt(state, 3, "e3"); @@ -159,7 +159,7 @@ void evictOneResumesFromLastEvictedBucketAndWrapsAround() { @Test void evictAllRemovesEveryMatchAndReturnsCount() { ConcurrentHashtable.State state = - ConcurrentHashtable.State.createBounded(TestEntry.class, 8); + ConcurrentHashtable.createBounded(TestEntry.class, 8); for (int i = 0; i < 6; i++) { insertAt(state, i, "e" + i); state.sizeManager.increment(); @@ -179,7 +179,7 @@ void evictAllRemovesEveryMatchAndReturnsCount() { @Test void evictAllResetsCursorSoSubsequentEvictOneScansFromBucketZero() { ConcurrentHashtable.State state = - ConcurrentHashtable.State.createBounded(TestEntry.class, 4); + ConcurrentHashtable.createBounded(TestEntry.class, 4); insertAt(state, 2, "a"); state.sizeManager.increment(); // Advance the cursor away from 0 via a successful eviction at bucket 2. @@ -203,7 +203,7 @@ void evictAllResetsCursorSoSubsequentEvictOneScansFromBucketZero() { @Test void releaseGivesBackRemovedSlotsAndRestartsScan() { ConcurrentHashtable.State state = - ConcurrentHashtable.State.createBounded(TestEntry.class, 4); + ConcurrentHashtable.createBounded(TestEntry.class, 4); insertAt(state, 2, "a"); state.sizeManager.increment(); evictOne(state, e -> true); // advances the cursor to 2, count back to 0 @@ -226,7 +226,7 @@ void releaseGivesBackRemovedSlotsAndRestartsScan() { @Test void stateCreateCappedBundlesBucketsAndSizeManager() { ConcurrentHashtable.State state = - ConcurrentHashtable.State.createBounded(TestEntry.class, 3); + ConcurrentHashtable.createBounded(TestEntry.class, 3); assertEquals(0, state.sizeManager.estimateSize()); assertEquals(3, state.sizeManager.capacity()); assertTrue(state.buckets.length() >= 3); @@ -235,7 +235,7 @@ void stateCreateCappedBundlesBucketsAndSizeManager() { @Test void stateLevelTryReserveOrEvictAndEvictOneAndEvictAllDelegateToSizeManager() { ConcurrentHashtable.State state = - ConcurrentHashtable.State.createBounded(TestEntry.class, 1); + ConcurrentHashtable.createBounded(TestEntry.class, 1); synchronized (ConcurrentHashtable.getWriteLockAt(state, 0)) { ConcurrentHashtable.insertHeadEntryAt(state, 0, new TestEntry(0, "a")); } @@ -280,7 +280,7 @@ void stateLevelTryReserveOrEvictAndEvictOneAndEvictAllDelegateToSizeManager() { @Test void clearCannotInterleaveBetweenReservationAndInsert() throws InterruptedException { ConcurrentHashtable.State state = - ConcurrentHashtable.State.createBounded(TestEntry.class, 1); + ConcurrentHashtable.createBounded(TestEntry.class, 1); insertAt(state, 0, "a"); state.sizeManager.increment(); assertTrue(ConcurrentHashtable.isFull(state)); @@ -318,7 +318,7 @@ void clearCannotInterleaveBetweenReservationAndInsert() throws InterruptedExcept @Test void reservationSurvivesAClearLandingBetweenReserveAndInsert() { ConcurrentHashtable.State state = - ConcurrentHashtable.State.createBounded(TestEntry.class, 2); + ConcurrentHashtable.createBounded(TestEntry.class, 2); insertAt(state, 0, "a"); state.sizeManager.increment(); From 52065cef63707124b2acb304765f53e52bdf08b4 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Thu, 10 Sep 2026 16:59:28 -0400 Subject: [PATCH 2/9] Add self-bounded Entry generics and a find-or-insert Reservation API to ConcurrentHashtable Entry> now forces every concrete entry to implement matches(TEntry), replacing ad-hoc equals()/hashCode() reuse. ConcurrentHashtable.reserve(state) returns an AutoCloseable Reservation that claims a slot lock-free, defers building the entry until the reservation succeeds, and auto-cancels an unconsumed slot on close() - collapsing the racy hand-rolled dedup pattern LogCollector used to need into a single try-with-resources call. Overloaded up to 4 key components (Function..Function4) so callers can pass a non-capturing constructor reference instead of a capturing lambda. LogCollector is ported onto this API illustratively, to see the resulting ergonomics against a real caller. Co-Authored-By: Claude Sonnet 5 --- .../util/ThreadSafeMapCounterBenchmark.java | 2 +- .../trace/util/ThreadSafeMapD1Benchmark.java | 2 +- .../trace/util/ThreadSafeMapD2Benchmark.java | 9 +- .../trace/api/telemetry/LogCollector.java | 105 +++--- .../trace/util/ConcurrentHashtable.java | 305 ++++++++++++++---- .../api/telemetry/LogCollectorTest.groovy | 5 +- .../trace/util/ConcurrentHashtableD1Test.java | 5 +- .../trace/util/ConcurrentHashtableD2Test.java | 3 +- .../ConcurrentHashtableReservationTest.java | 162 ++++++++++ .../ConcurrentHashtableSizeManagerTest.java | 7 +- .../util/ConcurrentHashtableStaticsTest.java | 7 +- 11 files changed, 498 insertions(+), 114 deletions(-) create mode 100644 internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableReservationTest.java diff --git a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapCounterBenchmark.java b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapCounterBenchmark.java index bc8bc07b9f5..7bdf3e1cdea 100644 --- a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapCounterBenchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapCounterBenchmark.java @@ -73,7 +73,7 @@ public class ThreadSafeMapCounterBenchmark { } } - static final class CounterEntry extends ConcurrentHashtable.D1.Entry { + static final class CounterEntry extends ConcurrentHashtable.D1.Entry { private static final AtomicLongFieldUpdater COUNT = AtomicLongFieldUpdater.newUpdater(CounterEntry.class, "count"); diff --git a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD1Benchmark.java b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD1Benchmark.java index 8fd07544264..0c7a1472772 100644 --- a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD1Benchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD1Benchmark.java @@ -83,7 +83,7 @@ public class ThreadSafeMapD1Benchmark { } } - static final class D1Entry extends ConcurrentHashtable.D1.Entry { + static final class D1Entry extends ConcurrentHashtable.D1.Entry { final long value; D1Entry(String key) { diff --git a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD2Benchmark.java b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD2Benchmark.java index aff30dd0a33..963de93f8ae 100644 --- a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD2Benchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD2Benchmark.java @@ -95,7 +95,7 @@ public class ThreadSafeMapD2Benchmark { } } - static final class D2Entry extends ConcurrentHashtable.D2.Entry { + static final class D2Entry extends ConcurrentHashtable.D2.Entry { final long value; D2Entry(String k1, Integer k2) { @@ -108,7 +108,7 @@ static final class D2Entry extends ConcurrentHashtable.D2.Entry * Entry used with the static helpers. Its primitive second key keeps storage and lookup unboxed, * independently of {@link Integer} caching or JVM escape analysis. */ - static final class SupportEntry extends ConcurrentHashtable.Entry { + static final class SupportEntry extends ConcurrentHashtable.Entry { final String k1; final int k2; final long value; @@ -127,6 +127,11 @@ static long hash(String k1, int k2) { boolean matches(String k1, int k2) { return this.k2 == k2 && this.k1.equals(k1); } + + @Override + public boolean matches(SupportEntry other) { + return matches(other.k1, other.k2); + } } /** Composite key for map-based baselines. */ diff --git a/internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java b/internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java index b7ad3cb0eb0..70c121c98ae 100644 --- a/internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java +++ b/internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java @@ -1,16 +1,15 @@ package datadog.trace.api.telemetry; +import datadog.trace.util.ConcurrentHashtable; import datadog.trace.util.HashingUtils; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.Iterator; import java.util.List; -import java.util.Map; import java.util.Objects; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicInteger; +import javax.annotation.Nonnull; import javax.annotation.Nullable; import org.slf4j.Marker; import org.slf4j.MarkerFactory; @@ -20,8 +19,7 @@ public class LogCollector { public static final Marker EXCLUDE_TELEMETRY = MarkerFactory.getMarker("EXCLUDE_TELEMETRY"); private static final int DEFAULT_MAX_CAPACITY = 10; private static final LogCollector INSTANCE = new LogCollector(); - private final Map rawLogMessages; - private final int maxCapacity; + private final ConcurrentHashtable.State rawLogMessages; public static LogCollector get() { return INSTANCE; @@ -35,8 +33,7 @@ private LogCollector() { value = "SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR", justification = "Usage in tests") LogCollector(int maxCapacity) { - this.maxCapacity = maxCapacity; - this.rawLogMessages = new ConcurrentHashMap<>(maxCapacity); + this.rawLogMessages = ConcurrentHashtable.createBounded(RawLogMessage.class, maxCapacity); } public void addLogMessage(String logLevel, String message, @Nullable Throwable throwable) { @@ -54,57 +51,62 @@ public void addLogMessage(String logLevel, String message, @Nullable Throwable t */ public void addLogMessage( String logLevel, String message, @Nullable Throwable throwable, @Nullable String tags) { - if (rawLogMessages.size() >= maxCapacity) { - // TODO: We could emit a metric for dropped logs. - return; + long keyHash = RawLogMessage.hash(logLevel, message, throwable); + + // Lock-free scan first: most calls are re-observations of an already-seen message, so this + // avoids paying for a reservation and a RawLogMessage allocation on the common path. + for (RawLogMessage existing = ConcurrentHashtable.bucketFor(rawLogMessages, keyHash); + existing != null; + existing = existing.next()) { + if (existing.keyHash == keyHash && existing.matchesKey(logLevel, message, throwable)) { + existing.count.incrementAndGet(); + return; + } + } + + try (ConcurrentHashtable.Reservation reservation = + ConcurrentHashtable.reserve(rawLogMessages)) { + // TODO: We could emit a metric for dropped logs when the reservation is empty (table full). + RawLogMessage rawLogMessage = + reservation.tryGetOrInsertOrNull(RawLogMessage::new, logLevel, message, throwable, tags); + if (rawLogMessage != null) { + rawLogMessage.count.incrementAndGet(); + } } - RawLogMessage rawLogMessage = - new RawLogMessage(logLevel, message, throwable, tags, System.currentTimeMillis() / 1000); - AtomicInteger count = rawLogMessages.computeIfAbsent(rawLogMessage, k -> new AtomicInteger()); - count.incrementAndGet(); } public Collection drain() { - if (rawLogMessages.isEmpty()) { + if (ConcurrentHashtable.estimateSize(rawLogMessages) == 0) { return Collections.emptyList(); } - List list = new ArrayList<>(rawLogMessages.size()); - Iterator> iterator = - rawLogMessages.entrySet().iterator(); - - while (iterator.hasNext()) { - Map.Entry entry = iterator.next(); - RawLogMessage logMessage = entry.getKey(); - // XXX: There might be lost writers to the counters under concurrency if another thread - // increments it - // while we are reading it here. At the moment, we are not overdoing this to prevent some - // counter losses. - logMessage.count = entry.getValue().get(); - iterator.remove(); - list.add(logMessage); - } - + List list = new ArrayList<>(ConcurrentHashtable.estimateSize(rawLogMessages)); + ConcurrentHashtable.drain(rawLogMessages, list::add); return list; } - public static final class RawLogMessage { + public static final class RawLogMessage extends ConcurrentHashtable.Entry { public final String message; public final String logLevel; public final Throwable throwable; public final String tags; public final long timestamp; - public int count; + public final AtomicInteger count = new AtomicInteger(); private StackTraceElement[] cachedStackTrace = null; public RawLogMessage( - String logLevel, String message, Throwable throwable, String tags, long timestamp) { + String logLevel, String message, Throwable throwable, @Nullable String tags) { + super(hash(logLevel, message, throwable)); this.logLevel = logLevel; this.message = message; this.throwable = throwable; this.tags = tags; - this.timestamp = timestamp; + this.timestamp = System.currentTimeMillis() / 1000; + } + + static long hash(String logLevel, String message, @Nullable Throwable throwable) { + return HashingUtils.hash(logLevel, message, throwable == null ? null : throwable.getClass()); } public StackTraceElement[] stackTrace() { @@ -122,25 +124,20 @@ public StackTraceElement[] stackTrace() { return stackTrace; } - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - RawLogMessage that = (RawLogMessage) o; - - if (!Objects.equals(logLevel, that.logLevel)) return false; - if (!Objects.equals(message, that.message)) return false; + private boolean matchesKey(String logLevel, String message, @Nullable Throwable throwable) { + if (!Objects.equals(this.logLevel, logLevel)) return false; + if (!Objects.equals(this.message, message)) return false; - if (throwable == that.throwable) { + if (this.throwable == throwable) { // DQH - While this path may seem unlikely, it does happen if the JVM fast // throws optimization kicks-in (for NPE, etc), so this case is worth optimizing. // This also covers the case where both throwables are null return true; - } else if (throwable != null && that.throwable != null) { + } else if (this.throwable != null && throwable != null) { // Both have a throwable perform a deeper comparison - return throwable.getClass().equals(that.throwable.getClass()) - && Objects.deepEquals(stackTrace(), that.stackTrace()); + return this.throwable.getClass().equals(throwable.getClass()) + && Objects.deepEquals(stackTrace(), throwable.getStackTrace()); } else { // One has an exception & the other doesn't, not equal return false; @@ -148,8 +145,18 @@ public boolean equals(Object o) { } @Override - public int hashCode() { - return HashingUtils.hash(logLevel, message, throwable == null ? null : throwable.getClass()); + public boolean matches(@Nonnull RawLogMessage other) { + if (!Objects.equals(logLevel, other.logLevel)) return false; + if (!Objects.equals(message, other.message)) return false; + + if (throwable == other.throwable) { + return true; + } else if (throwable != null && other.throwable != null) { + return throwable.getClass().equals(other.throwable.getClass()) + && Objects.deepEquals(stackTrace(), other.stackTrace()); + } else { + return false; + } } } } diff --git a/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java b/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java index 0be691a00a0..36077332a52 100644 --- a/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java +++ b/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java @@ -69,10 +69,16 @@ private ConcurrentHashtable() {} * arity. See {@link D1.Entry} and {@link D2.Entry}; for higher arities, or for primitive key * components, subclass this directly and drive the table with the static building blocks on * {@link ConcurrentHashtable}. + * + *

The self-bound type parameter ({@code TEntry extends Entry}, mirroring {@code Enum>}) exists so {@link #matches(Entry)} can compare two already-built entries + * without an {@code Object}/{@code instanceof} boundary. It requires every concrete subclass to + * declare itself as its own type argument (see {@link D1.Entry}/{@link D2.Entry}); Java has no + * true {@code Self} type, so this is enforced by convention, not the compiler. */ - public abstract static class Entry { + public abstract static class Entry> { public final long keyHash; - private volatile Entry next = null; + private volatile Entry next = null; protected Entry(long keyHash) { this.keyHash = keyHash; @@ -81,33 +87,48 @@ protected Entry(long keyHash) { // Package-private: the only writers are the static insert/remove building blocks // (insertHeadEntry, unlink) on the enclosing class, which reach it via the Entry bound. Custom // tables mutate chains through those helpers, never by touching next directly. - final void setNext(TEntry next) { + final > void setNext(TNext next) { this.next = next; } @SuppressWarnings("unchecked") @Nullable - public final TEntry next() { - return (TEntry) this.next; + public final > TNext next() { + return (TNext) this.next; } + + /** + * Returns {@code true} if {@code other} is logically the same entry as this one (same key(s)), + * used to compare two already-built entries under the write lock (e.g. in {@link + * Reservation#tryGetOrInsertOrNull}). Deliberately narrower than {@code equals}/{@code + * hashCode}: this class doesn't need reflexive/symmetric-with-null-and-unrelated-types contract + * baggage, and a bespoke method avoids entries accidentally working as {@code HashSet}/{@code + * HashMap} keys via an unrelated identity notion. {@link D1.Entry}/{@link D2.Entry} implement + * this in terms of their existing key-based {@code matches(...)}, so most callers never write + * it directly. + */ + public abstract boolean matches(@Nonnull TEntry other); } /** * Single-key concurrent hash table. Lock-free on hit; locked on miss/mutation. * * @param the key type - * @param the user's {@link D1.Entry D1.Entry<K>} subclass + * @param the user's {@link D1.Entry D1.Entry<K, TEntry>} subclass */ @ThreadSafe - public static final class D1> { + public static final class D1> { /** * Abstract base for {@link D1} entries. Subclass to add value fields you wish to mutate in * place after retrieving the entry via {@link D1#get}. * * @param the key type + * @param the concrete subclass extending this one (self-bound, see {@link + * ConcurrentHashtable.Entry}) */ - public abstract static class Entry extends ConcurrentHashtable.Entry { + public abstract static class Entry> + extends ConcurrentHashtable.Entry { final K key; protected Entry(@Nullable K key) { @@ -127,6 +148,12 @@ public boolean matches(@Nullable Object key) { return Objects.equals(key, this.key); } + /** {@link ConcurrentHashtable.Entry#matches(Entry)} in terms of the key-based overload. */ + @Override + public boolean matches(@Nonnull TEntry other) { + return matches(other.key); + } + /** * Returns the 64-bit lookup hash for {@code key}. Null keys map to {@link Long#MIN_VALUE} so * they don't collide with a real key that hashes to 0; real-key collisions in chains are @@ -149,7 +176,7 @@ private D1(State state) { * the insertion methods. The table does not resize. */ @Nonnull - public static > D1 createBounded( + public static > D1 createBounded( @Nonnull Class entryClass, int maxCapacity) { return new D1<>(ConcurrentHashtable.createBounded(entryClass, maxCapacity)); } @@ -354,10 +381,10 @@ public void forEach(C context, @Nonnull BiConsumer first key type * @param second key type - * @param the user's {@link D2.Entry D2.Entry<K1, K2>} subclass + * @param the user's {@link D2.Entry D2.Entry<K1, K2, TEntry>} subclass */ @ThreadSafe - public static final class D2> { + public static final class D2> { /** * Abstract base for {@link D2} entries. Subclass to add value fields you wish to mutate in @@ -365,8 +392,11 @@ public static final class D2> { * * @param first key type * @param second key type + * @param the concrete subclass extending this one (self-bound, see {@link + * ConcurrentHashtable.Entry}) */ - public abstract static class Entry extends ConcurrentHashtable.Entry { + public abstract static class Entry> + extends ConcurrentHashtable.Entry { final K1 key1; final K2 key2; @@ -394,6 +424,12 @@ public boolean matches(@Nullable K1 key1, @Nullable K2 key2) { return Objects.equals(key1, this.key1) && Objects.equals(key2, this.key2); } + /** {@link ConcurrentHashtable.Entry#matches(Entry)} in terms of the key-based overload. */ + @Override + public boolean matches(@Nonnull TEntry other) { + return matches(other.key1, other.key2); + } + /** Returns the 64-bit lookup hash combining both key parts via {@link LongHashingUtils}. */ public static long hash(@Nullable Object key1, @Nullable Object key2) { return LongHashingUtils.hash(key1, key2); @@ -412,8 +448,8 @@ private D2(State state) { * the insertion methods. The table does not resize. */ @Nonnull - public static > D2 createBounded( - @Nonnull Class entryClass, int maxCapacity) { + public static > + D2 createBounded(@Nonnull Class entryClass, int maxCapacity) { return new D2<>(ConcurrentHashtable.createBounded(entryClass, maxCapacity)); } @@ -663,8 +699,9 @@ public boolean isFull() { * cannot both acquire the last slot. Returns {@code false} with the count unchanged when the * table is full. * - *

Build the entry before reserving: there is no cancellation operation, so abandoning a - * successful reservation permanently consumes capacity. + *

Prefer {@link #cancelReservation()} plus deferred entry construction (see {@link + * ConcurrentHashtable#reserve}) over abandoning a reservation outright: this method by itself + * still has no way to give back a slot once claimed. */ public boolean tryReserve() { if (size.incrementAndGet() > capacity) { @@ -674,6 +711,27 @@ public boolean tryReserve() { return true; } + /** + * Gives back a slot claimed by {@link #tryReserve()} that was never filled — e.g. a concurrent + * match was found under the write lock instead of inserting. Lock-free, symmetric with {@link + * #decrement()}. + * + *

Caller must call this at most once per successful {@link #tryReserve()}; double-cancelling + * corrupts the count the same way double-incrementing would. {@link Reservation#close()} + * handles this bookkeeping automatically and should be preferred over calling this directly. + * + *

Under heavy contention on the same logical duplicate, multiple threads can each reserve a + * slot for what turns out to be the same entry before any of them cancels, transiently + * inflating {@code size} above the table's true occupancy. This can cause an unrelated, + * genuinely distinct concurrent insert to see the table as full when it isn't, until the losing + * reservations cancel. The effect is self-correcting (bounded by in-flight reservations, not + * sustained) and considered an acceptable tradeoff for tables expecting bursts of identical + * inserts (e.g. deduplication). + */ + public void cancelReservation() { + size.decrementAndGet(); + } + /** * Reserves one slot, evicting an entry matching {@code evictable} when the table is full. * Returns {@code false} without changing the table when no entry can be evicted. @@ -682,7 +740,7 @@ public boolean tryReserve() { * an abandoned reservation permanently consumes capacity. */ @GuardedBy("getTableWriteLock(buckets)") - public boolean tryReserveOrEvict( + public > boolean tryReserveOrEvict( @Nonnull AtomicReferenceArray buckets, @Nonnull Predicate evictable) { if (tryReserve()) { @@ -733,7 +791,7 @@ public void release(int removed) { */ @GuardedBy("getTableWriteLock(buckets)") @Nullable - public TEntry evictOne( + public > TEntry evictOne( @Nonnull AtomicReferenceArray buckets, @Nonnull Predicate evictable) { TEntry evicted = evictOneInRange(buckets, evictable, evictionCursor, buckets.length()); @@ -758,7 +816,7 @@ public TEntry evictOne( "evictionCursor is read and written only under synchronized (getTableWriteLock(buckets)); SpotBugs" + " cannot model that dynamic guard") @Nullable - private TEntry evictOneInRange( + private > TEntry evictOneInRange( @Nonnull AtomicReferenceArray buckets, @Nonnull Predicate evictable, int startBucket, @@ -788,7 +846,7 @@ private TEntry evictOneInRange( justification = "evictionCursor is read and written only under synchronized (getTableWriteLock(buckets)); SpotBugs" + " cannot model that dynamic guard") - public int evictAll( + public > int evictAll( @Nonnull AtomicReferenceArray buckets, @Nonnull Predicate evictable) { int count = 0; @@ -818,7 +876,7 @@ public int evictAll( * {@link #tryReserve}, {@link #tryReserveOrEvict}, {@link #evictOne}, {@link #evictAll}) rather * than reach into the manager directly. */ - public static final class State { + public static final class State> { public final AtomicReferenceArray buckets; final SizeManager sizeManager; @@ -833,7 +891,7 @@ private State(AtomicReferenceArray buckets, int maxCapacity) { * that cap. {@code entryClass} is used only to infer {@code TEntry}. */ @Nonnull - public static State createBounded( + public static > State createBounded( @Nonnull Class entryClass, int maxCapacity) { return new State<>(createFixedBuckets(entryClass, maxCapacity), maxCapacity); } @@ -855,13 +913,151 @@ public static boolean isFull(@Nonnull State state) { * Lock-free — does not acquire the table write lock. Returns {@code false} with the table * unchanged when it is full. * - *

Build the entry before reserving: there is no cancellation operation, so abandoning a - * successful reservation permanently consumes capacity. Complete it with {@link #insertReserved}. + *

Complete it with {@link #insertReserved}, or prefer {@link #reserve} for a higher-level, + * auto-cancelling handle that also defers entry construction until the reservation succeeds. */ - public static boolean tryReserve(@Nonnull State state) { + public static > boolean tryReserve(@Nonnull State state) { return state.sizeManager.tryReserve(); } + /** + * Claims one slot in {@code state} and returns a handle for completing the find-or-insert + * protocol, or an empty handle if the table is full. Lock-free — does not acquire the table write + * lock. Never returns {@code null}, so this always composes with try-with-resources: + * + * 

{@code
+   * try (Reservation r = ConcurrentHashtable.reserve(state)) {
+   *   return r.tryGetOrInsertOrNull(TEntry::new, component1, component2, component3);
+   * }
+   * }
+ * + *

Run a lock-free scan first (see {@link #bucketFor}/{@link #bucketAt}) and only call this + * once that scan has missed — a successful reservation isn't required for correctness (the + * reservation itself, and the locked comparison inside {@link Reservation#tryGetOrInsertOrNull}, + * are the source of truth), it just avoids paying for a lock and a factory call when a hit was + * already visible lock-free. + */ + @Nonnull + public static > Reservation reserve( + @Nonnull State state) { + return new Reservation<>(state.sizeManager.tryReserve() ? state : null); + } + + /** + * Handle returned by {@link #reserve}, gating {@link #tryGetOrInsertOrNull} behind a claimed slot + * and auto-cancelling it on {@link #close} if it's never consumed. A single {@code Reservation} + * must be used for at most one {@code tryGetOrInsertOrNull} call. + * + *

Overloaded up to 4 key components ({@link #tryGetOrInsertOrNull(Function, Object)} through + * {@link #tryGetOrInsertOrNull(Function4, Object, Object, Object, Object)}) so a non-capturing + * method reference can build the entry directly from its natural constructor arguments, without + * an intermediate holder object or a capturing lambda. + * + * @param the table's entry type, itself self-bound (see {@link + * ConcurrentHashtable.Entry}) + */ + public static final class Reservation> implements AutoCloseable { + @Nullable private final State state; + private boolean consumed; + + private Reservation(@Nullable State state) { + this.state = state; + } + + /** {@code true} if this is a real, claimed reservation rather than an empty one. */ + public boolean isPresent() { + return state != null; + } + + /** + * One key component; see {@link #tryGetOrInsertOrNull(BiFunction, Object, Object)} for the + * general contract. + */ + @Nullable + public TEntry tryGetOrInsertOrNull( + @Nonnull Function factory, A a) { + return state == null ? null : finish(factory.apply(a)); + } + + /** + * Two key components. Builds {@code factory.apply(...)} (skipped entirely if this reservation + * is empty — the table was full) and either links the result as a new entry or discards it in + * favor of an existing match found under the write lock. Returns {@code null} only when this + * reservation is empty; otherwise always returns a real entry (the newly built one, or the + * concurrent match). + * + *

Building the entry here, after the reservation already succeeded, keeps the write lock's + * critical section limited to the comparison/link/discard decision rather than whatever + * construction cost {@code factory} pays. See {@link ConcurrentHashtable.Entry#matches} — the + * under-lock comparison is entry-to-entry, so it needs {@code newEntry} already built. + */ + @Nullable + public TEntry tryGetOrInsertOrNull( + @Nonnull BiFunction factory, A a, B b) { + return state == null ? null : finish(factory.apply(a, b)); + } + + /** + * Three key components; see {@link #tryGetOrInsertOrNull(BiFunction, Object, Object)} for the + * general contract. + */ + @Nullable + public TEntry tryGetOrInsertOrNull( + @Nonnull Function3 factory, + A a, + B b, + C c) { + return state == null ? null : finish(factory.apply(a, b, c)); + } + + /** Four key components; see {@link #tryGetOrInsertOrNull(BiFunction, Object, Object)}. */ + @Nullable + public TEntry tryGetOrInsertOrNull( + @Nonnull Function4 factory, + A a, + B b, + C c, + D d) { + return state == null ? null : finish(factory.apply(a, b, c, d)); + } + + private TEntry finish(@Nonnull TEntry newEntry) { + synchronized (getTableWriteLock(state)) { + int index = bucketIndex(state.buckets, newEntry.keyHash); + for (TEntry curEntry = bucketAt(state, index); + curEntry != null; + curEntry = curEntry.next()) { + if (curEntry.keyHash == newEntry.keyHash && curEntry.matches(newEntry)) { + return curEntry; + } + } + insertHeadEntryAt(state, index, newEntry); + consumed = true; + return newEntry; + } + } + + /** Gives back an unconsumed reservation's slot; a no-op on an empty reservation. */ + @Override + public void close() { + if (state != null && !consumed) { + state.sizeManager.cancelReservation(); + } + } + } + + /** Three-argument analogue of {@link java.util.function.BiFunction}. */ + @FunctionalInterface + public interface Function3 { + R apply(A a, B b, C c); + } + + /** Four-argument analogue of {@link java.util.function.BiFunction}. */ + @FunctionalInterface + public interface Function4 { + R apply(A a, B b, C c, D d); + } + /** * Reserves one slot in {@code state}, evicting an entry matching {@code evictable} when * necessary. Returns {@code false} if the table is full and nothing can be evicted. This method @@ -870,7 +1066,7 @@ public static boolean tryReserve(@Nonnull State s *

The reservation survives drain and clear operations. Complete it with {@link * #insertReserved}; abandoning it permanently consumes capacity. */ - public static boolean tryReserveOrEvict( + public static > boolean tryReserveOrEvict( @Nonnull State state, @Nonnull Predicate evictable) { synchronized (getTableWriteLock(state)) { return state.sizeManager.tryReserveOrEvict(state.buckets, evictable); @@ -883,7 +1079,7 @@ public static boolean tryReserveOrEvict( * Self-locking. */ @Nullable - public static TEntry evictOne( + public static > TEntry evictOne( @Nonnull State state, @Nonnull Predicate evictable) { synchronized (getTableWriteLock(state)) { return state.sizeManager.evictOne(state.buckets, evictable); @@ -894,7 +1090,7 @@ public static TEntry evictOne( * Unlinks every entry in {@code state} matching {@code evictable}, decrementing per removal, and * returns how many went. Self-locking. */ - public static int evictAll( + public static > int evictAll( @Nonnull State state, @Nonnull Predicate evictable) { synchronized (getTableWriteLock(state)) { return state.sizeManager.evictAll(state.buckets, evictable); @@ -918,7 +1114,7 @@ public static int evictAll( * reflective allocation or runtime type checks; it only lets the compiler infer {@code TEntry}. */ @Nonnull - public static AtomicReferenceArray createFixedBuckets( + public static > AtomicReferenceArray createFixedBuckets( @Nonnull Class entryClass, int capacity) { return new AtomicReferenceArray<>(sizeFor(capacity)); } @@ -1013,14 +1209,14 @@ public static int bucketIndex(@Nonnull AtomicReferenceArray buckets, long key * bucket. */ @Nullable - public static TEntry bucketFor( + public static > TEntry bucketFor( @Nonnull AtomicReferenceArray buckets, long keyHash) { return buckets.get(bucketIndex(buckets, keyHash)); } /** {@link #bucketFor(AtomicReferenceArray, long)} over a {@link State}. */ @Nullable - public static TEntry bucketFor( + public static > TEntry bucketFor( @Nonnull State state, long keyHash) { return bucketFor(state.buckets, keyHash); } @@ -1032,14 +1228,15 @@ public static TEntry bucketFor( * overload of it. */ @Nullable - public static TEntry bucketAt( + public static > TEntry bucketAt( @Nonnull AtomicReferenceArray buckets, int index) { return buckets.get(index); } /** {@link #bucketAt(AtomicReferenceArray, int)} over a {@link State}. */ @Nullable - public static TEntry bucketAt(@Nonnull State state, int index) { + public static > TEntry bucketAt( + @Nonnull State state, int index) { return bucketAt(state.buckets, index); } @@ -1053,7 +1250,7 @@ public static TEntry bucketAt(@Nonnull State stat * retains its {@code next} link for readers already traversing that chain. */ @GuardedBy("getWriteLockAt(buckets, index)") - public static void insertHeadEntryAt( + public static > void insertHeadEntryAt( @Nonnull AtomicReferenceArray buckets, int index, @Nonnull TEntry entry) { assert Thread.holdsLock(getWriteLockAt(buckets, index)) : "insertHeadEntryAt called without holding getWriteLockAt(buckets, index)"; @@ -1067,7 +1264,7 @@ public static void insertHeadEntryAt( /** {@link #insertHeadEntryAt(AtomicReferenceArray, int, Entry)} over a {@link State}. */ @GuardedBy("getWriteLockAt(state, index)") - public static void insertHeadEntryAt( + public static > void insertHeadEntryAt( @Nonnull State state, int index, @Nonnull TEntry entry) { insertHeadEntryAt(state.buckets, index, entry); } @@ -1078,7 +1275,7 @@ public static void insertHeadEntryAt( * getOrCreate} that reuses it across the lock-free pre-check). */ @GuardedBy("getWriteLock(buckets, keyHash)") - public static void insertHeadEntryFor( + public static > void insertHeadEntryFor( @Nonnull AtomicReferenceArray buckets, long keyHash, @Nonnull TEntry entry) { insertHeadEntryAt(buckets, bucketIndex(buckets, keyHash), entry); } @@ -1092,7 +1289,7 @@ public static void insertHeadEntryFor( * reservations. */ @GuardedBy("getTableWriteLock(state)") - public static void insertReserved( + public static > void insertReserved( @Nonnull State state, long keyHash, @Nonnull TEntry entry) { insertHeadEntryFor(state.buckets, keyHash, entry); } @@ -1107,7 +1304,7 @@ public static void insertReserved( * Does not touch size accounting. */ @GuardedBy("getWriteLockAt(buckets, index)") - public static void unlink( + public static > void unlink( @Nonnull AtomicReferenceArray buckets, int index, @Nullable TEntry prev, @@ -1124,7 +1321,7 @@ public static void unlink( /** {@link #unlink(AtomicReferenceArray, int, Entry, Entry)} over a {@link State}. */ @GuardedBy("getWriteLockAt(state, index)") - public static void unlink( + public static > void unlink( @Nonnull State state, int index, @Nullable TEntry prev, @Nonnull TEntry entry) { unlink(state.buckets, index, prev, entry); } @@ -1135,7 +1332,7 @@ public static void unlink( * predicate sees a stable table and concurrent writers are excluded; lock-free readers continue * throughout. */ - public static boolean removeIf( + public static > boolean removeIf( @Nonnull AtomicReferenceArray buckets, @Nonnull AtomicInteger size, @Nonnull Predicate predicate) { @@ -1163,7 +1360,7 @@ public static boolean removeIf( * occupancy with a {@link State} instead of a bare counter — used by {@link D1#removeIf} and * {@link D2#removeIf}. */ - public static boolean removeIf( + public static > boolean removeIf( @Nonnull State state, @Nonnull Predicate predicate) { AtomicReferenceArray buckets = state.buckets; synchronized (getTableWriteLock(state)) { @@ -1192,7 +1389,7 @@ public static boolean removeIf( * *

The sink must not throw. If it does, the partial drain is not rolled back. */ - public static void drain( + public static > void drain( @Nonnull AtomicReferenceArray buckets, @Nonnull Consumer sink) { drainCounting(buckets, sink); } @@ -1202,7 +1399,7 @@ public static void drain( * sink}, so a {@link State} form can subtract exactly that from its {@link SizeManager} instead * of zeroing. The count is free here: the sweep already visits every entry. */ - private static int drainCounting( + private static > int drainCounting( @Nonnull AtomicReferenceArray buckets, @Nonnull Consumer sink) { int removed = 0; synchronized (getTableWriteLock(buckets)) { @@ -1222,7 +1419,7 @@ private static int drainCounting( } /** Context-passing variant of {@link #drain(AtomicReferenceArray, Consumer)}. Self-locking. */ - public static void drain( + public static > void drain( @Nonnull AtomicReferenceArray buckets, C context, @Nonnull BiConsumer sink) { @@ -1230,7 +1427,7 @@ public static void drain( } /** {@link #drainCounting(AtomicReferenceArray, Consumer)}, context-passing form. */ - private static int drainCounting( + private static > int drainCounting( @Nonnull AtomicReferenceArray buckets, C context, @Nonnull BiConsumer sink) { @@ -1257,7 +1454,7 @@ private static int drainCounting( * freed. Draining without that leaves the cap permanently consumed, so the two belong in one call * rather than as a pair the caller has to remember. */ - public static void drain( + public static > void drain( @Nonnull State state, @Nonnull Consumer sink) { synchronized (getTableWriteLock(state)) { state.sizeManager.release(drainCounting(state.buckets, sink)); @@ -1265,7 +1462,7 @@ public static void drain( } /** Context-passing form of {@link #drain(State, Consumer)}. */ - public static void drain( + public static > void drain( @Nonnull State state, C context, @Nonnull BiConsumer sink) { @@ -1290,16 +1487,16 @@ public static void clear(@Nonnull AtomicReferenceArray buckets) { * rather than O(buckets); clear is a rare, whole-table operation, so the walk is affordable and * keeping the count honest is worth more than the constant. */ - private static int clearCounting(@Nonnull AtomicReferenceArray buckets) { + private static int clearCounting(@Nonnull AtomicReferenceArray> buckets) { int removed = 0; synchronized (getTableWriteLock(buckets)) { for (int i = 0; i < buckets.length(); i++) { - Entry head = buckets.get(i); + Entry head = buckets.get(i); if (head == null) { continue; } buckets.set(i, null); - for (Entry e = head; e != null; e = e.next()) { + for (Entry e = head; e != null; e = e.next()) { removed++; } } @@ -1316,7 +1513,7 @@ public static void clear(@Nonnull State state) { } } - public static void forEach( + public static > void forEach( @Nonnull AtomicReferenceArray buckets, @Nonnull Consumer consumer) { for (int i = 0; i < buckets.length(); i++) { for (TEntry curEntry = buckets.get(i); curEntry != null; curEntry = curEntry.next()) { @@ -1325,7 +1522,7 @@ public static void forEach( } } - public static void forEach( + public static > void forEach( @Nonnull AtomicReferenceArray buckets, C context, @Nonnull BiConsumer consumer) { @@ -1337,13 +1534,13 @@ public static void forEach( } /** {@link #forEach(AtomicReferenceArray, Consumer)} over a {@link State}. */ - public static void forEach( + public static > void forEach( @Nonnull State state, @Nonnull Consumer consumer) { forEach(state.buckets, consumer); } /** {@link #forEach(AtomicReferenceArray, Object, BiConsumer)} over a {@link State}. */ - public static void forEach( + public static > void forEach( @Nonnull State state, C context, @Nonnull BiConsumer consumer) { diff --git a/internal-api/src/test/groovy/datadog/trace/api/telemetry/LogCollectorTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/telemetry/LogCollectorTest.groovy index 4f798f5bf9f..e82ad0dbad6 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/telemetry/LogCollectorTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/telemetry/LogCollectorTest.groovy @@ -1,6 +1,7 @@ package datadog.trace.api.telemetry import datadog.trace.test.util.DDSpecification +import datadog.trace.util.ConcurrentHashtable class LogCollectorTest extends DDSpecification { @@ -30,7 +31,7 @@ class LogCollectorTest extends DDSpecification { logCollector.addLogMessage("ERROR", "Message 4", null) then: - logCollector.rawLogMessages.size() == 3 + ConcurrentHashtable.estimateSize(logCollector.rawLogMessages) == 3 } void "grouping messages in LogCollector"() { @@ -57,7 +58,7 @@ class LogCollectorTest extends DDSpecification { boolean listContains(Collection list, String logLevel, String message, Throwable t, int count) { for (final def logMsg in list) { - if (logMsg.logLevel == logLevel && logMsg.message == message && logMsg.throwable == t && logMsg.count == count) { + if (logMsg.logLevel == logLevel && logMsg.message == message && logMsg.throwable == t && logMsg.count.get() == count) { return true } } diff --git a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD1Test.java b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD1Test.java index 540eb036c2f..62985e64658 100644 --- a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD1Test.java +++ b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD1Test.java @@ -464,7 +464,7 @@ void tryGetOrCreateOrEvictOrNullEvictionRunsBeforeThrowingCreator() { assertNull(table.get("new")); } - private static final class StringEntry extends ConcurrentHashtable.D1.Entry { + private static final class StringEntry extends ConcurrentHashtable.D1.Entry { final int value; StringEntry(String key, int value) { @@ -498,7 +498,8 @@ public boolean equals(Object o) { } } - private static final class CollidingEntry extends ConcurrentHashtable.D1.Entry { + private static final class CollidingEntry + extends ConcurrentHashtable.D1.Entry { CollidingEntry(CollidingKey key) { super(key); } diff --git a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD2Test.java b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD2Test.java index 182cb4c4f25..69422a07caf 100644 --- a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD2Test.java +++ b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD2Test.java @@ -391,7 +391,8 @@ void tryGetOrCreateOrEvictOrNullEvictionRunsBeforeThrowingCreator() { assertNull(table.get("new", 2)); } - private static final class PairEntry extends ConcurrentHashtable.D2.Entry { + private static final class PairEntry + extends ConcurrentHashtable.D2.Entry { PairEntry(String key1, Integer key2) { super(key1, key2); } diff --git a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableReservationTest.java b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableReservationTest.java new file mode 100644 index 00000000000..55650a72b2f --- /dev/null +++ b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableReservationTest.java @@ -0,0 +1,162 @@ +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.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.concurrent.atomic.AtomicInteger; +import javax.annotation.Nonnull; +import org.junit.jupiter.api.Test; + +/** Exercises {@link ConcurrentHashtable#reserve} and {@link ConcurrentHashtable.Reservation}. */ +class ConcurrentHashtableReservationTest { + + private static final class TestEntry extends ConcurrentHashtable.Entry { + final int value; + + TestEntry(int value) { + super(value); + this.value = value; + } + + @Override + public boolean matches(@Nonnull TestEntry other) { + return value == other.value; + } + } + + @Test + void tryGetOrInsertOrNullInsertsOnMissAndFindsOnHit() { + ConcurrentHashtable.State state = + ConcurrentHashtable.createBounded(TestEntry.class, 4); + + TestEntry first; + try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.reserve(state)) { + assertTrue(r.isPresent()); + first = r.tryGetOrInsertOrNull(TestEntry::new, 1); + } + assertEquals(1, first.value); + assertEquals(1, ConcurrentHashtable.estimateSize(state)); + + // Reserving again for a key that already exists should discard the reservation and return the + // existing entry, not double-insert or leak the claimed slot. + TestEntry second; + try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.reserve(state)) { + second = r.tryGetOrInsertOrNull(TestEntry::new, 1); + } + assertSame(first, second); + assertEquals(1, ConcurrentHashtable.estimateSize(state)); + } + + @Test + void reserveOnFullTableIsAbsentAndSkipsTheFactory() { + ConcurrentHashtable.State state = + ConcurrentHashtable.createBounded(TestEntry.class, 1); + try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.reserve(state)) { + r.tryGetOrInsertOrNull(TestEntry::new, 1); + } + assertTrue(ConcurrentHashtable.isFull(state)); + + AtomicInteger factoryCalls = new AtomicInteger(); + TestEntry result; + try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.reserve(state)) { + assertFalse(r.isPresent()); + result = + r.tryGetOrInsertOrNull( + v -> { + factoryCalls.incrementAndGet(); + return new TestEntry(v); + }, + 2); + } + assertNull(result); + assertEquals(0, factoryCalls.get()); + assertEquals(1, ConcurrentHashtable.estimateSize(state)); + } + + @Test + void closeCancelsAnUnconsumedReservation() { + ConcurrentHashtable.State state = + ConcurrentHashtable.createBounded(TestEntry.class, 1); + try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.reserve(state)) { + assertTrue(r.isPresent()); + // Deliberately not consuming the reservation. + } + assertEquals(0, ConcurrentHashtable.estimateSize(state)); + assertFalse(ConcurrentHashtable.isFull(state)); + } + + @Test + void closeOnAnAbsentReservationIsANoOp() { + ConcurrentHashtable.State state = + ConcurrentHashtable.createBounded(TestEntry.class, 0); + try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.reserve(state)) { + assertFalse(r.isPresent()); + } + assertEquals(0, ConcurrentHashtable.estimateSize(state)); + } + + private static final class ThreePartEntry extends ConcurrentHashtable.Entry { + final String a; + final String b; + final String c; + + ThreePartEntry(String a, String b, String c) { + super(HashingUtils.hash(a, b, c)); + this.a = a; + this.b = b; + this.c = c; + } + + @Override + public boolean matches(@Nonnull ThreePartEntry other) { + return a.equals(other.a) && b.equals(other.b) && c.equals(other.c); + } + } + + private static final class FourPartEntry extends ConcurrentHashtable.Entry { + final String a; + final String b; + final String c; + final String d; + + FourPartEntry(String a, String b, String c, String d) { + super(HashingUtils.hash(a, b, c, d)); + this.a = a; + this.b = b; + this.c = c; + this.d = d; + } + + @Override + public boolean matches(@Nonnull FourPartEntry other) { + return a.equals(other.a) && b.equals(other.b) && c.equals(other.c) && d.equals(other.d); + } + } + + @Test + void tryGetOrInsertOrNullSupportsUpToFourComponents() { + ConcurrentHashtable.State state3 = + ConcurrentHashtable.createBounded(ThreePartEntry.class, 2); + ThreePartEntry three; + try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.reserve(state3)) { + three = r.tryGetOrInsertOrNull(ThreePartEntry::new, "x", "y", "z"); + } + assertEquals("x", three.a); + assertEquals("y", three.b); + assertEquals("z", three.c); + + ConcurrentHashtable.State state4 = + ConcurrentHashtable.createBounded(FourPartEntry.class, 2); + FourPartEntry four; + try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.reserve(state4)) { + four = r.tryGetOrInsertOrNull(FourPartEntry::new, "w", "x", "y", "z"); + } + assertEquals("w", four.a); + assertEquals("x", four.b); + assertEquals("y", four.c); + assertEquals("z", four.d); + } +} diff --git a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableSizeManagerTest.java b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableSizeManagerTest.java index 35bb9cc24de..2fceb4ce3ea 100644 --- a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableSizeManagerTest.java +++ b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableSizeManagerTest.java @@ -420,12 +420,17 @@ private static int evictAll( } /** Entry with a caller-controlled {@code keyHash} so tests can place it in an exact bucket. */ - private static final class TestEntry extends ConcurrentHashtable.Entry { + private static final class TestEntry extends ConcurrentHashtable.Entry { final String label; TestEntry(long keyHash, String label) { super(keyHash); this.label = label; } + + @Override + public boolean matches(TestEntry other) { + return label.equals(other.label); + } } } diff --git a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableStaticsTest.java b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableStaticsTest.java index 8191e396e14..8be6e791b9e 100644 --- a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableStaticsTest.java +++ b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableStaticsTest.java @@ -329,7 +329,7 @@ private static boolean assertionsEnabled() { } /** Primitive-{@code int}-key entry: no boxing, keyHash is the key itself. */ - private static final class IntEntry extends ConcurrentHashtable.Entry { + private static final class IntEntry extends ConcurrentHashtable.Entry { final int key; final int value; @@ -342,6 +342,11 @@ private static final class IntEntry extends ConcurrentHashtable.Entry { boolean matches(int key) { return this.key == key; } + + @Override + public boolean matches(IntEntry other) { + return matches(other.key); + } } /** From bda4059438188576d264fdbef2df9ba872b49d1d Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Thu, 10 Sep 2026 21:50:34 -0400 Subject: [PATCH 3/9] Restore self-bounded D1/D2 Entry generics, add hashIterator, fix stack-trace re-allocation in LogCollector Reverts D1/D2 back to the self-bounded Entry> shape per PR review, so entry payloads live in the subclass rather than a boxed value field. Adds ConcurrentHashtable.hashIterator as a public, reusable bucket-chain iterator, replacing LogCollector's private bucketIterator copy. Also stops matchesKey from calling the incoming throwable's getStackTrace() (a defensive-copy allocation) on every candidate scanned in addLogMessage's bucket chain -- it's now memoized once per call. Co-Authored-By: Claude Sonnet 5 --- .../util/ThreadSafeMapCounterBenchmark.java | 29 +-- .../trace/util/ThreadSafeMapD1Benchmark.java | 27 +- .../trace/util/ThreadSafeMapD2Benchmark.java | 32 +-- .../trace/api/telemetry/LogCollector.java | 42 +++- .../trace/util/ConcurrentHashtable.java | 234 +++++++++++++----- .../trace/util/ConcurrentHashtableD1Test.java | 30 +-- .../trace/util/ConcurrentHashtableD2Test.java | 19 +- .../ConcurrentHashtableReservationTest.java | 40 ++- 8 files changed, 306 insertions(+), 147 deletions(-) diff --git a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapCounterBenchmark.java b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapCounterBenchmark.java index 7bdf3e1cdea..9a52ac6f172 100644 --- a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapCounterBenchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapCounterBenchmark.java @@ -23,11 +23,10 @@ * Measures lookup followed by an atomic counter increment in a shared, pre-populated table. Models * per-class or per-method hit counters in the tracer. * - *

The {@link ConcurrentHashtable.D1} case embeds a {@code volatile long} in each entry. {@link - * AtomicLongFieldUpdater} updates that field atomically without allocating an {@link AtomicLong} - * per key. The map baselines store a separate {@link AtomicLong} or {@link LongAdder}; {@code - * LongAdder} spreads contention across internal cells at the cost of more memory and a more - * expensive read. + *

The {@link ConcurrentHashtable.D1} case embeds a {@code volatile long} counter directly in the + * entry and increments it via {@link AtomicLongFieldUpdater}, avoiding a second heap object. The + * map baselines store a separate {@link AtomicLong} or {@link LongAdder}; {@code LongAdder} spreads + * contention across internal cells at the cost of more memory and a more expensive read. * *

Lookups reuse the key instances installed during setup. {@code Objects.equals} therefore * returns on its identity check without dispatching to {@code equals}, so this measures the @@ -50,8 +49,8 @@ *

  • {@code LongAdder} is marginally faster (79 vs 71 ops/us) because it shards the counter * across cells to reduce CAS contention; the advantage grows with thread count. *
  • {@code ConcurrentHashtable} matches {@code AtomicLong} throughput (69 vs 71 ops/us) while - * embedding the counter directly in the entry — one object instead of two, with no throughput - * penalty. + * embedding the counter directly in the entry via {@code AtomicLongFieldUpdater} — one object + * instead of two, with no throughput penalty. * */ @Fork(2) @@ -73,8 +72,12 @@ public class ThreadSafeMapCounterBenchmark { } } + /** + * Shared state ({@link Scope#Benchmark}): one instance of each map across all threads, modelling + * a shared instrumentation counter table. + */ static final class CounterEntry extends ConcurrentHashtable.D1.Entry { - private static final AtomicLongFieldUpdater COUNT = + static final AtomicLongFieldUpdater COUNT = AtomicLongFieldUpdater.newUpdater(CounterEntry.class, "count"); volatile long count; @@ -82,16 +85,8 @@ static final class CounterEntry extends ConcurrentHashtable.D1.Entry table; @@ -125,7 +120,7 @@ int next() { @Benchmark public long increment_concurrentHashtable(SharedState s, ThreadState t) { - return s.table.get(KEYS[t.next()]).increment(); + return CounterEntry.COUNT.incrementAndGet(s.table.get(KEYS[t.next()])); } @Benchmark diff --git a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD1Benchmark.java b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD1Benchmark.java index 0c7a1472772..768108952de 100644 --- a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD1Benchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD1Benchmark.java @@ -83,34 +83,35 @@ public class ThreadSafeMapD1Benchmark { } } - static final class D1Entry extends ConcurrentHashtable.D1.Entry { + /** + * Shared state ({@link Scope#Benchmark}): one instance of each map across all threads, modelling + * a shared instrumentation cache. + */ + static final class LongEntry extends ConcurrentHashtable.D1.Entry { final long value; - D1Entry(String key) { + LongEntry(String key, long value) { super(key); - this.value = 1L; + this.value = value; } } - /** - * Shared state ({@link Scope#Benchmark}): one instance of each map across all threads, modelling - * a shared instrumentation cache. - */ @State(Scope.Benchmark) public static class SharedState { - ConcurrentHashtable.D1 table; + ConcurrentHashtable.D1 table; ConcurrentHashMap concurrentHashMap; ConcurrentSkipListMap skipListMap; Map synchronizedHashMap; @Setup(Level.Iteration) public void setUp() { - table = ConcurrentHashtable.D1.createBounded(D1Entry.class, CAPACITY); + table = ConcurrentHashtable.D1.createBounded(LongEntry.class, CAPACITY); concurrentHashMap = new ConcurrentHashMap<>(CAPACITY); skipListMap = new ConcurrentSkipListMap<>(); synchronizedHashMap = Collections.synchronizedMap(new HashMap<>(CAPACITY)); for (int i = 0; i < N_KEYS; ++i) { - table.tryGetOrCreateOrNull(KEYS[i], D1Entry::new); + long value = i; + table.tryGetOrCreateOrNull(KEYS[i], k -> new LongEntry(k, value)); concurrentHashMap.put(KEYS[i], (long) i); skipListMap.put(KEYS[i], (long) i); synchronizedHashMap.put(KEYS[i], (long) i); @@ -131,7 +132,7 @@ int next() { } @Benchmark - public D1Entry get_concurrentHashtable(SharedState s, ThreadState t) { + public LongEntry get_concurrentHashtable(SharedState s, ThreadState t) { return s.table.get(KEYS[t.next()]); } @@ -151,8 +152,8 @@ public Long get_synchronizedHashMap(SharedState s, ThreadState t) { } @Benchmark - public D1Entry getOrCreate_concurrentHashtable(SharedState s, ThreadState t) { - return s.table.tryGetOrCreateOrNull(KEYS[t.next()], D1Entry::new); + public LongEntry getOrCreate_concurrentHashtable(SharedState s, ThreadState t) { + return s.table.tryGetOrCreateOrNull(KEYS[t.next()], k -> new LongEntry(k, 0L)); } /** diff --git a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD2Benchmark.java b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD2Benchmark.java index 963de93f8ae..118ecae54e8 100644 --- a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD2Benchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD2Benchmark.java @@ -95,15 +95,6 @@ public class ThreadSafeMapD2Benchmark { } } - static final class D2Entry extends ConcurrentHashtable.D2.Entry { - final long value; - - D2Entry(String k1, Integer k2) { - super(k1, k2); - this.value = 1L; - } - } - /** * Entry used with the static helpers. Its primitive second key keeps storage and lookup unboxed, * independently of {@link Integer} caching or JVM escape analysis. @@ -134,6 +125,16 @@ public boolean matches(SupportEntry other) { } } + /** Entry used with {@link ConcurrentHashtable.D2}. */ + static final class PairEntry extends ConcurrentHashtable.D2.Entry { + final long value; + + PairEntry(String key1, Integer key2, long value) { + super(key1, key2); + this.value = value; + } + } + /** Composite key for map-based baselines. */ static final class Key2 implements Comparable { final String k1; @@ -176,7 +177,7 @@ public int compareTo(Key2 other) { */ @State(Scope.Benchmark) public static class SharedState { - ConcurrentHashtable.D2 table; + ConcurrentHashtable.D2 table; java.util.concurrent.atomic.AtomicReferenceArray supportBuckets; ConcurrentHashMap concurrentHashMap; ConcurrentSkipListMap skipListMap; @@ -184,14 +185,14 @@ public static class SharedState { @Setup(Level.Iteration) public void setUp() { - table = ConcurrentHashtable.D2.createBounded(D2Entry.class, CAPACITY); + table = ConcurrentHashtable.D2.createBounded(PairEntry.class, CAPACITY); supportBuckets = ConcurrentHashtable.createFixedBuckets(SupportEntry.class, CAPACITY); concurrentHashMap = new ConcurrentHashMap<>(CAPACITY); skipListMap = new ConcurrentSkipListMap<>(); synchronizedHashMap = Collections.synchronizedMap(new HashMap<>(CAPACITY)); for (int i = 0; i < N_KEYS; ++i) { int k2 = SOURCE_K2[i]; - table.tryGetOrCreateOrNull(SOURCE_K1[i], SOURCE_K2[i], D2Entry::new); + table.tryGetOrCreateOrNull(SOURCE_K1[i], SOURCE_K2[i], (a, b) -> new PairEntry(a, b, 1L)); // populate support table SupportEntry se = new SupportEntry(SOURCE_K1[i], k2); synchronized (ConcurrentHashtable.getWriteLock(supportBuckets, se.keyHash)) { @@ -218,7 +219,7 @@ int next() { } @Benchmark - public D2Entry get_concurrentHashtable(SharedState s, ThreadState t) { + public PairEntry get_concurrentHashtable(SharedState s, ThreadState t) { int i = t.next(); return s.table.get(SOURCE_K1[i], SOURCE_K2[i]); } @@ -258,9 +259,10 @@ public Long get_synchronizedHashMap(SharedState s, ThreadState t) { } @Benchmark - public D2Entry getOrCreate_concurrentHashtable(SharedState s, ThreadState t) { + public PairEntry getOrCreate_concurrentHashtable(SharedState s, ThreadState t) { int i = t.next(); - return s.table.tryGetOrCreateOrNull(SOURCE_K1[i], SOURCE_K2[i], D2Entry::new); + return s.table.tryGetOrCreateOrNull( + SOURCE_K1[i], SOURCE_K2[i], (k1, k2) -> new PairEntry(k1, k2, 0L)); } @Benchmark diff --git a/internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java b/internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java index 70c121c98ae..2de274f93a7 100644 --- a/internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java +++ b/internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java @@ -6,6 +6,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.Iterator; import java.util.List; import java.util.Objects; import java.util.concurrent.atomic.AtomicInteger; @@ -53,19 +54,31 @@ public void addLogMessage( String logLevel, String message, @Nullable Throwable throwable, @Nullable String tags) { long keyHash = RawLogMessage.hash(logLevel, message, throwable); + // Memoized once per call and shared across every candidate below, rather than letting + // matchesKey call throwable.getStackTrace() (a defensive-copy allocation) on each comparison. + StackTraceElement[] throwableStackTrace = null; + // Lock-free scan first: most calls are re-observations of an already-seen message, so this // avoids paying for a reservation and a RawLogMessage allocation on the common path. - for (RawLogMessage existing = ConcurrentHashtable.bucketFor(rawLogMessages, keyHash); - existing != null; - existing = existing.next()) { - if (existing.keyHash == keyHash && existing.matchesKey(logLevel, message, throwable)) { + for (Iterator it = ConcurrentHashtable.hashIterator(rawLogMessages, keyHash); + it.hasNext(); ) { + RawLogMessage existing = it.next(); + if (existing.keyHash != keyHash) { + continue; + } + if (throwable != null && existing.throwable != null && existing.throwable != throwable) { + if (throwableStackTrace == null) { + throwableStackTrace = throwable.getStackTrace(); + } + } + if (existing.matchesKey(logLevel, message, throwable, throwableStackTrace)) { existing.count.incrementAndGet(); return; } } try (ConcurrentHashtable.Reservation reservation = - ConcurrentHashtable.reserve(rawLogMessages)) { + ConcurrentHashtable.tryReserve(rawLogMessages)) { // TODO: We could emit a metric for dropped logs when the reservation is empty (table full). RawLogMessage rawLogMessage = reservation.tryGetOrInsertOrNull(RawLogMessage::new, logLevel, message, throwable, tags); @@ -76,11 +89,12 @@ public void addLogMessage( } public Collection drain() { - if (ConcurrentHashtable.estimateSize(rawLogMessages) == 0) { + int size = ConcurrentHashtable.estimateSize(rawLogMessages); + if (size == 0) { return Collections.emptyList(); } - List list = new ArrayList<>(ConcurrentHashtable.estimateSize(rawLogMessages)); + List list = new ArrayList<>(size); ConcurrentHashtable.drain(rawLogMessages, list::add); return list; } @@ -124,7 +138,17 @@ public StackTraceElement[] stackTrace() { return stackTrace; } - private boolean matchesKey(String logLevel, String message, @Nullable Throwable throwable) { + /** + * @param throwableStackTrace {@code throwable.getStackTrace()}, memoized once by the caller and + * shared across every candidate scanned for a given {@code addLogMessage} call -- avoids + * paying {@code getStackTrace()}'s defensive-copy allocation on each comparison. Only + * non-null when {@code throwable} needs a deep comparison against some candidate. + */ + private boolean matchesKey( + String logLevel, + String message, + @Nullable Throwable throwable, + @Nullable StackTraceElement[] throwableStackTrace) { if (!Objects.equals(this.logLevel, logLevel)) return false; if (!Objects.equals(this.message, message)) return false; @@ -137,7 +161,7 @@ private boolean matchesKey(String logLevel, String message, @Nullable Throwable } else if (this.throwable != null && throwable != null) { // Both have a throwable perform a deeper comparison return this.throwable.getClass().equals(throwable.getClass()) - && Objects.deepEquals(stackTrace(), throwable.getStackTrace()); + && Objects.deepEquals(stackTrace(), throwableStackTrace); } else { // One has an exception & the other doesn't, not equal return false; diff --git a/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java b/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java index 36077332a52..4d4b2720057 100644 --- a/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java +++ b/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java @@ -1,6 +1,10 @@ package datadog.trace.util; +import datadog.trace.api.function.Strategy; +import datadog.trace.api.function.StrategyConsumer; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; +import java.util.Iterator; +import java.util.NoSuchElementException; import java.util.Objects; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReferenceArray; @@ -129,7 +133,7 @@ public static final class D1> { */ public abstract static class Entry> extends ConcurrentHashtable.Entry { - final K key; + @Nullable final K key; protected Entry(@Nullable K key) { super(hash(key)); @@ -172,8 +176,9 @@ private D1(State state) { /** * Creates a fixed-size table holding at most {@code maxCapacity} entries. {@code entryClass} is - * used only to infer the concrete entry type; entries are created by the functions passed to - * the insertion methods. The table does not resize. + * used only to allocate the backing array with the right component type; entries themselves are + * created by the {@code creator}/{@code evictable} functions passed to the insertion methods. + * The table does not resize. */ @Nonnull public static > D1 createBounded( @@ -209,7 +214,7 @@ public TEntry get(@Nullable K key) { */ @Nonnull public Maybe tryGetOrCreate( - @Nullable K key, @Nonnull Function creator) { + @Nullable K key, @Strategy @Nonnull Function creator) { return Maybe.of(tryGetOrCreateOrNull(key, creator)); } @@ -219,9 +224,10 @@ public Maybe tryGetOrCreate( * {@code key} was not already present. Re-checks under the lock to avoid duplicate entries * under concurrent misses. */ + @StrategyConsumer @Nullable public TEntry tryGetOrCreateOrNull( - @Nullable K key, @Nonnull Function creator) { + @Nullable K key, @Strategy @Nonnull Function creator) { long keyHash = D1.Entry.hash(key); int index = bucketIndex(state.buckets, keyHash); for (TEntry curEntry = bucketAt(state, index); curEntry != null; curEntry = curEntry.next()) { @@ -260,8 +266,8 @@ public TEntry tryGetOrCreateOrNull( @Nonnull public Maybe tryGetOrCreateOrEvict( @Nullable K key, - @Nonnull Function creator, - @Nonnull Predicate evictable) { + @Strategy @Nonnull Function creator, + @Strategy @Nonnull Predicate evictable) { return Maybe.of(tryGetOrCreateOrEvictOrNull(key, creator, evictable)); } @@ -272,11 +278,12 @@ public Maybe tryGetOrCreateOrEvict( * ever leaving a slot double-booked. A creator that throws after a successful eviction simply * leaves the table one entry smaller — no corruption, just a wasted eviction. */ + @StrategyConsumer @Nullable public TEntry tryGetOrCreateOrEvictOrNull( @Nullable K key, - @Nonnull Function creator, - @Nonnull Predicate evictable) { + @Strategy @Nonnull Function creator, + @Strategy @Nonnull Predicate evictable) { long keyHash = D1.Entry.hash(key); int index = bucketIndex(state.buckets, keyHash); for (TEntry curEntry = bucketAt(state, index); curEntry != null; curEntry = curEntry.next()) { @@ -332,7 +339,7 @@ public TEntry remove(@Nullable K key) { * Holds the table-level lock for the whole sweep, so the predicate sees a stable table and * concurrent writers are excluded; lock-free readers continue throughout. */ - public boolean removeIf(@Nonnull Predicate predicate) { + public boolean removeIf(@Strategy @Nonnull Predicate predicate) { return ConcurrentHashtable.removeIf(state, predicate); } @@ -343,7 +350,7 @@ public boolean removeIf(@Nonnull Predicate predicate) { * *

    Use {@link #drain(Object, BiConsumer)} to avoid a capturing lambda. */ - public void drain(@Nonnull Consumer sink) { + public void drain(@Strategy @Nonnull Consumer sink) { ConcurrentHashtable.drain(state, sink); } @@ -352,7 +359,8 @@ public void drain(@Nonnull Consumer sink) { * a {@code static final}) plus the accumulator as {@code context} (e.g. the target list or * event builder) to avoid a capturing-lambda allocation. */ - public void drain(C context, @Nonnull BiConsumer sink) { + public void drain( + C context, @Strategy @Nonnull BiConsumer sink) { ConcurrentHashtable.drain(state, context, sink); } @@ -361,7 +369,7 @@ public void clear() { ConcurrentHashtable.clear(state); } - public void forEach(@Nonnull Consumer consumer) { + public void forEach(@Strategy @Nonnull Consumer consumer) { ConcurrentHashtable.forEach(state, consumer); } @@ -369,7 +377,8 @@ public void forEach(@Nonnull Consumer consumer) { * Context-passing forEach. Avoids a capturing-lambda allocation — pass a non-capturing {@link * BiConsumer} (typically a {@code static final}) plus whatever side-band state it needs. */ - public void forEach(C context, @Nonnull BiConsumer consumer) { + public void forEach( + C context, @Strategy @Nonnull BiConsumer consumer) { ConcurrentHashtable.forEach(state, context, consumer); } } @@ -388,7 +397,7 @@ public static final class D2> { /** * Abstract base for {@link D2} entries. Subclass to add value fields you wish to mutate in - * place. + * place after retrieving the entry via {@link D2#get}. * * @param first key type * @param second key type @@ -397,8 +406,8 @@ public static final class D2> { */ public abstract static class Entry> extends ConcurrentHashtable.Entry { - final K1 key1; - final K2 key2; + @Nullable final K1 key1; + @Nullable final K2 key2; protected Entry(@Nullable K1 key1, @Nullable K2 key2) { super(hash(key1, key2)); @@ -444,8 +453,9 @@ private D2(State state) { /** * Creates a fixed-size table holding at most {@code maxCapacity} entries. {@code entryClass} is - * used only to infer the concrete entry type; entries are created by the functions passed to - * the insertion methods. The table does not resize. + * used only to allocate the backing array with the right component type; entries themselves are + * created by the {@code creator}/{@code evictable} functions passed to the insertion methods. + * The table does not resize. */ @Nonnull public static > @@ -478,15 +488,12 @@ public TEntry get(@Nullable K1 key1, @Nullable K2 key2) { * Returns the entry for {@code (key1, key2)}, creating one via {@code creator} if absent and * the table is under capacity. Lock-free on hit; acquires a table-level lock on miss. Wraps * {@link #tryGetOrCreateOrNull} — see that method for the refusal and ordering details. - * - *

    The {@code creator} should build an entry whose {@code keyHash} equals {@link - * D2.Entry#hash(Object, Object) D2.Entry.hash(key1, key2)}. */ @Nonnull public Maybe tryGetOrCreate( @Nullable K1 key1, @Nullable K2 key2, - @Nonnull BiFunction creator) { + @Strategy @Nonnull BiFunction creator) { return Maybe.of(tryGetOrCreateOrNull(key1, key2, creator)); } @@ -496,11 +503,12 @@ public Maybe tryGetOrCreate( * {@code (key1, key2)} was not already present. Re-checks under the lock to avoid duplicate * entries under concurrent misses. */ + @StrategyConsumer @Nullable public TEntry tryGetOrCreateOrNull( @Nullable K1 key1, @Nullable K2 key2, - @Nonnull BiFunction creator) { + @Strategy @Nonnull BiFunction creator) { long keyHash = D2.Entry.hash(key1, key2); int index = bucketIndex(state.buckets, keyHash); for (TEntry curEntry = bucketAt(state, index); curEntry != null; curEntry = curEntry.next()) { @@ -540,8 +548,8 @@ public TEntry tryGetOrCreateOrNull( public Maybe tryGetOrCreateOrEvict( @Nullable K1 key1, @Nullable K2 key2, - @Nonnull BiFunction creator, - @Nonnull Predicate evictable) { + @Strategy @Nonnull BiFunction creator, + @Strategy @Nonnull Predicate evictable) { return Maybe.of(tryGetOrCreateOrEvictOrNull(key1, key2, creator, evictable)); } @@ -552,12 +560,13 @@ public Maybe tryGetOrCreateOrEvict( * ever leaving a slot double-booked. A creator that throws after a successful eviction simply * leaves the table one entry smaller — no corruption, just a wasted eviction. */ + @StrategyConsumer @Nullable public TEntry tryGetOrCreateOrEvictOrNull( @Nullable K1 key1, @Nullable K2 key2, - @Nonnull BiFunction creator, - @Nonnull Predicate evictable) { + @Strategy @Nonnull BiFunction creator, + @Strategy @Nonnull Predicate evictable) { long keyHash = D2.Entry.hash(key1, key2); int index = bucketIndex(state.buckets, keyHash); for (TEntry curEntry = bucketAt(state, index); curEntry != null; curEntry = curEntry.next()) { @@ -613,7 +622,7 @@ public TEntry remove(@Nullable K1 key1, @Nullable K2 key2) { * Holds the table-level lock for the whole sweep, so the predicate sees a stable table and * concurrent writers are excluded; lock-free readers continue throughout. */ - public boolean removeIf(@Nonnull Predicate predicate) { + public boolean removeIf(@Strategy @Nonnull Predicate predicate) { return ConcurrentHashtable.removeIf(state, predicate); } @@ -624,7 +633,7 @@ public boolean removeIf(@Nonnull Predicate predicate) { * *

    Use {@link #drain(Object, BiConsumer)} to avoid a capturing lambda. */ - public void drain(@Nonnull Consumer sink) { + public void drain(@Strategy @Nonnull Consumer sink) { ConcurrentHashtable.drain(state, sink); } @@ -633,7 +642,8 @@ public void drain(@Nonnull Consumer sink) { * a {@code static final}) plus the accumulator as {@code context} (e.g. the target list or * event builder) to avoid a capturing-lambda allocation. */ - public void drain(C context, @Nonnull BiConsumer sink) { + public void drain( + C context, @Strategy @Nonnull BiConsumer sink) { ConcurrentHashtable.drain(state, context, sink); } @@ -642,7 +652,7 @@ public void clear() { ConcurrentHashtable.clear(state); } - public void forEach(@Nonnull Consumer consumer) { + public void forEach(@Strategy @Nonnull Consumer consumer) { ConcurrentHashtable.forEach(state, consumer); } @@ -650,7 +660,8 @@ public void forEach(@Nonnull Consumer consumer) { * Context-passing forEach. Avoids a capturing-lambda allocation — pass a non-capturing {@link * BiConsumer} (typically a {@code static final}) plus whatever side-band state it needs. */ - public void forEach(C context, @Nonnull BiConsumer consumer) { + public void forEach( + C context, @Strategy @Nonnull BiConsumer consumer) { ConcurrentHashtable.forEach(state, context, consumer); } } @@ -742,7 +753,7 @@ public void cancelReservation() { @GuardedBy("getTableWriteLock(buckets)") public > boolean tryReserveOrEvict( @Nonnull AtomicReferenceArray buckets, - @Nonnull Predicate evictable) { + @Strategy @Nonnull Predicate evictable) { if (tryReserve()) { return true; } @@ -793,7 +804,7 @@ public void release(int removed) { @Nullable public > TEntry evictOne( @Nonnull AtomicReferenceArray buckets, - @Nonnull Predicate evictable) { + @Strategy @Nonnull Predicate evictable) { TEntry evicted = evictOneInRange(buckets, evictable, evictionCursor, buckets.length()); if (evicted == null && evictionCursor != 0) { evicted = evictOneInRange(buckets, evictable, 0, evictionCursor); @@ -815,10 +826,11 @@ public > TEntry evictOne( justification = "evictionCursor is read and written only under synchronized (getTableWriteLock(buckets)); SpotBugs" + " cannot model that dynamic guard") + @StrategyConsumer @Nullable private > TEntry evictOneInRange( @Nonnull AtomicReferenceArray buckets, - @Nonnull Predicate evictable, + @Strategy @Nonnull Predicate evictable, int startBucket, int endBucket) { for (int i = startBucket; i < endBucket; i++) { @@ -846,9 +858,10 @@ private > TEntry evictOneInRange( justification = "evictionCursor is read and written only under synchronized (getTableWriteLock(buckets)); SpotBugs" + " cannot model that dynamic guard") + @StrategyConsumer public > int evictAll( @Nonnull AtomicReferenceArray buckets, - @Nonnull Predicate evictable) { + @Strategy @Nonnull Predicate evictable) { int count = 0; for (int i = 0; i < buckets.length(); i++) { TEntry prev = null; @@ -913,10 +926,11 @@ public static boolean isFull(@Nonnull State state) { * Lock-free — does not acquire the table write lock. Returns {@code false} with the table * unchanged when it is full. * - *

    Complete it with {@link #insertReserved}, or prefer {@link #reserve} for a higher-level, + *

    Complete it with {@link #insertReserved}, or prefer {@link #tryReserve} for a higher-level, * auto-cancelling handle that also defers entry construction until the reservation succeeds. */ - public static > boolean tryReserve(@Nonnull State state) { + public static > boolean tryReserveSlot( + @Nonnull State state) { return state.sizeManager.tryReserve(); } @@ -926,7 +940,7 @@ public static > boolean tryReserve(@Nonnull State{@code - * try (Reservation r = ConcurrentHashtable.reserve(state)) { + * try (Reservation r = ConcurrentHashtable.tryReserve(state)) { * return r.tryGetOrInsertOrNull(TEntry::new, component1, component2, component3); * } * } @@ -936,17 +950,21 @@ public static > boolean tryReserve(@Nonnull StateAlways returns a non-null handle — even when the table is full — so the caller must check + * {@link Reservation#isPresent()} (or simply call {@link Reservation#tryGetOrInsertOrNull}, which + * returns {@code null} on an absent reservation) rather than assume every reservation is real. */ @Nonnull - public static > Reservation reserve( + public static > Reservation tryReserve( @Nonnull State state) { return new Reservation<>(state.sizeManager.tryReserve() ? state : null); } /** - * Handle returned by {@link #reserve}, gating {@link #tryGetOrInsertOrNull} behind a claimed slot - * and auto-cancelling it on {@link #close} if it's never consumed. A single {@code Reservation} - * must be used for at most one {@code tryGetOrInsertOrNull} call. + * Handle returned by {@link #tryReserve}, gating {@link #tryGetOrInsertOrNull} behind a claimed + * slot and auto-cancelling it on {@link #close} if it's never consumed. A single {@code + * Reservation} must be used for at most one {@code tryGetOrInsertOrNull} call. * *

    Overloaded up to 4 key components ({@link #tryGetOrInsertOrNull(Function, Object)} through * {@link #tryGetOrInsertOrNull(Function4, Object, Object, Object, Object)}) so a non-capturing @@ -973,9 +991,10 @@ public boolean isPresent() { * One key component; see {@link #tryGetOrInsertOrNull(BiFunction, Object, Object)} for the * general contract. */ + @StrategyConsumer @Nullable public TEntry tryGetOrInsertOrNull( - @Nonnull Function factory, A a) { + @Strategy @Nonnull Function factory, A a) { return state == null ? null : finish(factory.apply(a)); } @@ -991,9 +1010,10 @@ public TEntry tryGetOrInsertOrNull( * construction cost {@code factory} pays. See {@link ConcurrentHashtable.Entry#matches} — the * under-lock comparison is entry-to-entry, so it needs {@code newEntry} already built. */ + @StrategyConsumer @Nullable public TEntry tryGetOrInsertOrNull( - @Nonnull BiFunction factory, A a, B b) { + @Strategy @Nonnull BiFunction factory, A a, B b) { return state == null ? null : finish(factory.apply(a, b)); } @@ -1001,6 +1021,7 @@ public TEntry tryGetOrInsertOrNull( * Three key components; see {@link #tryGetOrInsertOrNull(BiFunction, Object, Object)} for the * general contract. */ + @StrategyConsumer @Nullable public TEntry tryGetOrInsertOrNull( @Nonnull Function3 factory, @@ -1011,6 +1032,7 @@ public TEntry tryGetOrInsertOrNull( } /** Four key components; see {@link #tryGetOrInsertOrNull(BiFunction, Object, Object)}. */ + @StrategyConsumer @Nullable public TEntry tryGetOrInsertOrNull( @Nonnull Function4 factory, @@ -1021,6 +1043,46 @@ public TEntry tryGetOrInsertOrNull( return state == null ? null : finish(factory.apply(a, b, c, d)); } + /** + * {@link Maybe}-wrapping counterpart of {@link #tryGetOrInsertOrNull(Function, Object)}, for + * callers who'd rather make the "this can fail unlike unbounded collections" outcome visible in + * the return type than rely on a {@code null} check — mirrors {@link D1#tryGetOrCreate} / + * {@link D2#tryGetOrCreate} wrapping their own {@code ...OrNull} methods. + */ + @Nonnull + public Maybe tryGetOrInsert( + @Strategy @Nonnull Function factory, A a) { + return Maybe.of(tryGetOrInsertOrNull(factory, a)); + } + + /** Two key components; see {@link #tryGetOrInsert(Function, Object)}. */ + @Nonnull + public Maybe tryGetOrInsert( + @Strategy @Nonnull BiFunction factory, A a, B b) { + return Maybe.of(tryGetOrInsertOrNull(factory, a, b)); + } + + /** Three key components; see {@link #tryGetOrInsert(Function, Object)}. */ + @Nonnull + public Maybe tryGetOrInsert( + @Nonnull Function3 factory, + A a, + B b, + C c) { + return Maybe.of(tryGetOrInsertOrNull(factory, a, b, c)); + } + + /** Four key components; see {@link #tryGetOrInsert(Function, Object)}. */ + @Nonnull + public Maybe tryGetOrInsert( + @Nonnull Function4 factory, + A a, + B b, + C c, + D d) { + return Maybe.of(tryGetOrInsertOrNull(factory, a, b, c, d)); + } + private TEntry finish(@Nonnull TEntry newEntry) { synchronized (getTableWriteLock(state)) { int index = bucketIndex(state.buckets, newEntry.keyHash); @@ -1047,12 +1109,14 @@ public void close() { } /** Three-argument analogue of {@link java.util.function.BiFunction}. */ + @Strategy @FunctionalInterface public interface Function3 { R apply(A a, B b, C c); } /** Four-argument analogue of {@link java.util.function.BiFunction}. */ + @Strategy @FunctionalInterface public interface Function4 { R apply(A a, B b, C c, D d); @@ -1067,7 +1131,7 @@ public interface Function4 { * #insertReserved}; abandoning it permanently consumes capacity. */ public static > boolean tryReserveOrEvict( - @Nonnull State state, @Nonnull Predicate evictable) { + @Nonnull State state, @Strategy @Nonnull Predicate evictable) { synchronized (getTableWriteLock(state)) { return state.sizeManager.tryReserveOrEvict(state.buckets, evictable); } @@ -1080,7 +1144,7 @@ public static > boolean tryReserveOrEvict( */ @Nullable public static > TEntry evictOne( - @Nonnull State state, @Nonnull Predicate evictable) { + @Nonnull State state, @Strategy @Nonnull Predicate evictable) { synchronized (getTableWriteLock(state)) { return state.sizeManager.evictOne(state.buckets, evictable); } @@ -1091,7 +1155,7 @@ public static > TEntry evictOne( * returns how many went. Self-locking. */ public static > int evictAll( - @Nonnull State state, @Nonnull Predicate evictable) { + @Nonnull State state, @Strategy @Nonnull Predicate evictable) { synchronized (getTableWriteLock(state)) { return state.sizeManager.evictAll(state.buckets, evictable); } @@ -1240,6 +1304,45 @@ public static > TEntry bucketAt( return bucketAt(state.buckets, index); } + /** + * Returns a lock-free iterator over the bucket chain that {@code keyHash} maps to, starting from + * {@link #bucketFor(AtomicReferenceArray, long)}. Each {@link Iterator#next()} call follows + * {@link Entry#next()}, so the iterator reflects entries linked at the time each step runs rather + * than a point-in-time snapshot -- entries inserted ahead of the iterator's current position + * after iteration starts may or may not be observed, and a concurrently removed entry remains + * reachable because {@code unlink()} deliberately retains its {@code next} link for in-flight + * readers. + */ + @Nonnull + public static > Iterator hashIterator( + @Nonnull AtomicReferenceArray buckets, long keyHash) { + return new Iterator() { + private TEntry next = bucketFor(buckets, keyHash); + + @Override + public boolean hasNext() { + return next != null; + } + + @Override + public TEntry next() { + TEntry current = next; + if (current == null) { + throw new NoSuchElementException(); + } + next = current.next(); + return current; + } + }; + } + + /** {@link #hashIterator(AtomicReferenceArray, long)} over a {@link State}. */ + @Nonnull + public static > Iterator hashIterator( + @Nonnull State state, long keyHash) { + return hashIterator(state.buckets, keyHash); + } + /** * Publishes {@code entry} as the head of bucket {@code index}. The helper writes the entry's * {@code next} link before the volatile {@link AtomicReferenceArray#set}; a volatile bucket read @@ -1332,10 +1435,11 @@ public static > void unlink( * predicate sees a stable table and concurrent writers are excluded; lock-free readers continue * throughout. */ + @StrategyConsumer public static > boolean removeIf( @Nonnull AtomicReferenceArray buckets, @Nonnull AtomicInteger size, - @Nonnull Predicate predicate) { + @Strategy @Nonnull Predicate predicate) { synchronized (getTableWriteLock(buckets)) { boolean removed = false; for (int i = 0; i < buckets.length(); i++) { @@ -1360,8 +1464,9 @@ public static > boolean removeIf( * occupancy with a {@link State} instead of a bare counter — used by {@link D1#removeIf} and * {@link D2#removeIf}. */ + @StrategyConsumer public static > boolean removeIf( - @Nonnull State state, @Nonnull Predicate predicate) { + @Nonnull State state, @Strategy @Nonnull Predicate predicate) { AtomicReferenceArray buckets = state.buckets; synchronized (getTableWriteLock(state)) { boolean removed = false; @@ -1390,7 +1495,8 @@ public static > boolean removeIf( *

    The sink must not throw. If it does, the partial drain is not rolled back. */ public static > void drain( - @Nonnull AtomicReferenceArray buckets, @Nonnull Consumer sink) { + @Nonnull AtomicReferenceArray buckets, + @Strategy @Nonnull Consumer sink) { drainCounting(buckets, sink); } @@ -1399,8 +1505,10 @@ public static > void drain( * sink}, so a {@link State} form can subtract exactly that from its {@link SizeManager} instead * of zeroing. The count is free here: the sweep already visits every entry. */ + @StrategyConsumer private static > int drainCounting( - @Nonnull AtomicReferenceArray buckets, @Nonnull Consumer sink) { + @Nonnull AtomicReferenceArray buckets, + @Strategy @Nonnull Consumer sink) { int removed = 0; synchronized (getTableWriteLock(buckets)) { for (int i = 0; i < buckets.length(); i++) { @@ -1422,15 +1530,16 @@ private static > int drainCounting( public static > void drain( @Nonnull AtomicReferenceArray buckets, C context, - @Nonnull BiConsumer sink) { + @Strategy @Nonnull BiConsumer sink) { drainCounting(buckets, context, sink); } /** {@link #drainCounting(AtomicReferenceArray, Consumer)}, context-passing form. */ + @StrategyConsumer private static > int drainCounting( @Nonnull AtomicReferenceArray buckets, C context, - @Nonnull BiConsumer sink) { + @Strategy @Nonnull BiConsumer sink) { int removed = 0; synchronized (getTableWriteLock(buckets)) { for (int i = 0; i < buckets.length(); i++) { @@ -1455,7 +1564,7 @@ private static > int drainCounting( * rather than as a pair the caller has to remember. */ public static > void drain( - @Nonnull State state, @Nonnull Consumer sink) { + @Nonnull State state, @Strategy @Nonnull Consumer sink) { synchronized (getTableWriteLock(state)) { state.sizeManager.release(drainCounting(state.buckets, sink)); } @@ -1465,7 +1574,7 @@ public static > void drain( public static > void drain( @Nonnull State state, C context, - @Nonnull BiConsumer sink) { + @Strategy @Nonnull BiConsumer sink) { synchronized (getTableWriteLock(state)) { state.sizeManager.release(drainCounting(state.buckets, context, sink)); } @@ -1513,8 +1622,10 @@ public static void clear(@Nonnull State state) { } } + @StrategyConsumer public static > void forEach( - @Nonnull AtomicReferenceArray buckets, @Nonnull Consumer consumer) { + @Nonnull AtomicReferenceArray buckets, + @Strategy @Nonnull Consumer consumer) { for (int i = 0; i < buckets.length(); i++) { for (TEntry curEntry = buckets.get(i); curEntry != null; curEntry = curEntry.next()) { consumer.accept(curEntry); @@ -1522,10 +1633,11 @@ public static > void forEach( } } + @StrategyConsumer public static > void forEach( @Nonnull AtomicReferenceArray buckets, C context, - @Nonnull BiConsumer consumer) { + @Strategy @Nonnull BiConsumer consumer) { for (int i = 0; i < buckets.length(); i++) { for (TEntry curEntry = buckets.get(i); curEntry != null; curEntry = curEntry.next()) { consumer.accept(context, curEntry); @@ -1535,7 +1647,7 @@ public static > void forEach( /** {@link #forEach(AtomicReferenceArray, Consumer)} over a {@link State}. */ public static > void forEach( - @Nonnull State state, @Nonnull Consumer consumer) { + @Nonnull State state, @Strategy @Nonnull Consumer consumer) { forEach(state.buckets, consumer); } @@ -1543,7 +1655,7 @@ public static > void forEach( public static > void forEach( @Nonnull State state, C context, - @Nonnull BiConsumer consumer) { + @Strategy @Nonnull BiConsumer consumer) { forEach(state.buckets, context, consumer); } } diff --git a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD1Test.java b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD1Test.java index 62985e64658..499470743c8 100644 --- a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD1Test.java +++ b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD1Test.java @@ -80,7 +80,7 @@ void forEachVisitsAllEntries() { table.tryGetOrCreateOrNull("b", k -> new StringEntry(k, 2)); table.tryGetOrCreateOrNull("c", k -> new StringEntry(k, 3)); Set seen = new HashSet<>(); - table.forEach(e -> seen.add(e.key)); + table.forEach(e -> seen.add(e.key())); assertEquals(3, seen.size()); assertTrue(seen.contains("a")); assertTrue(seen.contains("b")); @@ -94,7 +94,7 @@ void forEachWithContextPassesContext() { table.tryGetOrCreateOrNull("x", k -> new StringEntry(k, 10)); table.tryGetOrCreateOrNull("y", k -> new StringEntry(k, 20)); Set seen = new HashSet<>(); - table.forEach(seen, (ctx, e) -> ctx.add(e.key)); + table.forEach(seen, (ctx, e) -> ctx.add(e.key())); assertEquals(2, seen.size()); assertTrue(seen.contains("x")); assertTrue(seen.contains("y")); @@ -258,7 +258,7 @@ void removeIfRemovesMatchingEntries() { assertTrue(removed); assertEquals(5, table.size()); Set seen = new HashSet<>(); - table.forEach(e -> seen.add(e.key)); + table.forEach(e -> seen.add(e.key())); assertEquals(5, seen.size()); for (String key : seen) { assertNotNull(table.get(key)); @@ -301,7 +301,7 @@ void drainRemovesEveryEntryAndFeedsSink() { int[] sum = {0}; table.drain( e -> { - drained.add(e.key); + drained.add(e.key()); sum[0] += e.value; }); @@ -323,7 +323,7 @@ void drainWithContextFeedsSink() { table.tryGetOrCreateOrNull("b", k -> new StringEntry(k, 2)); Set drained = new HashSet<>(); - table.drain(drained, (ctx, e) -> ctx.add(e.key)); + table.drain(drained, (ctx, e) -> ctx.add(e.key())); assertEquals(new HashSet<>(Arrays.asList("a", "b")), drained); assertEquals(0, table.size()); @@ -421,7 +421,7 @@ void tryGetOrCreateOrEvictEvictsWhenFullAndInsertsNewEntry() { Maybe created = table.tryGetOrCreateOrEvict("new", k -> new StringEntry(k, 2), e -> true); assertTrue(created.isPresent()); - assertEquals("new", created.getOrNull().key); + assertEquals("new", created.getOrNull().key()); assertEquals(1, table.size()); assertNull(table.get("old")); assertSame(created.getOrNull(), table.get("new")); @@ -464,8 +464,9 @@ void tryGetOrCreateOrEvictOrNullEvictionRunsBeforeThrowingCreator() { assertNull(table.get("new")); } + /** Entry holding a key plus one mutable {@code int} payload. */ private static final class StringEntry extends ConcurrentHashtable.D1.Entry { - final int value; + volatile int value; StringEntry(String key, int value) { super(key); @@ -473,6 +474,14 @@ private static final class StringEntry extends ConcurrentHashtable.D1.Entry { + CollidingEntry(CollidingKey key) { + super(key); + } + } + /** Key with a fixed hashCode to force deterministic bucket placement. */ private static final class CollidingKey { final String label; @@ -497,11 +506,4 @@ public boolean equals(Object o) { return fixedHash == that.fixedHash && label.equals(that.label); } } - - private static final class CollidingEntry - extends ConcurrentHashtable.D1.Entry { - CollidingEntry(CollidingKey key) { - super(key); - } - } } diff --git a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD2Test.java b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD2Test.java index 69422a07caf..38190b52a1d 100644 --- a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD2Test.java +++ b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD2Test.java @@ -45,8 +45,8 @@ void getOrCreateOnMissBuildsEntryViaCreator() { return new PairEntry(k1, k2); }); assertNotNull(created); - assertEquals("a", created.key1); - assertEquals(Integer.valueOf(1), created.key2); + assertEquals("a", created.key1()); + assertEquals(Integer.valueOf(1), created.key2()); assertEquals(1, table.size()); assertEquals(1, createCount[0]); assertSame(created, table.get("a", 1)); @@ -78,7 +78,7 @@ void forEachVisitsBothPairs() { table.tryGetOrCreateOrNull("a", 1, PairEntry::new); table.tryGetOrCreateOrNull("b", 2, PairEntry::new); Set seen = new HashSet<>(); - table.forEach(e -> seen.add(e.key1 + ":" + e.key2)); + table.forEach(e -> seen.add(e.key1() + ":" + e.key2())); assertEquals(2, seen.size()); assertTrue(seen.contains("a:1")); assertTrue(seen.contains("b:2")); @@ -91,7 +91,7 @@ void forEachWithContextPassesContextToConsumer() { table.tryGetOrCreateOrNull("a", 1, PairEntry::new); table.tryGetOrCreateOrNull("b", 2, PairEntry::new); Set seen = new HashSet<>(); - table.forEach(seen, (ctx, e) -> ctx.add(e.key1 + ":" + e.key2)); + table.forEach(seen, (ctx, e) -> ctx.add(e.key1() + ":" + e.key2())); assertEquals(2, seen.size()); assertTrue(seen.contains("a:1")); assertTrue(seen.contains("b:2")); @@ -246,11 +246,11 @@ void removeIfRemovesMatchingEntries() { for (int i = 0; i < 10; i++) { table.tryGetOrCreateOrNull("k", i, PairEntry::new); } - boolean removed = table.removeIf(e -> e.key2 % 2 == 0); // removes key2 0,2,4,6,8 + boolean removed = table.removeIf(e -> e.key2() % 2 == 0); // removes key2 0,2,4,6,8 assertTrue(removed); assertEquals(5, table.size()); Set seen = new HashSet<>(); - table.forEach(e -> seen.add(e.key1 + ":" + e.key2)); + table.forEach(e -> seen.add(e.key1() + ":" + e.key2())); assertEquals(5, seen.size()); } @@ -286,7 +286,7 @@ void drainRemovesEveryEntryAndFeedsSink() { table.tryGetOrCreateOrNull("b", 1, PairEntry::new); Set drained = new HashSet<>(); - table.drain(e -> drained.add(e.key1 + ":" + e.key2)); + table.drain(e -> drained.add(e.key1() + ":" + e.key2())); assertEquals(new HashSet<>(Arrays.asList("a:1", "a:2", "b:1")), drained); assertEquals(0, table.size()); @@ -304,7 +304,7 @@ void drainWithContextFeedsSink() { table.tryGetOrCreateOrNull("b", 2, PairEntry::new); Set drained = new HashSet<>(); - table.drain(drained, (ctx, e) -> ctx.add(e.key1 + ":" + e.key2)); + table.drain(drained, (ctx, e) -> ctx.add(e.key1() + ":" + e.key2())); assertEquals(new HashSet<>(Arrays.asList("a:1", "b:2")), drained); assertEquals(0, table.size()); @@ -348,7 +348,7 @@ void tryGetOrCreateOrEvictEvictsWhenFullAndInsertsNewEntry() { Maybe created = table.tryGetOrCreateOrEvict("new", 2, PairEntry::new, e -> true); assertTrue(created.isPresent()); - assertEquals("new", created.getOrNull().key1); + assertEquals("new", created.getOrNull().key1()); assertEquals(1, table.size()); assertNull(table.get("old", 1)); assertSame(created.getOrNull(), table.get("new", 2)); @@ -391,6 +391,7 @@ void tryGetOrCreateOrEvictOrNullEvictionRunsBeforeThrowingCreator() { assertNull(table.get("new", 2)); } + /** Entry with no payload beyond its two key parts, used to exercise the D2 identity/API. */ private static final class PairEntry extends ConcurrentHashtable.D2.Entry { PairEntry(String key1, Integer key2) { diff --git a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableReservationTest.java b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableReservationTest.java index 55650a72b2f..b9d89a3b119 100644 --- a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableReservationTest.java +++ b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableReservationTest.java @@ -10,7 +10,7 @@ import javax.annotation.Nonnull; import org.junit.jupiter.api.Test; -/** Exercises {@link ConcurrentHashtable#reserve} and {@link ConcurrentHashtable.Reservation}. */ +/** Exercises {@link ConcurrentHashtable#tryReserve} and {@link ConcurrentHashtable.Reservation}. */ class ConcurrentHashtableReservationTest { private static final class TestEntry extends ConcurrentHashtable.Entry { @@ -33,7 +33,7 @@ void tryGetOrInsertOrNullInsertsOnMissAndFindsOnHit() { ConcurrentHashtable.createBounded(TestEntry.class, 4); TestEntry first; - try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.reserve(state)) { + try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { assertTrue(r.isPresent()); first = r.tryGetOrInsertOrNull(TestEntry::new, 1); } @@ -43,7 +43,7 @@ void tryGetOrInsertOrNullInsertsOnMissAndFindsOnHit() { // Reserving again for a key that already exists should discard the reservation and return the // existing entry, not double-insert or leak the claimed slot. TestEntry second; - try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.reserve(state)) { + try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { second = r.tryGetOrInsertOrNull(TestEntry::new, 1); } assertSame(first, second); @@ -54,14 +54,14 @@ void tryGetOrInsertOrNullInsertsOnMissAndFindsOnHit() { void reserveOnFullTableIsAbsentAndSkipsTheFactory() { ConcurrentHashtable.State state = ConcurrentHashtable.createBounded(TestEntry.class, 1); - try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.reserve(state)) { + try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { r.tryGetOrInsertOrNull(TestEntry::new, 1); } assertTrue(ConcurrentHashtable.isFull(state)); AtomicInteger factoryCalls = new AtomicInteger(); TestEntry result; - try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.reserve(state)) { + try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { assertFalse(r.isPresent()); result = r.tryGetOrInsertOrNull( @@ -80,7 +80,7 @@ void reserveOnFullTableIsAbsentAndSkipsTheFactory() { void closeCancelsAnUnconsumedReservation() { ConcurrentHashtable.State state = ConcurrentHashtable.createBounded(TestEntry.class, 1); - try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.reserve(state)) { + try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { assertTrue(r.isPresent()); // Deliberately not consuming the reservation. } @@ -88,11 +88,31 @@ void closeCancelsAnUnconsumedReservation() { assertFalse(ConcurrentHashtable.isFull(state)); } + @Test + void tryGetOrInsertWrapsResultInMaybe() { + ConcurrentHashtable.State state = + ConcurrentHashtable.createBounded(TestEntry.class, 1); + + Maybe present; + try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { + present = r.tryGetOrInsert(TestEntry::new, 1); + } + assertTrue(present.isPresent()); + assertEquals(1, present.getOrNull().value); + + Maybe absent; + try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { + absent = r.tryGetOrInsert(TestEntry::new, 2); + } + assertFalse(absent.isPresent()); + assertNull(absent.getOrNull()); + } + @Test void closeOnAnAbsentReservationIsANoOp() { ConcurrentHashtable.State state = ConcurrentHashtable.createBounded(TestEntry.class, 0); - try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.reserve(state)) { + try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { assertFalse(r.isPresent()); } assertEquals(0, ConcurrentHashtable.estimateSize(state)); @@ -141,7 +161,8 @@ void tryGetOrInsertOrNullSupportsUpToFourComponents() { ConcurrentHashtable.State state3 = ConcurrentHashtable.createBounded(ThreePartEntry.class, 2); ThreePartEntry three; - try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.reserve(state3)) { + try (ConcurrentHashtable.Reservation r = + ConcurrentHashtable.tryReserve(state3)) { three = r.tryGetOrInsertOrNull(ThreePartEntry::new, "x", "y", "z"); } assertEquals("x", three.a); @@ -151,7 +172,8 @@ void tryGetOrInsertOrNullSupportsUpToFourComponents() { ConcurrentHashtable.State state4 = ConcurrentHashtable.createBounded(FourPartEntry.class, 2); FourPartEntry four; - try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.reserve(state4)) { + try (ConcurrentHashtable.Reservation r = + ConcurrentHashtable.tryReserve(state4)) { four = r.tryGetOrInsertOrNull(FourPartEntry::new, "w", "x", "y", "z"); } assertEquals("w", four.a); From f34842ecf555aaf1c5f9b8c91f945dd28595e0a6 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Thu, 10 Sep 2026 21:57:35 -0400 Subject: [PATCH 4/9] Filter hashIterator to exact keyHash matches hashIterator's bucket chain can contain entries whose keyHash differs (hash collisions on bucketIndex), so it now skips those internally rather than making every caller repeat curEntry.keyHash == keyHash -- consistent with how D1/D2's own lookups already pre-filter by keyHash before calling matches(). Drops the now-redundant keyHash check from LogCollector.addLogMessage. Co-Authored-By: Claude Sonnet 5 --- .../trace/api/telemetry/LogCollector.java | 3 -- .../trace/util/ConcurrentHashtable.java | 29 +++++++++++++------ 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java b/internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java index 2de274f93a7..d5aaf7c0c72 100644 --- a/internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java +++ b/internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java @@ -63,9 +63,6 @@ public void addLogMessage( for (Iterator it = ConcurrentHashtable.hashIterator(rawLogMessages, keyHash); it.hasNext(); ) { RawLogMessage existing = it.next(); - if (existing.keyHash != keyHash) { - continue; - } if (throwable != null && existing.throwable != null && existing.throwable != throwable) { if (throwableStackTrace == null) { throwableStackTrace = throwable.getStackTrace(); diff --git a/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java b/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java index 4d4b2720057..93681dd61da 100644 --- a/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java +++ b/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java @@ -1305,19 +1305,30 @@ public static > TEntry bucketAt( } /** - * Returns a lock-free iterator over the bucket chain that {@code keyHash} maps to, starting from - * {@link #bucketFor(AtomicReferenceArray, long)}. Each {@link Iterator#next()} call follows - * {@link Entry#next()}, so the iterator reflects entries linked at the time each step runs rather - * than a point-in-time snapshot -- entries inserted ahead of the iterator's current position - * after iteration starts may or may not be observed, and a concurrently removed entry remains - * reachable because {@code unlink()} deliberately retains its {@code next} link for in-flight - * readers. + * Returns a lock-free iterator over the candidates for {@code keyHash}: entries in the bucket + * chain that {@code keyHash} maps to (starting from {@link #bucketFor(AtomicReferenceArray, + * long)}) whose own {@link Entry#keyHash} equals it, skipping any other entry sharing the same + * bucket via hash collision on {@link #bucketIndex}. Callers only need a {@code matches} check + * against the entries this yields, not a {@code keyHash} check of their own. + * + *

    Each step follows {@link Entry#next()}, so the iterator reflects entries linked at the time + * each step runs rather than a point-in-time snapshot -- entries inserted ahead of the iterator's + * current position after iteration starts may or may not be observed, and a concurrently removed + * entry remains reachable because {@code unlink()} deliberately retains its {@code next} link for + * in-flight readers. */ @Nonnull public static > Iterator hashIterator( @Nonnull AtomicReferenceArray buckets, long keyHash) { return new Iterator() { - private TEntry next = bucketFor(buckets, keyHash); + private TEntry next = advance(bucketFor(buckets, keyHash)); + + private TEntry advance(TEntry candidate) { + while (candidate != null && candidate.keyHash != keyHash) { + candidate = candidate.next(); + } + return candidate; + } @Override public boolean hasNext() { @@ -1330,7 +1341,7 @@ public TEntry next() { if (current == null) { throw new NoSuchElementException(); } - next = current.next(); + next = advance(current.next()); return current; } }; From 9934c8298d6118d8f710146a0bdb852937ae2e05 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Thu, 10 Sep 2026 22:10:17 -0400 Subject: [PATCH 5/9] Move Reservation factory params to the end, add hashIterable, rename loop var Reservation.tryGetOrInsertOrNull/tryGetOrInsert took their Strategy factory first, ahead of the key components -- inconsistent with D1/D2's own tryGetOrCreate(key, creator) convention of putting the functional-interface parameter last. Also adds hashIterable, a thin Iterable wrapper around hashIterator so callers can write a plain for-each loop, and renames LogCollector's loop variable from `it` to `iter`. Co-Authored-By: Claude Sonnet 5 --- .../trace/api/telemetry/LogCollector.java | 7 +- .../trace/util/ConcurrentHashtable.java | 72 ++++++++++++------- .../ConcurrentHashtableReservationTest.java | 18 ++--- 3 files changed, 57 insertions(+), 40 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java b/internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java index d5aaf7c0c72..2528cebddaa 100644 --- a/internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java +++ b/internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java @@ -6,7 +6,6 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.Iterator; import java.util.List; import java.util.Objects; import java.util.concurrent.atomic.AtomicInteger; @@ -60,9 +59,7 @@ public void addLogMessage( // Lock-free scan first: most calls are re-observations of an already-seen message, so this // avoids paying for a reservation and a RawLogMessage allocation on the common path. - for (Iterator it = ConcurrentHashtable.hashIterator(rawLogMessages, keyHash); - it.hasNext(); ) { - RawLogMessage existing = it.next(); + for (RawLogMessage existing : ConcurrentHashtable.hashIterable(rawLogMessages, keyHash)) { if (throwable != null && existing.throwable != null && existing.throwable != throwable) { if (throwableStackTrace == null) { throwableStackTrace = throwable.getStackTrace(); @@ -78,7 +75,7 @@ public void addLogMessage( ConcurrentHashtable.tryReserve(rawLogMessages)) { // TODO: We could emit a metric for dropped logs when the reservation is empty (table full). RawLogMessage rawLogMessage = - reservation.tryGetOrInsertOrNull(RawLogMessage::new, logLevel, message, throwable, tags); + reservation.tryGetOrInsertOrNull(logLevel, message, throwable, tags, RawLogMessage::new); if (rawLogMessage != null) { rawLogMessage.count.incrementAndGet(); } diff --git a/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java b/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java index 93681dd61da..0dab75f47f4 100644 --- a/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java +++ b/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java @@ -941,7 +941,7 @@ public static > boolean tryReserveSlot( * * 

    {@code
    * try (Reservation r = ConcurrentHashtable.tryReserve(state)) {
-   *   return r.tryGetOrInsertOrNull(TEntry::new, component1, component2, component3);
+   *   return r.tryGetOrInsertOrNull(component1, component2, component3, TEntry::new);
    * }
    * }
    * @@ -966,8 +966,8 @@ public static > Reservation tryReserve( * slot and auto-cancelling it on {@link #close} if it's never consumed. A single {@code * Reservation} must be used for at most one {@code tryGetOrInsertOrNull} call. * - *

    Overloaded up to 4 key components ({@link #tryGetOrInsertOrNull(Function, Object)} through - * {@link #tryGetOrInsertOrNull(Function4, Object, Object, Object, Object)}) so a non-capturing + *

    Overloaded up to 4 key components ({@link #tryGetOrInsertOrNull(Object, Function)} through + * {@link #tryGetOrInsertOrNull(Object, Object, Object, Object, Function4)}) so a non-capturing * method reference can build the entry directly from its natural constructor arguments, without * an intermediate holder object or a capturing lambda. * @@ -988,13 +988,13 @@ public boolean isPresent() { } /** - * One key component; see {@link #tryGetOrInsertOrNull(BiFunction, Object, Object)} for the + * One key component; see {@link #tryGetOrInsertOrNull(Object, Object, BiFunction)} for the * general contract. */ @StrategyConsumer @Nullable public TEntry tryGetOrInsertOrNull( - @Strategy @Nonnull Function factory, A a) { + A a, @Strategy @Nonnull Function factory) { return state == null ? null : finish(factory.apply(a)); } @@ -1013,74 +1013,76 @@ public TEntry tryGetOrInsertOrNull( @StrategyConsumer @Nullable public TEntry tryGetOrInsertOrNull( - @Strategy @Nonnull BiFunction factory, A a, B b) { + A a, B b, @Strategy @Nonnull BiFunction factory) { return state == null ? null : finish(factory.apply(a, b)); } /** - * Three key components; see {@link #tryGetOrInsertOrNull(BiFunction, Object, Object)} for the + * Three key components; see {@link #tryGetOrInsertOrNull(Object, Object, BiFunction)} for the * general contract. */ @StrategyConsumer @Nullable public TEntry tryGetOrInsertOrNull( - @Nonnull Function3 factory, A a, B b, - C c) { + C c, + @Strategy @Nonnull Function3 factory) { return state == null ? null : finish(factory.apply(a, b, c)); } - /** Four key components; see {@link #tryGetOrInsertOrNull(BiFunction, Object, Object)}. */ + /** Four key components; see {@link #tryGetOrInsertOrNull(Object, Object, BiFunction)}. */ @StrategyConsumer @Nullable public TEntry tryGetOrInsertOrNull( - @Nonnull Function4 factory, A a, B b, C c, - D d) { + D d, + @Strategy @Nonnull + Function4 factory) { return state == null ? null : finish(factory.apply(a, b, c, d)); } /** - * {@link Maybe}-wrapping counterpart of {@link #tryGetOrInsertOrNull(Function, Object)}, for + * {@link Maybe}-wrapping counterpart of {@link #tryGetOrInsertOrNull(Object, Function)}, for * callers who'd rather make the "this can fail unlike unbounded collections" outcome visible in * the return type than rely on a {@code null} check — mirrors {@link D1#tryGetOrCreate} / * {@link D2#tryGetOrCreate} wrapping their own {@code ...OrNull} methods. */ @Nonnull public Maybe tryGetOrInsert( - @Strategy @Nonnull Function factory, A a) { - return Maybe.of(tryGetOrInsertOrNull(factory, a)); + A a, @Strategy @Nonnull Function factory) { + return Maybe.of(tryGetOrInsertOrNull(a, factory)); } - /** Two key components; see {@link #tryGetOrInsert(Function, Object)}. */ + /** Two key components; see {@link #tryGetOrInsert(Object, Function)}. */ @Nonnull public Maybe tryGetOrInsert( - @Strategy @Nonnull BiFunction factory, A a, B b) { - return Maybe.of(tryGetOrInsertOrNull(factory, a, b)); + A a, B b, @Strategy @Nonnull BiFunction factory) { + return Maybe.of(tryGetOrInsertOrNull(a, b, factory)); } - /** Three key components; see {@link #tryGetOrInsert(Function, Object)}. */ + /** Three key components; see {@link #tryGetOrInsert(Object, Function)}. */ @Nonnull public Maybe tryGetOrInsert( - @Nonnull Function3 factory, A a, B b, - C c) { - return Maybe.of(tryGetOrInsertOrNull(factory, a, b, c)); + C c, + @Strategy @Nonnull Function3 factory) { + return Maybe.of(tryGetOrInsertOrNull(a, b, c, factory)); } - /** Four key components; see {@link #tryGetOrInsert(Function, Object)}. */ + /** Four key components; see {@link #tryGetOrInsert(Object, Function)}. */ @Nonnull public Maybe tryGetOrInsert( - @Nonnull Function4 factory, A a, B b, C c, - D d) { - return Maybe.of(tryGetOrInsertOrNull(factory, a, b, c, d)); + D d, + @Strategy @Nonnull + Function4 factory) { + return Maybe.of(tryGetOrInsertOrNull(a, b, c, d, factory)); } private TEntry finish(@Nonnull TEntry newEntry) { @@ -1354,6 +1356,24 @@ public static > Iterator hashIterator( return hashIterator(state.buckets, keyHash); } + /** + * {@link Iterable} wrapper around {@link #hashIterator(AtomicReferenceArray, long)}, for callers + * that want a plain for-each loop over the candidates for {@code keyHash} rather than driving the + * {@link Iterator} by hand. + */ + @Nonnull + public static > Iterable hashIterable( + @Nonnull AtomicReferenceArray buckets, long keyHash) { + return () -> hashIterator(buckets, keyHash); + } + + /** {@link #hashIterable(AtomicReferenceArray, long)} over a {@link State}. */ + @Nonnull + public static > Iterable hashIterable( + @Nonnull State state, long keyHash) { + return hashIterable(state.buckets, keyHash); + } + /** * Publishes {@code entry} as the head of bucket {@code index}. The helper writes the entry's * {@code next} link before the volatile {@link AtomicReferenceArray#set}; a volatile bucket read diff --git a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableReservationTest.java b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableReservationTest.java index b9d89a3b119..7d0bb25d714 100644 --- a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableReservationTest.java +++ b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableReservationTest.java @@ -35,7 +35,7 @@ void tryGetOrInsertOrNullInsertsOnMissAndFindsOnHit() { TestEntry first; try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { assertTrue(r.isPresent()); - first = r.tryGetOrInsertOrNull(TestEntry::new, 1); + first = r.tryGetOrInsertOrNull(1, TestEntry::new); } assertEquals(1, first.value); assertEquals(1, ConcurrentHashtable.estimateSize(state)); @@ -44,7 +44,7 @@ void tryGetOrInsertOrNullInsertsOnMissAndFindsOnHit() { // existing entry, not double-insert or leak the claimed slot. TestEntry second; try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { - second = r.tryGetOrInsertOrNull(TestEntry::new, 1); + second = r.tryGetOrInsertOrNull(1, TestEntry::new); } assertSame(first, second); assertEquals(1, ConcurrentHashtable.estimateSize(state)); @@ -55,7 +55,7 @@ void reserveOnFullTableIsAbsentAndSkipsTheFactory() { ConcurrentHashtable.State state = ConcurrentHashtable.createBounded(TestEntry.class, 1); try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { - r.tryGetOrInsertOrNull(TestEntry::new, 1); + r.tryGetOrInsertOrNull(1, TestEntry::new); } assertTrue(ConcurrentHashtable.isFull(state)); @@ -65,11 +65,11 @@ void reserveOnFullTableIsAbsentAndSkipsTheFactory() { assertFalse(r.isPresent()); result = r.tryGetOrInsertOrNull( + 2, v -> { factoryCalls.incrementAndGet(); return new TestEntry(v); - }, - 2); + }); } assertNull(result); assertEquals(0, factoryCalls.get()); @@ -95,14 +95,14 @@ void tryGetOrInsertWrapsResultInMaybe() { Maybe present; try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { - present = r.tryGetOrInsert(TestEntry::new, 1); + present = r.tryGetOrInsert(1, TestEntry::new); } assertTrue(present.isPresent()); assertEquals(1, present.getOrNull().value); Maybe absent; try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { - absent = r.tryGetOrInsert(TestEntry::new, 2); + absent = r.tryGetOrInsert(2, TestEntry::new); } assertFalse(absent.isPresent()); assertNull(absent.getOrNull()); @@ -163,7 +163,7 @@ void tryGetOrInsertOrNullSupportsUpToFourComponents() { ThreePartEntry three; try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state3)) { - three = r.tryGetOrInsertOrNull(ThreePartEntry::new, "x", "y", "z"); + three = r.tryGetOrInsertOrNull("x", "y", "z", ThreePartEntry::new); } assertEquals("x", three.a); assertEquals("y", three.b); @@ -174,7 +174,7 @@ void tryGetOrInsertOrNullSupportsUpToFourComponents() { FourPartEntry four; try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state4)) { - four = r.tryGetOrInsertOrNull(FourPartEntry::new, "w", "x", "y", "z"); + four = r.tryGetOrInsertOrNull("w", "x", "y", "z", FourPartEntry::new); } assertEquals("w", four.a); assertEquals("x", four.b); From bf02bf95a275be065fe258aaed9c7b942501f5ae Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Thu, 10 Sep 2026 22:12:53 -0400 Subject: [PATCH 6/9] Type Entry.next as TEntry instead of Entry setNext/next used their own generic type parameter with an unchecked cast, but every caller already assigns the result to a TEntry-typed variable -- the field can just be declared as TEntry directly, since a bucket chain's entries always share the same concrete TEntry as the array that holds them. Co-Authored-By: Claude Sonnet 5 --- .../java/datadog/trace/util/ConcurrentHashtable.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java b/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java index 0dab75f47f4..dd49e1dcbe6 100644 --- a/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java +++ b/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java @@ -82,7 +82,7 @@ private ConcurrentHashtable() {} */ public abstract static class Entry> { public final long keyHash; - private volatile Entry next = null; + private volatile TEntry next = null; protected Entry(long keyHash) { this.keyHash = keyHash; @@ -91,14 +91,13 @@ protected Entry(long keyHash) { // Package-private: the only writers are the static insert/remove building blocks // (insertHeadEntry, unlink) on the enclosing class, which reach it via the Entry bound. Custom // tables mutate chains through those helpers, never by touching next directly. - final > void setNext(TNext next) { + final void setNext(TEntry next) { this.next = next; } - @SuppressWarnings("unchecked") @Nullable - public final > TNext next() { - return (TNext) this.next; + public final TEntry next() { + return this.next; } /** From a62095fa42161f6f96c2cadf0fb80a017ec160f8 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Thu, 10 Sep 2026 22:26:11 -0400 Subject: [PATCH 7/9] Add Reservation escape hatch for pre-built entries, rename isPresent to isReserved tryGetOrInsertOrNull(TEntry)/tryGetOrInsert(TEntry) expose finish() directly for callers with more than 4 key components or that want to avoid boxing primitives into a Function's type argument. isPresent() read like Optional.isPresent() (an entry already exists), but it actually means the reservation succeeded / the table had room -- true even before any entry is created. isReserved() matches that meaning. Co-Authored-By: Claude Sonnet 5 --- .../trace/util/ConcurrentHashtable.java | 34 +++++++++++++++++-- .../ConcurrentHashtableReservationTest.java | 8 ++--- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java b/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java index dd49e1dcbe6..3d1cfa050d6 100644 --- a/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java +++ b/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java @@ -951,8 +951,9 @@ public static > boolean tryReserveSlot( * already visible lock-free. * *

    Always returns a non-null handle — even when the table is full — so the caller must check - * {@link Reservation#isPresent()} (or simply call {@link Reservation#tryGetOrInsertOrNull}, which - * returns {@code null} on an absent reservation) rather than assume every reservation is real. + * {@link Reservation#isReserved()} (or simply call {@link Reservation#tryGetOrInsertOrNull}, + * which returns {@code null} on an absent reservation) rather than assume every reservation is + * real. */ @Nonnull public static > Reservation tryReserve( @@ -982,10 +983,31 @@ private Reservation(@Nullable State state) { } /** {@code true} if this is a real, claimed reservation rather than an empty one. */ - public boolean isPresent() { + public boolean isReserved() { return state != null; } + /** + * Escape hatch for a caller that already built {@code newEntry} itself -- e.g. more than 4 key + * components, or components the caller wants to keep as primitives rather than boxing them into + * a {@code Function}'s type argument: + * + * 

    {@code
+     * try (Reservation r = ConcurrentHashtable.tryReserve(state)) {
+     *   if (!r.isReserved()) {
+     *     return null;
+     *   }
+     *   return r.tryGetOrInsertOrNull(new TEntry(longComponent1, longComponent2));
+     * }
+     * }
    + * + * See {@link #tryGetOrInsertOrNull(Object, Object, BiFunction)} for the general contract. + */ + @Nullable + public TEntry tryGetOrInsertOrNull(@Nonnull TEntry newEntry) { + return state == null ? null : finish(newEntry); + } + /** * One key component; see {@link #tryGetOrInsertOrNull(Object, Object, BiFunction)} for the * general contract. @@ -1043,6 +1065,12 @@ public TEntry tryGetOrInsertOrNull( return state == null ? null : finish(factory.apply(a, b, c, d)); } + /** {@link Maybe}-wrapping counterpart of {@link #tryGetOrInsertOrNull(Entry)}. */ + @Nonnull + public Maybe tryGetOrInsert(@Nonnull TEntry newEntry) { + return Maybe.of(tryGetOrInsertOrNull(newEntry)); + } + /** * {@link Maybe}-wrapping counterpart of {@link #tryGetOrInsertOrNull(Object, Function)}, for * callers who'd rather make the "this can fail unlike unbounded collections" outcome visible in diff --git a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableReservationTest.java b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableReservationTest.java index 7d0bb25d714..f61876b2e66 100644 --- a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableReservationTest.java +++ b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableReservationTest.java @@ -34,7 +34,7 @@ void tryGetOrInsertOrNullInsertsOnMissAndFindsOnHit() { TestEntry first; try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { - assertTrue(r.isPresent()); + assertTrue(r.isReserved()); first = r.tryGetOrInsertOrNull(1, TestEntry::new); } assertEquals(1, first.value); @@ -62,7 +62,7 @@ void reserveOnFullTableIsAbsentAndSkipsTheFactory() { AtomicInteger factoryCalls = new AtomicInteger(); TestEntry result; try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { - assertFalse(r.isPresent()); + assertFalse(r.isReserved()); result = r.tryGetOrInsertOrNull( 2, @@ -81,7 +81,7 @@ void closeCancelsAnUnconsumedReservation() { ConcurrentHashtable.State state = ConcurrentHashtable.createBounded(TestEntry.class, 1); try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { - assertTrue(r.isPresent()); + assertTrue(r.isReserved()); // Deliberately not consuming the reservation. } assertEquals(0, ConcurrentHashtable.estimateSize(state)); @@ -113,7 +113,7 @@ void closeOnAnAbsentReservationIsANoOp() { ConcurrentHashtable.State state = ConcurrentHashtable.createBounded(TestEntry.class, 0); try (ConcurrentHashtable.Reservation r = ConcurrentHashtable.tryReserve(state)) { - assertFalse(r.isPresent()); + assertFalse(r.isReserved()); } assertEquals(0, ConcurrentHashtable.estimateSize(state)); } From 995f7cb667c67edf39992260d423ae5fd3cd5db4 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Thu, 10 Sep 2026 22:53:55 -0400 Subject: [PATCH 8/9] Drop the self-type parameter from D1.Entry/D2.Entry D1.Entry and D2.Entry imposed the same self-bound generic pattern as the base ConcurrentHashtable.Entry, but D1/D2 only need it to implement matches() -- the concrete subclass never needs to flow through to callers of D1/D2's own generic surface. D1.Entry and D2.Entry now extend ConcurrentHashtable.Entry> (their own base type) directly, so subclasses are declared as e.g. `class MyEntry extends D1.Entry` rather than `D1.Entry`. D1/D2 store and link entries internally as that base Entry type and cast back to TEntry at their public API boundary -- sound because the table only ever holds instances its own caller-supplied creator produced. matches(Entry) is now final since subclasses only need to implement the key-based matches(Object) overload. Co-Authored-By: Claude Sonnet 5 --- .../util/ThreadSafeMapCounterBenchmark.java | 2 +- .../trace/util/ThreadSafeMapD1Benchmark.java | 2 +- .../trace/util/ThreadSafeMapD2Benchmark.java | 2 +- .../trace/util/ConcurrentHashtable.java | 192 ++++++++++++------ .../trace/util/ConcurrentHashtableD1Test.java | 5 +- .../trace/util/ConcurrentHashtableD2Test.java | 3 +- 6 files changed, 137 insertions(+), 69 deletions(-) diff --git a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapCounterBenchmark.java b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapCounterBenchmark.java index 9a52ac6f172..239dd3890f4 100644 --- a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapCounterBenchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapCounterBenchmark.java @@ -76,7 +76,7 @@ public class ThreadSafeMapCounterBenchmark { * Shared state ({@link Scope#Benchmark}): one instance of each map across all threads, modelling * a shared instrumentation counter table. */ - static final class CounterEntry extends ConcurrentHashtable.D1.Entry { + static final class CounterEntry extends ConcurrentHashtable.D1.Entry { static final AtomicLongFieldUpdater COUNT = AtomicLongFieldUpdater.newUpdater(CounterEntry.class, "count"); diff --git a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD1Benchmark.java b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD1Benchmark.java index 768108952de..6f5a8dcd769 100644 --- a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD1Benchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD1Benchmark.java @@ -87,7 +87,7 @@ public class ThreadSafeMapD1Benchmark { * Shared state ({@link Scope#Benchmark}): one instance of each map across all threads, modelling * a shared instrumentation cache. */ - static final class LongEntry extends ConcurrentHashtable.D1.Entry { + static final class LongEntry extends ConcurrentHashtable.D1.Entry { final long value; LongEntry(String key, long value) { diff --git a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD2Benchmark.java b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD2Benchmark.java index 118ecae54e8..f292ede835e 100644 --- a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD2Benchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD2Benchmark.java @@ -126,7 +126,7 @@ public boolean matches(SupportEntry other) { } /** Entry used with {@link ConcurrentHashtable.D2}. */ - static final class PairEntry extends ConcurrentHashtable.D2.Entry { + static final class PairEntry extends ConcurrentHashtable.D2.Entry { final long value; PairEntry(String key1, Integer key2, long value) { diff --git a/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java b/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java index 3d1cfa050d6..c205af5099f 100644 --- a/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java +++ b/internal-api/src/main/java/datadog/trace/util/ConcurrentHashtable.java @@ -117,21 +117,25 @@ public final TEntry next() { * Single-key concurrent hash table. Lock-free on hit; locked on miss/mutation. * * @param the key type - * @param the user's {@link D1.Entry D1.Entry<K, TEntry>} subclass + * @param the user's {@link D1.Entry D1.Entry<K>} subclass */ @ThreadSafe - public static final class D1> { + public static final class D1> { /** * Abstract base for {@link D1} entries. Subclass to add value fields you wish to mutate in * place after retrieving the entry via {@link D1#get}. * + *

    Deliberately parameterized on {@code K} alone, not self-bound on the concrete subclass: + * {@link D1} stores and links entries internally as {@code Entry} and casts back to {@code + * TEntry} at its API boundary, trading one unchecked cast (always sound -- the table only ever + * holds instances the caller's own {@code creator} produced) for a simpler subclass signature, + * e.g. {@code class MyEntry extends D1.Entry} rather than {@code D1.Entry}. + * * @param the key type - * @param the concrete subclass extending this one (self-bound, see {@link - * ConcurrentHashtable.Entry}) */ - public abstract static class Entry> - extends ConcurrentHashtable.Entry { + public abstract static class Entry extends ConcurrentHashtable.Entry> { @Nullable final K key; protected Entry(@Nullable K key) { @@ -153,7 +157,7 @@ public boolean matches(@Nullable Object key) { /** {@link ConcurrentHashtable.Entry#matches(Entry)} in terms of the key-based overload. */ @Override - public boolean matches(@Nonnull TEntry other) { + public final boolean matches(@Nonnull Entry other) { return matches(other.key); } @@ -167,9 +171,9 @@ public static long hash(@Nullable Object key) { } } - private final State state; + private final State> state; - private D1(State state) { + private D1(State> state) { this.state = state; } @@ -180,9 +184,38 @@ private D1(State state) { * The table does not resize. */ @Nonnull - public static > D1 createBounded( + @SuppressWarnings("unchecked") + public static > D1 createBounded( @Nonnull Class entryClass, int maxCapacity) { - return new D1<>(ConcurrentHashtable.createBounded(entryClass, maxCapacity)); + // entryClass is erased away (see ConcurrentHashtable#createFixedBuckets), so treating it as + // Class> instead of the caller's concrete Class is safe. + Class> baseEntryClass = (Class>) (Class) entryClass; + return new D1<>(ConcurrentHashtable.createBounded(baseEntryClass, maxCapacity)); + } + + /** + * Sound because every entry ever inserted into {@link #state} was produced as a {@code TEntry}. + */ + @SuppressWarnings("unchecked") + private TEntry cast(@Nullable Entry entry) { + return (TEntry) entry; + } + + /** See {@link #cast(Entry)}; casts the functional-interface reference, not each element. */ + @SuppressWarnings("unchecked") + private Predicate> castPredicate(Predicate predicate) { + return (Predicate>) predicate; + } + + @SuppressWarnings("unchecked") + private Consumer> castConsumer(Consumer consumer) { + return (Consumer>) consumer; + } + + @SuppressWarnings("unchecked") + private BiConsumer> castConsumer( + BiConsumer consumer) { + return (BiConsumer>) consumer; } public int size() { @@ -196,11 +229,11 @@ public boolean isFull() { @Nullable public TEntry get(@Nullable K key) { long keyHash = D1.Entry.hash(key); - for (TEntry curEntry = bucketFor(state, keyHash); + for (Entry curEntry = bucketFor(state, keyHash); curEntry != null; curEntry = curEntry.next()) { if (curEntry.keyHash == keyHash && curEntry.matches(key)) { - return curEntry; + return cast(curEntry); } } return null; @@ -229,17 +262,19 @@ public TEntry tryGetOrCreateOrNull( @Nullable K key, @Strategy @Nonnull Function creator) { long keyHash = D1.Entry.hash(key); int index = bucketIndex(state.buckets, keyHash); - for (TEntry curEntry = bucketAt(state, index); curEntry != null; curEntry = curEntry.next()) { + for (Entry curEntry = bucketAt(state, index); + curEntry != null; + curEntry = curEntry.next()) { if (curEntry.keyHash == keyHash && curEntry.matches(key)) { - return curEntry; + return cast(curEntry); } } synchronized (getTableWriteLock(state)) { - for (TEntry curEntry = bucketAt(state, index); + for (Entry curEntry = bucketAt(state, index); curEntry != null; curEntry = curEntry.next()) { if (curEntry.keyHash == keyHash && curEntry.matches(key)) { - return curEntry; + return cast(curEntry); } } // isFull() is checked before creating the entry, not before reserving a slot for it: @@ -285,21 +320,23 @@ public TEntry tryGetOrCreateOrEvictOrNull( @Strategy @Nonnull Predicate evictable) { long keyHash = D1.Entry.hash(key); int index = bucketIndex(state.buckets, keyHash); - for (TEntry curEntry = bucketAt(state, index); curEntry != null; curEntry = curEntry.next()) { + for (Entry curEntry = bucketAt(state, index); + curEntry != null; + curEntry = curEntry.next()) { if (curEntry.keyHash == keyHash && curEntry.matches(key)) { - return curEntry; + return cast(curEntry); } } synchronized (getTableWriteLock(state)) { - for (TEntry curEntry = bucketAt(state, index); + for (Entry curEntry = bucketAt(state, index); curEntry != null; curEntry = curEntry.next()) { if (curEntry.keyHash == keyHash && curEntry.matches(key)) { - return curEntry; + return cast(curEntry); } } if (state.sizeManager.isFull() - && state.sizeManager.evictOne(state.buckets, evictable) == null) { + && state.sizeManager.evictOne(state.buckets, castPredicate(evictable)) == null) { return null; } TEntry newEntry = creator.apply(key); @@ -319,14 +356,14 @@ public TEntry remove(@Nullable K key) { long keyHash = D1.Entry.hash(key); int index = bucketIndex(state.buckets, keyHash); synchronized (getTableWriteLock(state)) { - TEntry prev = null; - for (TEntry curEntry = bucketAt(state, index); + Entry prev = null; + for (Entry curEntry = bucketAt(state, index); curEntry != null; prev = curEntry, curEntry = curEntry.next()) { if (curEntry.keyHash == keyHash && curEntry.matches(key)) { unlink(state, index, prev, curEntry); state.sizeManager.decrement(); - return curEntry; + return cast(curEntry); } } return null; @@ -339,7 +376,7 @@ public TEntry remove(@Nullable K key) { * concurrent writers are excluded; lock-free readers continue throughout. */ public boolean removeIf(@Strategy @Nonnull Predicate predicate) { - return ConcurrentHashtable.removeIf(state, predicate); + return ConcurrentHashtable.removeIf(state, castPredicate(predicate)); } /** @@ -350,7 +387,7 @@ public boolean removeIf(@Strategy @Nonnull Predicate predicate) *

    Use {@link #drain(Object, BiConsumer)} to avoid a capturing lambda. */ public void drain(@Strategy @Nonnull Consumer sink) { - ConcurrentHashtable.drain(state, sink); + ConcurrentHashtable.drain(state, castConsumer(sink)); } /** @@ -360,7 +397,7 @@ public void drain(@Strategy @Nonnull Consumer sink) { */ public void drain( C context, @Strategy @Nonnull BiConsumer sink) { - ConcurrentHashtable.drain(state, context, sink); + ConcurrentHashtable.drain(state, context, castConsumer(sink)); } /** Removes all entries. Lock-free readers mid-walk complete against the entries they hold. */ @@ -369,7 +406,7 @@ public void clear() { } public void forEach(@Strategy @Nonnull Consumer consumer) { - ConcurrentHashtable.forEach(state, consumer); + ConcurrentHashtable.forEach(state, castConsumer(consumer)); } /** @@ -378,7 +415,7 @@ public void forEach(@Strategy @Nonnull Consumer consumer) { */ public void forEach( C context, @Strategy @Nonnull BiConsumer consumer) { - ConcurrentHashtable.forEach(state, context, consumer); + ConcurrentHashtable.forEach(state, context, castConsumer(consumer)); } } @@ -389,22 +426,22 @@ public void forEach( * * @param first key type * @param second key type - * @param the user's {@link D2.Entry D2.Entry<K1, K2, TEntry>} subclass + * @param the user's {@link D2.Entry D2.Entry<K1, K2>} subclass */ @ThreadSafe - public static final class D2> { + public static final class D2> { /** * Abstract base for {@link D2} entries. Subclass to add value fields you wish to mutate in * place after retrieving the entry via {@link D2#get}. * + *

    Deliberately parameterized on {@code K1}/{@code K2} alone, not self-bound on the concrete + * subclass -- see {@link D1.Entry} for why. + * * @param first key type * @param second key type - * @param the concrete subclass extending this one (self-bound, see {@link - * ConcurrentHashtable.Entry}) */ - public abstract static class Entry> - extends ConcurrentHashtable.Entry { + public abstract static class Entry extends ConcurrentHashtable.Entry> { @Nullable final K1 key1; @Nullable final K2 key2; @@ -434,7 +471,7 @@ public boolean matches(@Nullable K1 key1, @Nullable K2 key2) { /** {@link ConcurrentHashtable.Entry#matches(Entry)} in terms of the key-based overload. */ @Override - public boolean matches(@Nonnull TEntry other) { + public final boolean matches(@Nonnull Entry other) { return matches(other.key1, other.key2); } @@ -444,9 +481,9 @@ public static long hash(@Nullable Object key1, @Nullable Object key2) { } } - private final State state; + private final State> state; - private D2(State state) { + private D2(State> state) { this.state = state; } @@ -457,9 +494,38 @@ private D2(State state) { * The table does not resize. */ @Nonnull - public static > - D2 createBounded(@Nonnull Class entryClass, int maxCapacity) { - return new D2<>(ConcurrentHashtable.createBounded(entryClass, maxCapacity)); + @SuppressWarnings("unchecked") + public static > D2 createBounded( + @Nonnull Class entryClass, int maxCapacity) { + // entryClass is erased away (see ConcurrentHashtable#createFixedBuckets), so treating it as + // Class> instead of the caller's concrete Class is safe. + Class> baseEntryClass = (Class>) (Class) entryClass; + return new D2<>(ConcurrentHashtable.createBounded(baseEntryClass, maxCapacity)); + } + + /** + * Sound because every entry ever inserted into {@link #state} was produced as a {@code TEntry}. + */ + @SuppressWarnings("unchecked") + private TEntry cast(@Nullable Entry entry) { + return (TEntry) entry; + } + + /** See {@link #cast(Entry)}; casts the functional-interface reference, not each element. */ + @SuppressWarnings("unchecked") + private Predicate> castPredicate(Predicate predicate) { + return (Predicate>) predicate; + } + + @SuppressWarnings("unchecked") + private Consumer> castConsumer(Consumer consumer) { + return (Consumer>) consumer; + } + + @SuppressWarnings("unchecked") + private BiConsumer> castConsumer( + BiConsumer consumer) { + return (BiConsumer>) consumer; } public int size() { @@ -473,11 +539,11 @@ public boolean isFull() { @Nullable public TEntry get(@Nullable K1 key1, @Nullable K2 key2) { long keyHash = D2.Entry.hash(key1, key2); - for (TEntry curEntry = bucketFor(state, keyHash); + for (Entry curEntry = bucketFor(state, keyHash); curEntry != null; curEntry = curEntry.next()) { if (curEntry.keyHash == keyHash && curEntry.matches(key1, key2)) { - return curEntry; + return cast(curEntry); } } return null; @@ -510,17 +576,19 @@ public TEntry tryGetOrCreateOrNull( @Strategy @Nonnull BiFunction creator) { long keyHash = D2.Entry.hash(key1, key2); int index = bucketIndex(state.buckets, keyHash); - for (TEntry curEntry = bucketAt(state, index); curEntry != null; curEntry = curEntry.next()) { + for (Entry curEntry = bucketAt(state, index); + curEntry != null; + curEntry = curEntry.next()) { if (curEntry.keyHash == keyHash && curEntry.matches(key1, key2)) { - return curEntry; + return cast(curEntry); } } synchronized (getTableWriteLock(state)) { - for (TEntry curEntry = bucketAt(state, index); + for (Entry curEntry = bucketAt(state, index); curEntry != null; curEntry = curEntry.next()) { if (curEntry.keyHash == keyHash && curEntry.matches(key1, key2)) { - return curEntry; + return cast(curEntry); } } // isFull() is checked before creating the entry, not before reserving a slot for it: @@ -568,21 +636,23 @@ public TEntry tryGetOrCreateOrEvictOrNull( @Strategy @Nonnull Predicate evictable) { long keyHash = D2.Entry.hash(key1, key2); int index = bucketIndex(state.buckets, keyHash); - for (TEntry curEntry = bucketAt(state, index); curEntry != null; curEntry = curEntry.next()) { + for (Entry curEntry = bucketAt(state, index); + curEntry != null; + curEntry = curEntry.next()) { if (curEntry.keyHash == keyHash && curEntry.matches(key1, key2)) { - return curEntry; + return cast(curEntry); } } synchronized (getTableWriteLock(state)) { - for (TEntry curEntry = bucketAt(state, index); + for (Entry curEntry = bucketAt(state, index); curEntry != null; curEntry = curEntry.next()) { if (curEntry.keyHash == keyHash && curEntry.matches(key1, key2)) { - return curEntry; + return cast(curEntry); } } if (state.sizeManager.isFull() - && state.sizeManager.evictOne(state.buckets, evictable) == null) { + && state.sizeManager.evictOne(state.buckets, castPredicate(evictable)) == null) { return null; } TEntry newEntry = creator.apply(key1, key2); @@ -602,14 +672,14 @@ public TEntry remove(@Nullable K1 key1, @Nullable K2 key2) { long keyHash = D2.Entry.hash(key1, key2); int index = bucketIndex(state.buckets, keyHash); synchronized (getTableWriteLock(state)) { - TEntry prev = null; - for (TEntry curEntry = bucketAt(state, index); + Entry prev = null; + for (Entry curEntry = bucketAt(state, index); curEntry != null; prev = curEntry, curEntry = curEntry.next()) { if (curEntry.keyHash == keyHash && curEntry.matches(key1, key2)) { unlink(state, index, prev, curEntry); state.sizeManager.decrement(); - return curEntry; + return cast(curEntry); } } return null; @@ -622,7 +692,7 @@ public TEntry remove(@Nullable K1 key1, @Nullable K2 key2) { * concurrent writers are excluded; lock-free readers continue throughout. */ public boolean removeIf(@Strategy @Nonnull Predicate predicate) { - return ConcurrentHashtable.removeIf(state, predicate); + return ConcurrentHashtable.removeIf(state, castPredicate(predicate)); } /** @@ -633,7 +703,7 @@ public boolean removeIf(@Strategy @Nonnull Predicate predicate) *

    Use {@link #drain(Object, BiConsumer)} to avoid a capturing lambda. */ public void drain(@Strategy @Nonnull Consumer sink) { - ConcurrentHashtable.drain(state, sink); + ConcurrentHashtable.drain(state, castConsumer(sink)); } /** @@ -643,7 +713,7 @@ public void drain(@Strategy @Nonnull Consumer sink) { */ public void drain( C context, @Strategy @Nonnull BiConsumer sink) { - ConcurrentHashtable.drain(state, context, sink); + ConcurrentHashtable.drain(state, context, castConsumer(sink)); } /** Removes all entries. Lock-free readers mid-walk complete against the entries they hold. */ @@ -652,7 +722,7 @@ public void clear() { } public void forEach(@Strategy @Nonnull Consumer consumer) { - ConcurrentHashtable.forEach(state, consumer); + ConcurrentHashtable.forEach(state, castConsumer(consumer)); } /** @@ -661,7 +731,7 @@ public void forEach(@Strategy @Nonnull Consumer consumer) { */ public void forEach( C context, @Strategy @Nonnull BiConsumer consumer) { - ConcurrentHashtable.forEach(state, context, consumer); + ConcurrentHashtable.forEach(state, context, castConsumer(consumer)); } } diff --git a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD1Test.java b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD1Test.java index 499470743c8..46963236366 100644 --- a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD1Test.java +++ b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD1Test.java @@ -465,7 +465,7 @@ void tryGetOrCreateOrEvictOrNullEvictionRunsBeforeThrowingCreator() { } /** Entry holding a key plus one mutable {@code int} payload. */ - private static final class StringEntry extends ConcurrentHashtable.D1.Entry { + private static final class StringEntry extends ConcurrentHashtable.D1.Entry { volatile int value; StringEntry(String key, int value) { @@ -475,8 +475,7 @@ private static final class StringEntry extends ConcurrentHashtable.D1.Entry { + private static final class CollidingEntry extends ConcurrentHashtable.D1.Entry { CollidingEntry(CollidingKey key) { super(key); } diff --git a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD2Test.java b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD2Test.java index 38190b52a1d..e2b3f8dd353 100644 --- a/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD2Test.java +++ b/internal-api/src/test/java/datadog/trace/util/ConcurrentHashtableD2Test.java @@ -392,8 +392,7 @@ void tryGetOrCreateOrEvictOrNullEvictionRunsBeforeThrowingCreator() { } /** Entry with no payload beyond its two key parts, used to exercise the D2 identity/API. */ - private static final class PairEntry - extends ConcurrentHashtable.D2.Entry { + private static final class PairEntry extends ConcurrentHashtable.D2.Entry { PairEntry(String key1, Integer key2) { super(key1, key2); } From 8430830b21a441d355d39a411b2719a5461a056f Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Thu, 10 Sep 2026 23:11:21 -0400 Subject: [PATCH 9/9] Restore original D2Entry class in ThreadSafeMapD2Benchmark Now that D2.Entry no longer requires a self-type parameter, D2Entry doesn't need the extra value constructor param or lambda call sites that were introduced when self-bound generics were restored. Revert to the original D2Entry class and method references to keep the diff minimal. Co-Authored-By: Claude Sonnet 5 --- .../trace/util/ThreadSafeMapD2Benchmark.java | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD2Benchmark.java b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD2Benchmark.java index f292ede835e..57506a12230 100644 --- a/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD2Benchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/util/ThreadSafeMapD2Benchmark.java @@ -95,6 +95,15 @@ public class ThreadSafeMapD2Benchmark { } } + static final class D2Entry extends ConcurrentHashtable.D2.Entry { + final long value; + + D2Entry(String k1, Integer k2) { + super(k1, k2); + this.value = 1L; + } + } + /** * Entry used with the static helpers. Its primitive second key keeps storage and lookup unboxed, * independently of {@link Integer} caching or JVM escape analysis. @@ -125,16 +134,6 @@ public boolean matches(SupportEntry other) { } } - /** Entry used with {@link ConcurrentHashtable.D2}. */ - static final class PairEntry extends ConcurrentHashtable.D2.Entry { - final long value; - - PairEntry(String key1, Integer key2, long value) { - super(key1, key2); - this.value = value; - } - } - /** Composite key for map-based baselines. */ static final class Key2 implements Comparable { final String k1; @@ -177,7 +176,7 @@ public int compareTo(Key2 other) { */ @State(Scope.Benchmark) public static class SharedState { - ConcurrentHashtable.D2 table; + ConcurrentHashtable.D2 table; java.util.concurrent.atomic.AtomicReferenceArray supportBuckets; ConcurrentHashMap concurrentHashMap; ConcurrentSkipListMap skipListMap; @@ -185,14 +184,14 @@ public static class SharedState { @Setup(Level.Iteration) public void setUp() { - table = ConcurrentHashtable.D2.createBounded(PairEntry.class, CAPACITY); + table = ConcurrentHashtable.D2.createBounded(D2Entry.class, CAPACITY); supportBuckets = ConcurrentHashtable.createFixedBuckets(SupportEntry.class, CAPACITY); concurrentHashMap = new ConcurrentHashMap<>(CAPACITY); skipListMap = new ConcurrentSkipListMap<>(); synchronizedHashMap = Collections.synchronizedMap(new HashMap<>(CAPACITY)); for (int i = 0; i < N_KEYS; ++i) { int k2 = SOURCE_K2[i]; - table.tryGetOrCreateOrNull(SOURCE_K1[i], SOURCE_K2[i], (a, b) -> new PairEntry(a, b, 1L)); + table.tryGetOrCreateOrNull(SOURCE_K1[i], SOURCE_K2[i], D2Entry::new); // populate support table SupportEntry se = new SupportEntry(SOURCE_K1[i], k2); synchronized (ConcurrentHashtable.getWriteLock(supportBuckets, se.keyHash)) { @@ -219,7 +218,7 @@ int next() { } @Benchmark - public PairEntry get_concurrentHashtable(SharedState s, ThreadState t) { + public D2Entry get_concurrentHashtable(SharedState s, ThreadState t) { int i = t.next(); return s.table.get(SOURCE_K1[i], SOURCE_K2[i]); } @@ -259,10 +258,9 @@ public Long get_synchronizedHashMap(SharedState s, ThreadState t) { } @Benchmark - public PairEntry getOrCreate_concurrentHashtable(SharedState s, ThreadState t) { + public D2Entry getOrCreate_concurrentHashtable(SharedState s, ThreadState t) { int i = t.next(); - return s.table.tryGetOrCreateOrNull( - SOURCE_K1[i], SOURCE_K2[i], (k1, k2) -> new PairEntry(k1, k2, 0L)); + return s.table.tryGetOrCreateOrNull(SOURCE_K1[i], SOURCE_K2[i], D2Entry::new); } @Benchmark