Repository navigation
Fix Redwood cache accounting when page sizes change - #14270
tclinkenbeard-oai merged 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
tclinkenbeard-oai
left a comment
There was a problem hiding this comment.
Generated by Codex.
What problem is this PR trying to solve? (ELI10)
Redwood keeps recently used database pages in memory and tracks their sizes to decide when to remove them.
When a page ID is reused for a differently sized page, that bookkeeping stays at the old size. For example, replacing a 4 KiB page with a 12 KiB page still counts only 4 KiB. This illustrative case hides 8 KiB from the cache budget; shrinking a page can cause the opposite error.
How does this PR solve the problem? (ELI10 and review walkthrough)
The cache updates its bookkeeping when an existing entry changes size. The example now counts all 12 KiB while keeping the same entry available to its caller.
- The pager supplies the replacement size.
DWALPager::updatePagepasses the page’s block count multiplied by its physical block size, without counting the replacement as a read hit. - The cache adjusts both counters together.
ObjectCache::getdetects the size change, andEvictor::resizeadds the signed difference to the total before updating the entry’s size. This also covers entries waiting on the prioritized-eviction list. - Eviction remains safe for the caller. Resizing does not remove entries, which could invalidate the reference being returned. The existing trim-on-miss path subsequently uses the corrected accounting. The regression test checks growth, shrinkage, entry identity, prioritized entries, eviction, and final cleanup.
What is it trying to do?
Correct Redwood’s cache-size accounting when a cached page ID is reused with a different block count, while preserving existing eviction policy and entry identity.
Is it correct?
Yes, based on code inspection of head 16d28c45f72d against base 7df606cc232a.
The signed 64-bit adjustment handles growth and shrinkage. Prioritized entries remain included in the shared total, and eviction and reclamation subtract the updated entry size. The change adds constant work, introduces no waits or allocations, and preserves existing assertions, read/write ordering, and ownership behavior. No persisted format or wire protocol changes.
I ran no builds or tests. Current-head clang-format passed; clang-tidy and Windows Boost CONFIG checks remain pending. Earlier build-success comments refer to the preceding commit.
Are there bugs?
I did not find any correctness bugs.
Are there omissions?
None that I think block this. The regression directly checks observable accounting and eviction behavior and would detect the original stale-size error.
Are there better ways of doing things?
No change needed. Keeping the total and entry-size updates together is appropriate. Trimming inside the replacement path would require additional protection for the returned entry.
Should this CL be LGTMd?
Yes, LGTM. I inspected the cache callers, accounting, prioritized eviction, cleanup, and regression test. The main remaining limitation is deliberate: growth can leave the cache over budget until a later miss triggers trimming. Runtime behavior was not independently validated in this review.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux RHEL 9
|
Result of foundationdb-pr-macos on macOS 14.x
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS 14.x
|
Reusing a cached physical page ID for a differently sized page left its original byte charge unchanged. For example, replacing a 4 KiB page with a 12 KiB page still counted only 4 KiB toward the cache budget.
Update the entry size and shared evictor total on size-changing cache hits, including entries awaiting prioritized eviction. Preserve entry identity and the existing trim-on-miss policy; trimming during replacement could invalidate the reference being returned.
Add a regression covering growth, shrinkage, prioritized entries, eviction using the corrected charge, and cleanup to zero.
Validation:
fdbserver_kvstore_test; all three/redwood/correctness/unit/tests pass in native and simulated modes with seed 1.git diff --checkpass.