From dd779943f885c7758041d8b86df7a6c07b7c8e5e Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 8 May 2026 09:41:15 -0400 Subject: [PATCH 1/3] Fix bugs in GenerationalUtf8Cache constructor and access-time handling - getUtf8(value, accessTimeMs) was capturing this.accessTimeMs and using that for entry timestamps, silently ignoring the parameter contrary to its Javadoc. - The two-arg constructor sized the eden array using tenuredCapacity instead of edenCapacity. - Rename refreshAcessTime to refreshAccessTime (no other callers). - Correct misleading "evicting the LRU" comments in the LFU paths of both SimpleUtf8Cache and GenerationalUtf8Cache. Adds regression tests for both bugs. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../serialization/GenerationalUtf8Cache.java | 15 +++--- .../serialization/SimpleUtf8Cache.java | 2 +- .../GenerationalUtf8CacheTest.java | 54 +++++++++++++++++++ 3 files changed, 62 insertions(+), 9 deletions(-) diff --git a/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java b/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java index 036abfe6e79..4c48643e6d0 100644 --- a/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java +++ b/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java @@ -120,7 +120,7 @@ public GenerationalUtf8Cache(int capacity) { public GenerationalUtf8Cache(int edenCapacity, int tenuredCapacity) { this.accessTimeMs = System.currentTimeMillis(); - int edenSize = Caching.cacheSizeFor(Math.min(tenuredCapacity, MAX_EDEN_CAPACITY)); + int edenSize = Caching.cacheSizeFor(Math.min(edenCapacity, MAX_EDEN_CAPACITY)); this.edenEntries = new CacheEntry[edenSize]; this.edenMarkers = new int[edenSize]; @@ -143,7 +143,7 @@ public void updateAccessTime(long accessTimeMs) { } /** Updates access time to the @link {@link System#currentTimeMillis()} */ - public void refreshAcessTime() { + public void refreshAccessTime() { this.updateAccessTime(System.currentTimeMillis()); } @@ -215,14 +215,13 @@ public final byte[] getUtf8(String value, long accessTimeMs) { if (value.length() > MAX_ENTRY_LEN) return CacheEntry.utf8(value); int adjHash = Caching.adjHash(value); - long lookupTimeMs = this.accessTimeMs; CacheEntry[] tenuredEntries = this.tenuredEntries; int matchingTenuredIndex = lookupEntryIndex(tenuredEntries, MAX_TENURED_PROBES, adjHash, value); if (matchingTenuredIndex != -1) { CacheEntry tenuredEntry = tenuredEntries[matchingTenuredIndex]; - tenuredEntry.hit(lookupTimeMs); + tenuredEntry.hit(accessTimeMs); this.tenuredHits += 1; return tenuredEntry.utf8(); @@ -233,7 +232,7 @@ public final byte[] getUtf8(String value, long accessTimeMs) { if (matchingEdenIndex != -1) { CacheEntry edenEntry = edenEntries[matchingEdenIndex]; - double hits = edenEntry.hit(lookupTimeMs); + double hits = edenEntry.hit(accessTimeMs); if (hits > this.promotionThreshold) { // mark promoted first - to avoid racy insertions this.promotions += 1; @@ -256,8 +255,8 @@ public final byte[] getUtf8(String value, long accessTimeMs) { CacheEntry newEntry = new CacheEntry(adjHash, value); // First request was swallowed by marking, so double hit - newEntry.hit(lookupTimeMs); - newEntry.hit(lookupTimeMs); + newEntry.hit(accessTimeMs); + newEntry.hit(accessTimeMs); // search for empty slot or failing that the MFU entry int edenMfuIndex = findFirstAvailableOrMfuIndex(edenEntries, MAX_EDEN_PROBES, adjHash); @@ -346,7 +345,7 @@ static final boolean lfuInsert(CacheEntry[] entries, int numProbes, CacheEntry n } } - // If we get here, then we're evicted the LRU + // If we get here, then we're evicting the LFU entries[lfuIndex] = newEntry; return true; } diff --git a/communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java b/communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java index bb2dcf11f5d..93e6c92bb9b 100644 --- a/communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java +++ b/communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java @@ -160,7 +160,7 @@ static final boolean lfuInsert(CacheEntry[] entries, CacheEntry newEntry) { } } - // If we get here, then we're evicting the LRU + // If we get here, then we're evicting the LFU entries[lfuIndex] = newEntry; return true; } diff --git a/communication/src/test/java/datadog/communication/serialization/GenerationalUtf8CacheTest.java b/communication/src/test/java/datadog/communication/serialization/GenerationalUtf8CacheTest.java index b8fb3fde316..53a90ec0327 100644 --- a/communication/src/test/java/datadog/communication/serialization/GenerationalUtf8CacheTest.java +++ b/communication/src/test/java/datadog/communication/serialization/GenerationalUtf8CacheTest.java @@ -6,6 +6,7 @@ import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertSame; +import java.lang.reflect.Field; import java.nio.charset.StandardCharsets; import java.util.Random; import java.util.concurrent.ThreadLocalRandom; @@ -36,6 +37,13 @@ public void capacity() { assertEquals(128, cache.tenuredCapacity()); } + @Test + public void capacity_twoArg() { + GenerationalUtf8Cache cache = new GenerationalUtf8Cache(64, 256); + assertEquals(64, cache.edenCapacity()); + assertEquals(256, cache.tenuredCapacity()); + } + @Test public void maxCapacity() { GenerationalUtf8Cache cache = @@ -82,6 +90,29 @@ public void caching() { assertNotEquals(0, cache.edenHits); } + @Test + public void getUtf8_perCallAccessTime_overridesField() throws Exception { + GenerationalUtf8Cache cache = create(); + // The field value should not leak into the entry when an explicit time is supplied. + cache.updateAccessTime(0L); + + String value = "bar"; + long callTime = 12345L; + + // First call only marks; the second call creates the entry. + cache.getUtf8(value, callTime); + cache.getUtf8(value, callTime); + + assertEquals(callTime, lookupEdenLastUsedMs(cache, value)); + + // Drive enough hits to promote into tenured. + while (cache.promotions == 0) { + cache.getUtf8(value, callTime); + } + + assertEquals(callTime, lookupTenuredLastUsedMs(cache, value)); + } + @Test public void promotion() { GenerationalUtf8Cache cache = create(); @@ -205,6 +236,29 @@ static final String nextValue() { return baseString + valueSuffix; } + static long lookupEdenLastUsedMs(GenerationalUtf8Cache cache, String value) throws Exception { + return lookupLastUsedMs(cache, "edenEntries", value); + } + + static long lookupTenuredLastUsedMs(GenerationalUtf8Cache cache, String value) throws Exception { + return lookupLastUsedMs(cache, "tenuredEntries", value); + } + + private static long lookupLastUsedMs(GenerationalUtf8Cache cache, String fieldName, String value) + throws Exception { + Field arrayField = GenerationalUtf8Cache.class.getDeclaredField(fieldName); + arrayField.setAccessible(true); + GenerationalUtf8Cache.CacheEntry[] entries = + (GenerationalUtf8Cache.CacheEntry[]) arrayField.get(cache); + + for (GenerationalUtf8Cache.CacheEntry entry : entries) { + if (entry != null && value.equals(entry.value)) { + return entry.lastUsedMs(); + } + } + throw new AssertionError("entry for value '" + value + "' not found in " + fieldName); + } + static final void printStats(GenerationalUtf8Cache cache) { System.out.printf( "eden hits: %5d\tpromotion hits: %5d\tpromotions: %5d\tearly: %5d\tlocal evictions: %5d\tglobal evictions: %5d%n", From 778953ce5b51153849e5314e03c0e58c5c880594 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 8 May 2026 09:54:59 -0400 Subject: [PATCH 2/3] Address review feedback - Restore refreshAcessTime as a deprecated forwarding alias to preserve binary compatibility for any existing callers compiled against the prior artifact. - Drop reflection in GenerationalUtf8CacheTest by making the eden and tenured entry arrays package-visible. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../serialization/GenerationalUtf8Cache.java | 10 +++++++-- .../GenerationalUtf8CacheTest.java | 22 +++++++------------ 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java b/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java index 4c48643e6d0..d100d5bd4e1 100644 --- a/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java +++ b/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java @@ -85,10 +85,10 @@ public final class GenerationalUtf8Cache implements EncodingCache { static final int MAX_ENTRY_LEN = 256; - private final CacheEntry[] edenEntries; + final CacheEntry[] edenEntries; private final int[] edenMarkers; - private final CacheEntry[] tenuredEntries; + final CacheEntry[] tenuredEntries; private long accessTimeMs; private double promotionThreshold = INITIAL_PROMOTION_THRESHOLD; @@ -147,6 +147,12 @@ public void refreshAccessTime() { this.updateAccessTime(System.currentTimeMillis()); } + /** Deprecated in favor of {@link #refreshAccessTime()} - retained for binary compatibility. */ + @Deprecated + public void refreshAcessTime() { + this.refreshAccessTime(); + } + public synchronized void recalibrate() { this.recalibrate(System.currentTimeMillis()); } diff --git a/communication/src/test/java/datadog/communication/serialization/GenerationalUtf8CacheTest.java b/communication/src/test/java/datadog/communication/serialization/GenerationalUtf8CacheTest.java index 53a90ec0327..86876b3d29f 100644 --- a/communication/src/test/java/datadog/communication/serialization/GenerationalUtf8CacheTest.java +++ b/communication/src/test/java/datadog/communication/serialization/GenerationalUtf8CacheTest.java @@ -6,7 +6,6 @@ import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertSame; -import java.lang.reflect.Field; import java.nio.charset.StandardCharsets; import java.util.Random; import java.util.concurrent.ThreadLocalRandom; @@ -91,7 +90,7 @@ public void caching() { } @Test - public void getUtf8_perCallAccessTime_overridesField() throws Exception { + public void getUtf8_perCallAccessTime_overridesField() { GenerationalUtf8Cache cache = create(); // The field value should not leak into the entry when an explicit time is supplied. cache.updateAccessTime(0L); @@ -236,27 +235,22 @@ static final String nextValue() { return baseString + valueSuffix; } - static long lookupEdenLastUsedMs(GenerationalUtf8Cache cache, String value) throws Exception { - return lookupLastUsedMs(cache, "edenEntries", value); + static long lookupEdenLastUsedMs(GenerationalUtf8Cache cache, String value) { + return lookupLastUsedMs(cache.edenEntries, "edenEntries", value); } - static long lookupTenuredLastUsedMs(GenerationalUtf8Cache cache, String value) throws Exception { - return lookupLastUsedMs(cache, "tenuredEntries", value); + static long lookupTenuredLastUsedMs(GenerationalUtf8Cache cache, String value) { + return lookupLastUsedMs(cache.tenuredEntries, "tenuredEntries", value); } - private static long lookupLastUsedMs(GenerationalUtf8Cache cache, String fieldName, String value) - throws Exception { - Field arrayField = GenerationalUtf8Cache.class.getDeclaredField(fieldName); - arrayField.setAccessible(true); - GenerationalUtf8Cache.CacheEntry[] entries = - (GenerationalUtf8Cache.CacheEntry[]) arrayField.get(cache); - + private static long lookupLastUsedMs( + GenerationalUtf8Cache.CacheEntry[] entries, String arrayName, String value) { for (GenerationalUtf8Cache.CacheEntry entry : entries) { if (entry != null && value.equals(entry.value)) { return entry.lastUsedMs(); } } - throw new AssertionError("entry for value '" + value + "' not found in " + fieldName); + throw new AssertionError("entry for value '" + value + "' not found in " + arrayName); } static final void printStats(GenerationalUtf8Cache cache) { From adee794d19fb203c7605d9d53b1fd61201382c0e Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 8 May 2026 10:00:05 -0400 Subject: [PATCH 3/3] Revert deprecated refreshAcessTime alias Keep only the corrected refreshAccessTime method. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../communication/serialization/GenerationalUtf8Cache.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java b/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java index d100d5bd4e1..33b775b197d 100644 --- a/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java +++ b/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java @@ -147,12 +147,6 @@ public void refreshAccessTime() { this.updateAccessTime(System.currentTimeMillis()); } - /** Deprecated in favor of {@link #refreshAccessTime()} - retained for binary compatibility. */ - @Deprecated - public void refreshAcessTime() { - this.refreshAccessTime(); - } - public synchronized void recalibrate() { this.recalibrate(System.currentTimeMillis()); }