Fix caching & memory infrastructure review findings - #185
Open
schenksj wants to merge 2 commits into
Open
Conversation
Addresses the correctness, efficiency, honesty, and dead-code findings from the caching/memory infrastructure review across the L2 disk cache, global cache, split cache manager, memory pool, and Java SplitCacheManager. Disk cache (L2): - Coalesced reads no longer silently zero-fill/truncate: failed sub-range reads are pushed into gaps (refetched from L3), overlaps are clipped to the cursor, and combine_segments guards result length. - Rewrite find_overlapping to correctly return nested/overlapping ranges (the old backward binary scan could drop an overlapping range). - Close the lost-wakeup race in size-based write backpressure: the writer decrements queued_bytes and notifies under the backpressure mutex. - Seed the LRU table from the persisted manifest at startup so cold splits from prior runs remain evictable; rewrite the LRU to O(1) HashMap. - Invalidate mmap'd files on eviction (remove_under_dir) and after replace. - Make Drop non-blocking: signal shutdown + non-blocking wake, no self-join (the background threads transiently upgrade the Weak, so Drop can run on them; joining there would deadlock / panic dropping the runtime). - compress_data returns Cow (zero-copy uncompressed path); manifest sync snapshots+compact-serializes outside the write lock; count queued bytes toward the eviction trigger. Compression: deprecate/document the no-op compression API (data is stored uncompressed by design for access-latency reasons). Split cache manager: - Enforce cache-name uniqueness (native registry is name-keyed) and make SplitCacheManager reference-counted + idempotent on close(), fixing the shared-singleton teardown and a resurrection race. - Implement force_eviction (real clearing) so flush* APIs work; fix CacheFlushStats.itemsEvicted. Get-or-create the L2 disk cache by root path. Wire Azure config into the manager. Warn on first-wins L1 sizing. Remove dead per-manager stat atomics. Global cache: - Flow GlobalCacheConfig knobs into SearcherConfig so the caches and the aggregation limits are actually sized (previously silent no-ops); remove the dead component cache instances. - Unify and bound the storage-resolver cache, clear it on last-manager close, and build a resolver from S3 + Azure together (mixed cloud). Include endpoint/path-style in the credential context key. Memory pool: target the deficit on JVM acquire; skip churny sub-threshold releases (still releasing the final grant when fully drained). Dead code: delete the unused byterange_cache module. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
try_acquire now sizes the JVM request by the deficit (new_used - granted), but the success check still required acquired >= size. When an earlier watermark-batched grant already backed part of current usage, deficit < size, so acquiring exactly the deficit (which makes granted == new_used) was wrongly rejected — denying e.g. a 128MB index_writer allocation even though the JVM accountant granted the full request. Validate against the deficit instead of size (deficit == 0 is the in-grant headroom top-up case and is always fine). Fixes NativeMemoryManagerTest.testCategoryBreakdownWithJvmPool. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
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
1 participant
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.
Summary
Addresses the findings from the caching/memory-infrastructure review across the L2 disk cache, global cache, split cache manager, memory pool, and Java
SplitCacheManager. Nativecargo checkis clean andmvn compilebuilds successfully.Correctness (high severity)
gapsand refetched from L3 instead of being silently reported as fully-cached (zero-filled/truncated); overlaps are clipped to the cursor;combine_segmentsguards result length.find_overlappingrewritten (§1.3) to correctly return nested/overlapping cached ranges.queued_bytesandnotify_allunder the backpressure mutex.SplitCacheManager.close()reference-counted + idempotent (§1.10), fixing shared-singleton teardown, a resurrection race, and thegetGlobalInstance()refcount bypass.GlobalCacheConfigknobs now take effect (§1.1): written intoSearcherConfigso caches and aggregation limits are actually sized; dead component caches removed.Honesty / API
force_evictionreally clears the in-memory query caches soflush*APIs work;CacheFlushStats.itemsEvictedfixed (§1.11).Other
Dropnon-blocking (no self-join deadlock); manifest sync outside the lock;compress_datausesCow; queued bytes count toward eviction (§2.1, §3.2, §3.3, §2.7).byterange_cachemodule (§4).Documented (with rationale) two known limitations where a full fix was higher-risk than warranted: the query path uses the global (not credential-scoped)
SearcherContext, and the per-search permit provider bypasses global concurrency budgeting (§1.9, §2.6).Review
Two adversarial review rounds. A self-join deadlock introduced by an initial
Drop-joins-threads change was caught and reverted; thegetGlobalInstance()refcount bypass was caught and fixed. Remaining areas verified correct.🤖 Generated with Claude Code