[common] Bound the file-size memo in both local cache managers - #9775
Open
LuciferYang wants to merge 6 commits into
Open
[common] Bound the file-size memo in both local cache managers#9775LuciferYang wants to merge 6 commits into
LuciferYang wants to merge 6 commits into
Conversation
LocalMemoryCacheManager memoized file sizes in an unbounded ConcurrentHashMap with only prefix invalidation removing entries: in memory-cache mode, a job reading many distinct whitelisted files grew it without limit (a long map of path strings on the heap). Cap the memo at 65536 entries with insertion-order eviction, guarded by the manager's existing lock (an access-ordered LinkedHashMap also keeps repeated hits on the same entries). A dropped memo only costs one extra getFileStatus on the next open. Assisted-by: GLM-5.3
LuciferYang
marked this pull request as draft
September 13, 2026 03:07
The memo lives in both cache managers and which one runs is decided purely by whether local-cache.dir is set, so bounding only the in-memory one left the same unbounded map on the path the javadoc was describing: a long-lived process reading millions of distinct files. Extract the bounded LRU memo and use it in both, with the disk manager's accesses under the lock it already holds. Co-Authored-By: Claude Code <noreply@anthropic.com>
The assertion only checked that the oldest entry was gone and the newest was present, which holds for any bound at all. Count the survivors against MAX_ENTRIES so moving the bound fails the test. Co-Authored-By: Claude Code <noreply@anthropic.com>
…s prefix scope Reads the bound through a method so it is not inlined into the test, adds a unit test on FileSizeMemo itself, and states the bound each case needs rather than assuming an even one.
LuciferYang
marked this pull request as ready for review
September 13, 2026 18:24
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
close #9774
In memory-cache mode
CachingFileIO.newInputStreamconstructs the stream withfileSize = -1, so every whitelisted file it opens lazily memoizes its size throughLocalCacheManager.getFileSize/putFileSize. The block cache is bounded by bytes; that memo was a plainConcurrentHashMapwith no bound at all, and only prefixinvalidateever removed anything from it. A long-lived process reading many distinct files grew the heap without limit, one small entry per path.The memo is now a bounded access-ordered LRU, capped by entry count. Dropping an entry costs one extra
getFileStatus, and the files these caches accept are immutable, so the re-stat returns the same size and any cached blocks stay valid.Two things this required beyond the bound itself:
The map is no longer thread-safe, and access order means
getmutates it, so every access has to be inside the manager's existing lock. That includesinvalidate'sremoveIf, which previously ran outside it and was only safe because the map was aConcurrentHashMap.The same memo exists in
LocalDiskCacheManager, and which manager runs is decided purely by whetherlocal-cache.diris set, so the bound is now shared by both through aFileSizeMemoclass rather than living in one of them. To be precise about that half: throughCachingFileIOthe disk branch stats the file and passes a known length, sofileSize()returns before consulting the cache and the disk memo is never populated today. Bounding it is defensive, and it removes the duplication that let the two implementations of one interface disagree about boundedness in the first place.Tests
FileSizeMemoTestunit-tests the memo itself.putsAloneBoundTheMemofills to the bound, asserts the size, puts one more and asserts again before putting a thousand more: checking the first overflow on its own is what catches an eviction step that removes the wrong number of entries, which a single assertion after an even number of overflowing puts does not.aReadEntryOutlivesAnUnreadOnereads the older entries and then puts exactly as many new ones as were left unread, so eviction has to take the unread ones; that is what pins the access-order flag.puttingAnEntryAgainRefreshesItpins that a re-put carries both the newer value and the newer position.invalidateRemovesOnlyTheMatchingPrefixincludes a key that carries the prefix but not at the front, without which acontainsimplementation passes.The bound is read through a package-private
maxEntries()rather than the constant, because astatic final intis inlined into the test's bytecode: an incremental build that recompiles only the main class then leaves the test asserting the old number, which produced a real spurious failure. Each test also states the smallest bound it needs, so lowering the constant reports that rather than looking like an eviction bug.CachingFileIOTest.fileSizeMemoIsBoundedstays as the wiring check that both managers route through a bounded memo.Verified on JDK 11 by mutation, not by argument. Caught: removing the eviction loop (
expected: 65536), moving it fromputintoget,accessOrder = false(expected: 0L but was: -1L), evicting two entries per round,putskipping a key already present, andinvalidatematching bycontains(expected: 4L but was: -1L). The suite also passes at bounds 4, 5 and 65537, so it does not assume an even bound.Concurrency was exercised directly rather than argued: 8 threads for 3s mixing
putFileSize,getFileSize,putBlock,getBlockandinvalidateagainst a size-capped disk manager with eviction active, plus 64 concurrent streams doing lazy size initialisation. Lock order is always stream lock then manager lock, the manager never calls back into the stream, and the remotegetFileStatushappens between the two memo calls rather than inside the manager's critical section.paimon-commonbuilds clean with checkstyle and spotless enabled.