Skip to content

Fix Redwood cache accounting when page sizes change - #14270

Merged
tclinkenbeard-oai merged 3 commits into
apple:mainfrom
tclinkenbeard-oai:dev/tclinkenbeard/redwood-cache-size-accounting
Oct 13, 2026
Merged

tclinkenbeard-oai merged 3 commits into
apple:mainfrom
tclinkenbeard-oai:dev/tclinkenbeard/redwood-cache-size-accounting

Conversation

@tclinkenbeard-oai

Copy link
Copy Markdown
Collaborator

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:

  • The new regression fails without the fix: 4,096 bytes accounted versus a 12,288-byte page.
  • Built fdbserver_kvstore_test; all three /redwood/correctness/unit/ tests pass in native and simulated modes with seed 1.
  • Changed-line clang-format and git diff --check pass.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@tclinkenbeard-oai
tclinkenbeard-oai marked this pull request as ready for review October 12, 2026 22:22

@tclinkenbeard-oai tclinkenbeard-oai left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. The pager supplies the replacement size. DWALPager::updatePage passes the page’s block count multiplied by its physical block size, without counting the replacement as a read hit.
  2. The cache adjusts both counters together. ObjectCache::get detects the size change, and Evictor::resize adds the signed difference to the total before updating the entry’s size. This also covers entries waiting on the prioritized-eviction list.
  3. 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.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang-ide on Linux RHEL 9

  • Commit ID: cb8d07a
  • Duration 0:49:03
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang-arm on Linux RHEL 9

  • Commit ID: cb8d07a
  • Duration 0:49:21
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos on macOS 14.x

  • Commit ID: cb8d07a
  • Duration 1:13:28
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang on Linux RHEL 9

  • Commit ID: cb8d07a
  • Duration 1:21:17
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr on Linux RHEL 9

  • Commit ID: cb8d07a
  • Duration 1:23:27
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

  • Commit ID: cb8d07a
  • Duration 2:09:37
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos-m1 on macOS 14.x

  • Commit ID: cb8d07a
  • Duration 3:40:26
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@tclinkenbeard-oai
tclinkenbeard-oai merged commit 7b8530e into apple:main Oct 13, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants