Fix bugs in GenerationalUtf8Cache constructor and access-time handling#11323
Fix bugs in GenerationalUtf8Cache constructor and access-time handling#11323
Conversation
- 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>
dougqh
added
comp: core
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd779943f8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| /** Updates access time to the @link {@link System#currentTimeMillis()} */ | ||
| public void refreshAcessTime() { | ||
| public void refreshAccessTime() { |
There was a problem hiding this comment.
Renaming this public method removes the refreshAcessTime() symbol from the published GenerationalUtf8Cache class. Any external or out-of-tree code compiled against the previous communication artifact that calls the misspelled method will now fail with NoSuchMethodError at runtime (or fail source compilation) even though the implementation could remain as a deprecated forwarding alias to refreshAccessTime().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
My choice - as described by Claude...
Decided not to keep the deprecated alias.
GenerationalUtf8Cachelives indatadog.communication.serializationand is internal to the agent — not a documented public API.grepacross the repo confirms there are no callers outside this class itself, and the package isn't shipped as a stable extension point. Carrying a misspelled forwarding method just to hedge against hypothetical out-of-tree compilation isn't worth the API surface debt.
| return baseString + valueSuffix; | ||
| } | ||
|
|
||
| static long lookupEdenLastUsedMs(GenerationalUtf8Cache cache, String value) throws Exception { |
There was a problem hiding this comment.
I'd prefer to make the fields package visible rather than use reflection in the tests.
- 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>
Keep only the corrected refreshAccessTime method. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
What Does This Do
Fixes three bugs and a misleading comment in the UTF-8 caches.
None of these issues impacted how the cache is currently used, but could pose a problem in the future.
GenerationalUtf8Cache.getUtf8(String, long accessTimeMs)capturedthis.accessTimeMsinto a local and used that for entry timestamps, silently ignoring the supplied parameter — directly contradicting its Javadoc.GenerationalUtf8Cache(int edenCapacity, int tenuredCapacity)sized the eden array usingtenuredCapacityinstead ofedenCapacity. The existing test only exercised the single-arg constructor, so this was unobserved.refreshAcessTimerenamed torefreshAccessTime(typo). No other callers exist in the repo.lfuInsert(bothSimpleUtf8CacheandGenerationalUtf8Cache) was misleading — those paths evict the LFU.Motivation
Clean-up
Test plan
./gradlew :communication:test— passes, including the new regression testsGenerationalUtf8CacheTest.getUtf8_perCallAccessTime_overridesField— fails on master, passes after the fix.GenerationalUtf8CacheTest.capacity_twoArg— fails on master, passes after the fix../gradlew :communication:spotlessCheck🤖 Generated with Claude Code