Reserve data file space in large steps - #74
Conversation
Every fresh page extended the file by eight KiB, so each one paid a metadata update. The file now grows in eight MiB steps and tracks how far content actually reaches, since the reserved tail must not be counted as pages after a reopen.
📝 WalkthroughWalkthrough
ChangesDataFile page tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
storage/src/data_file.rs (1)
202-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSynchronize before reopening in the durability test.
Without
data.sync(), the test only verifies that a same-process reopen observes cached writes; it does not exercise the persisted file state used by recovery.Proposed fix
assert!( data.file.metadata().expect("meta").len() > 2 * PAGE_SIZE as u64, "the file reserves room beyond the pages written" ); + data.sync().expect("sync");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@storage/src/data_file.rs` around lines 202 - 213, Update the durability test’s DataFile write block to call data.sync() after the page writes and before reopening or validating persisted state, ensuring the test exercises synchronized file contents rather than cached writes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@storage/src/data_file.rs`:
- Around line 75-78: Replace the saturating reservation arithmetic in the growth
branch of the data-file write logic with checked addition; when adding
GROW_BYTES to end overflows, return the existing CorruptDataFile error
consistently with the offset validation, and only call set_len and update
self.physical after a successful checked result.
---
Nitpick comments:
In `@storage/src/data_file.rs`:
- Around line 202-213: Update the durability test’s DataFile write block to call
data.sync() after the page writes and before reopening or validating persisted
state, ensuring the test exercises synchronized file contents rather than cached
writes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 53e93c14-b72f-4a90-98d9-2af54a86a633
📒 Files selected for processing (1)
storage/src/data_file.rs
| if end > self.physical { | ||
| let grown = end.saturating_add(GROW_BYTES); | ||
| self.file.set_len(grown)?; | ||
| self.physical = grown; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Return an error when reservation arithmetic overflows.
end is checked, but end.saturating_add(GROW_BYTES) can become u64::MAX and then pass an unrepresentable or enormous length to set_len. Use checked_add and return CorruptDataFile on overflow, consistent with the existing offset check.
Proposed fix
- let grown = end.saturating_add(GROW_BYTES);
+ let grown = end.checked_add(GROW_BYTES).ok_or_else(|| {
+ StorageError::CorruptDataFile("file growth overflow".to_owned())
+ })?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if end > self.physical { | |
| let grown = end.saturating_add(GROW_BYTES); | |
| self.file.set_len(grown)?; | |
| self.physical = grown; | |
| if end > self.physical { | |
| let grown = end.checked_add(GROW_BYTES).ok_or_else(|| { | |
| StorageError::CorruptDataFile("file growth overflow".to_owned()) | |
| })?; | |
| self.file.set_len(grown)?; | |
| self.physical = grown; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@storage/src/data_file.rs` around lines 75 - 78, Replace the saturating
reservation arithmetic in the growth branch of the data-file write logic with
checked addition; when adding GROW_BYTES to end overflows, return the existing
CorruptDataFile error consistently with the offset validation, and only call
set_len and update self.physical after a successful checked result.
Bench reportBaseline: main @ same runn. Cells read baseline → this PR. Verdicts need more than ±15% to leave the noise band. linux-arm64-full
linux-x64-1cpu-1gb
linux-x64-2cpu-2gb
linux-x64-full
windows-x64-full
Field referenceAll engines in identical containers, same driver, measured 2026-07-24 @ ba0ec39. A different harness than the tables above, so read this as standing, not as this PR's delta. linux-arm64-full
linux-x64-1cpu-1gb
linux-x64-2cpu-2gb
linux-x64-full
Shared CI runners: treat single-profile swings as suggestive, agreement across profiles as real. |
|
Measured and rejected.
So the eighteen milliseconds per group that motivated this is a Windows property, not a general one. NTFS extends a file by zeroing to the new valid data length, so reserving eight MiB costs an eight MiB write, which is why Windows gets materially worse rather than better. Linux extends a file cheaply and had nothing to gain. Two things worth keeping from the attempt. The probe confirmed we issue exactly 1.00 syncs per commit group, so the sync count was never the problem and can stop being suspected. And local timings on this Windows machine cannot be used to reason about the Linux runners at all: eighteen milliseconds per group here versus roughly one there, from file extension alone. |
1 participant
Found by counting rather than guessing. A probe confirmed we issue exactly 1.00 syncs per commit group, so the sync count was never the problem, but the same probe showed 18 ms per group under eight writers where a single threaded commit measures 3 ms. Fifteen milliseconds was going somewhere other than the flush.
Every fresh page extended the data file by one page, so each new page paid a file metadata update, and under load that is most of the group. The WAL has been preallocated for this reason since #40; the data file never was.
It now grows in eight MiB steps. The catch is that page_count drives both page id allocation and the recovery sweep, so the reserved tail must not be counted as pages: the file tracks how far content actually reaches and finds it at open by walking back over the zeroed tail. A test covers the reopen path.
Summary by CodeRabbit