Skip to content

Commit 9574a3c

Browse files
dougqhbric3devflow.devflow-routing-intake
authored
Fix bugs in GenerationalUtf8Cache constructor and access-time handling (#11323)
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> 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) <noreply@anthropic.com> Revert deprecated refreshAcessTime alias Keep only the corrected refreshAccessTime method. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Merge branch 'master' into dougqh/utf8-cache-fixes Merge branch 'master' into dougqh/utf8-cache-fixes Merge branch 'master' into dougqh/utf8-cache-fixes Merge branch 'master' into dougqh/utf8-cache-fixes Merge branch 'master' into dougqh/utf8-cache-fixes Merge branch 'master' into dougqh/utf8-cache-fixes Co-authored-by: bric3 <brice.dutheil@gmail.com> Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
1 parent 1abf511 commit 9574a3c

3 files changed

Lines changed: 58 additions & 11 deletions

File tree

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

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -85,10 +85,10 @@ public final class GenerationalUtf8Cache implements EncodingCache {
8585

8686
static final int MAX_ENTRY_LEN = 256;
8787

88-
private final CacheEntry[] edenEntries;
88+
final CacheEntry[] edenEntries;
8989
private final int[] edenMarkers;
9090

91-
private final CacheEntry[] tenuredEntries;
91+
final CacheEntry[] tenuredEntries;
9292

9393
private long accessTimeMs;
9494
private double promotionThreshold = INITIAL_PROMOTION_THRESHOLD;
@@ -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: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,13 @@ public void capacity() {
3636
assertEquals(128, cache.tenuredCapacity());
3737
}
3838

39+
@Test
40+
public void capacity_twoArg() {
41+
GenerationalUtf8Cache cache = new GenerationalUtf8Cache(64, 256);
42+
assertEquals(64, cache.edenCapacity());
43+
assertEquals(256, cache.tenuredCapacity());
44+
}
45+
3946
@Test
4047
public void maxCapacity() {
4148
GenerationalUtf8Cache cache =
@@ -82,6 +89,29 @@ public void caching() {
8289
assertNotEquals(0, cache.edenHits);
8390
}
8491

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

238+
static long lookupEdenLastUsedMs(GenerationalUtf8Cache cache, String value) {
239+
return lookupLastUsedMs(cache.edenEntries, "edenEntries", value);
240+
}
241+
242+
static long lookupTenuredLastUsedMs(GenerationalUtf8Cache cache, String value) {
243+
return lookupLastUsedMs(cache.tenuredEntries, "tenuredEntries", value);
244+
}
245+
246+
private static long lookupLastUsedMs(
247+
GenerationalUtf8Cache.CacheEntry[] entries, String arrayName, String value) {
248+
for (GenerationalUtf8Cache.CacheEntry entry : entries) {
249+
if (entry != null && value.equals(entry.value)) {
250+
return entry.lastUsedMs();
251+
}
252+
}
253+
throw new AssertionError("entry for value '" + value + "' not found in " + arrayName);
254+
}
255+
208256
static final void printStats(GenerationalUtf8Cache cache) {
209257
System.out.printf(
210258
"eden hits: %5d\tpromotion hits: %5d\tpromotions: %5d\tearly: %5d\tlocal evictions: %5d\tglobal evictions: %5d%n",

0 commit comments

Comments
 (0)