Skip to content

Commit dd77994

Browse files
dougqhclaude
andcommitted
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) <noreply@anthropic.com>
1 parent 0015e8c commit dd77994

3 files changed

Lines changed: 62 additions & 9 deletions

File tree

communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ public GenerationalUtf8Cache(int capacity) {
120120
public GenerationalUtf8Cache(int edenCapacity, int tenuredCapacity) {
121121
this.accessTimeMs = System.currentTimeMillis();
122122

123-
int edenSize = Caching.cacheSizeFor(Math.min(tenuredCapacity, MAX_EDEN_CAPACITY));
123+
int edenSize = Caching.cacheSizeFor(Math.min(edenCapacity, MAX_EDEN_CAPACITY));
124124
this.edenEntries = new CacheEntry[edenSize];
125125
this.edenMarkers = new int[edenSize];
126126

@@ -143,7 +143,7 @@ public void updateAccessTime(long accessTimeMs) {
143143
}
144144

145145
/** Updates access time to the @link {@link System#currentTimeMillis()} */
146-
public void refreshAcessTime() {
146+
public void refreshAccessTime() {
147147
this.updateAccessTime(System.currentTimeMillis());
148148
}
149149

@@ -215,14 +215,13 @@ public final byte[] getUtf8(String value, long accessTimeMs) {
215215
if (value.length() > MAX_ENTRY_LEN) return CacheEntry.utf8(value);
216216

217217
int adjHash = Caching.adjHash(value);
218-
long lookupTimeMs = this.accessTimeMs;
219218

220219
CacheEntry[] tenuredEntries = this.tenuredEntries;
221220
int matchingTenuredIndex = lookupEntryIndex(tenuredEntries, MAX_TENURED_PROBES, adjHash, value);
222221
if (matchingTenuredIndex != -1) {
223222
CacheEntry tenuredEntry = tenuredEntries[matchingTenuredIndex];
224223

225-
tenuredEntry.hit(lookupTimeMs);
224+
tenuredEntry.hit(accessTimeMs);
226225

227226
this.tenuredHits += 1;
228227
return tenuredEntry.utf8();
@@ -233,7 +232,7 @@ public final byte[] getUtf8(String value, long accessTimeMs) {
233232
if (matchingEdenIndex != -1) {
234233
CacheEntry edenEntry = edenEntries[matchingEdenIndex];
235234

236-
double hits = edenEntry.hit(lookupTimeMs);
235+
double hits = edenEntry.hit(accessTimeMs);
237236
if (hits > this.promotionThreshold) {
238237
// mark promoted first - to avoid racy insertions
239238
this.promotions += 1;
@@ -256,8 +255,8 @@ public final byte[] getUtf8(String value, long accessTimeMs) {
256255

257256
CacheEntry newEntry = new CacheEntry(adjHash, value);
258257
// First request was swallowed by marking, so double hit
259-
newEntry.hit(lookupTimeMs);
260-
newEntry.hit(lookupTimeMs);
258+
newEntry.hit(accessTimeMs);
259+
newEntry.hit(accessTimeMs);
261260

262261
// search for empty slot or failing that the MFU entry
263262
int edenMfuIndex = findFirstAvailableOrMfuIndex(edenEntries, MAX_EDEN_PROBES, adjHash);
@@ -346,7 +345,7 @@ static final boolean lfuInsert(CacheEntry[] entries, int numProbes, CacheEntry n
346345
}
347346
}
348347

349-
// If we get here, then we're evicted the LRU
348+
// If we get here, then we're evicting the LFU
350349
entries[lfuIndex] = newEntry;
351350
return true;
352351
}

communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ static final boolean lfuInsert(CacheEntry[] entries, CacheEntry newEntry) {
160160
}
161161
}
162162

163-
// If we get here, then we're evicting the LRU
163+
// If we get here, then we're evicting the LFU
164164
entries[lfuIndex] = newEntry;
165165
return true;
166166
}

communication/src/test/java/datadog/communication/serialization/GenerationalUtf8CacheTest.java

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import static org.junit.jupiter.api.Assertions.assertNotSame;
77
import static org.junit.jupiter.api.Assertions.assertSame;
88

9+
import java.lang.reflect.Field;
910
import java.nio.charset.StandardCharsets;
1011
import java.util.Random;
1112
import java.util.concurrent.ThreadLocalRandom;
@@ -36,6 +37,13 @@ public void capacity() {
3637
assertEquals(128, cache.tenuredCapacity());
3738
}
3839

40+
@Test
41+
public void capacity_twoArg() {
42+
GenerationalUtf8Cache cache = new GenerationalUtf8Cache(64, 256);
43+
assertEquals(64, cache.edenCapacity());
44+
assertEquals(256, cache.tenuredCapacity());
45+
}
46+
3947
@Test
4048
public void maxCapacity() {
4149
GenerationalUtf8Cache cache =
@@ -82,6 +90,29 @@ public void caching() {
8290
assertNotEquals(0, cache.edenHits);
8391
}
8492

93+
@Test
94+
public void getUtf8_perCallAccessTime_overridesField() throws Exception {
95+
GenerationalUtf8Cache cache = create();
96+
// The field value should not leak into the entry when an explicit time is supplied.
97+
cache.updateAccessTime(0L);
98+
99+
String value = "bar";
100+
long callTime = 12345L;
101+
102+
// First call only marks; the second call creates the entry.
103+
cache.getUtf8(value, callTime);
104+
cache.getUtf8(value, callTime);
105+
106+
assertEquals(callTime, lookupEdenLastUsedMs(cache, value));
107+
108+
// Drive enough hits to promote into tenured.
109+
while (cache.promotions == 0) {
110+
cache.getUtf8(value, callTime);
111+
}
112+
113+
assertEquals(callTime, lookupTenuredLastUsedMs(cache, value));
114+
}
115+
85116
@Test
86117
public void promotion() {
87118
GenerationalUtf8Cache cache = create();
@@ -205,6 +236,29 @@ static final String nextValue() {
205236
return baseString + valueSuffix;
206237
}
207238

239+
static long lookupEdenLastUsedMs(GenerationalUtf8Cache cache, String value) throws Exception {
240+
return lookupLastUsedMs(cache, "edenEntries", value);
241+
}
242+
243+
static long lookupTenuredLastUsedMs(GenerationalUtf8Cache cache, String value) throws Exception {
244+
return lookupLastUsedMs(cache, "tenuredEntries", value);
245+
}
246+
247+
private static long lookupLastUsedMs(GenerationalUtf8Cache cache, String fieldName, String value)
248+
throws Exception {
249+
Field arrayField = GenerationalUtf8Cache.class.getDeclaredField(fieldName);
250+
arrayField.setAccessible(true);
251+
GenerationalUtf8Cache.CacheEntry[] entries =
252+
(GenerationalUtf8Cache.CacheEntry[]) arrayField.get(cache);
253+
254+
for (GenerationalUtf8Cache.CacheEntry entry : entries) {
255+
if (entry != null && value.equals(entry.value)) {
256+
return entry.lastUsedMs();
257+
}
258+
}
259+
throw new AssertionError("entry for value '" + value + "' not found in " + fieldName);
260+
}
261+
208262
static final void printStats(GenerationalUtf8Cache cache) {
209263
System.out.printf(
210264
"eden hits: %5d\tpromotion hits: %5d\tpromotions: %5d\tearly: %5d\tlocal evictions: %5d\tglobal evictions: %5d%n",

0 commit comments

Comments
 (0)