Stop asking the file where it is, and write a run of pages in one call - #74
Closed
pathscale wants to merge 6 commits into
Closed
Stop asking the file where it is, and write a run of pages in one call#74pathscale wants to merge 6 commits into
pathscale wants to merge 6 commits into
Conversation
`eyre::Report` was in the return type of every fallible function here, so the
crate reached `std` through its own signatures. Nothing about page framing
needs an operating system: the layout is bytes and the checks are arithmetic.
This is the type that lets the rest of the crate say so, and it is the
prerequisite for the file access moving behind a trait - doing that first
would buy nothing while every signature still named a `std` type.
The enum is also more useful than formatted prose. A caller could not tell
"this page is full" from "these bytes are damaged", because both arrived as a
`Report` carrying a string. Six variants now carry the numbers that justify
them: LinkLengthMismatch, LinkOutOfBounds, PageOverflow, Corrupt, Encode, Io.
18 construction sites, 7 files, no .context or .wrap_err chains to unpick
Io holds raw_os_error rather than an io::Error, which is the std type this
change exists to stop depending on
Three tests asserted on eyre's message strings and now match on variants with
their fields, which is what the change is for.
cargo test 69 + 2 passed, 0 failed
clippy clean
**This breaks consumers**, so 0.6.0 rather than 0.5.8. Not every `?` converts:
a function returning `persist_page(..).await` in tail position needs `?` and
an `Ok(())`, and `error.wrap_err(..)` has to become
`eyre::Report::new(error).wrap_err(..)`. WorkTable needs exactly three such
edits, verified by building it against this branch; the patch is not applied
there because that checkout has another agent's uncommitted work in it.
Three changes to the write path, none of which change a byte of what lands on
disk. There are two new tests holding that.
**`persist_page` no longer calls `stream_position`.** It seeks to the page
start, writes, and then has to pad to the next page boundary, and to do that it
asked the operating system where the cursor had got to. The number was already
in hand: `persist_page_in_place` computes the inner length two lines earlier and
the header length is right there. It returns the total now and the padding is
arithmetic.
The call is not new and nobody introduced it recently: `git log -S` puts it in
`c33dfd1`, "finalise persist methods", November 2024. It was defensible when
both writes were inline in `persist_page` and a later refactor could have
changed what got written; `863e3b5` in June 2025 is that refactor, and after it
the lengths were genuinely hidden inside the helper. Returning them gives them
back.
**`persist_pages_batch` writes a run of consecutive pages in one call.** Every
page occupies exactly `PAGE_SIZE` at a known offset, so a run can be laid out in
memory and handed over once instead of seeking and writing per page. A run ends
wherever the page ids stop being consecutive, so a caller passing a
non-contiguous batch still gets a correct file, one write per run.
**`seek_to_page_start_relatively` is gone.** It asked the file where it was and
then seeked by the difference, arriving at exactly the offset
`seek_to_page_start` reaches in one call. Two system calls for the same place,
on both batch read paths as well as the write one.
Measured on the real async path, `examples/write-pages.rs`, 6,400 pages and
104.9 MB, interleaved over two passes:
before after
persist_page 235.5, 233.1 ms 180.5, 180.0 ms 1.30x
persist_pages_batch 201.4, 195.7 ms 49.4, 36.2 ms 4.1 to 5.4x
Old one-at-a-time against the new batch is about 5.4x.
The two new tests are the ones that matter. `a_batch_writes_what_one_at_a_time_writes`
compares the two files byte for byte, and caught the first version of this
padding the final page: writing one at a time only *seeks* past the last page,
and a seek past the end of a file does not extend it, so the file ends at the
last page's content rather than on a boundary. `a_batch_with_a_gap_puts_each_page_at_its_own_offset`
holds the run-breaking behaviour, including that the skipped pages stay zero.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three things the first version of this branch got wrong or left open, found by asking what it does where it actually runs: a Linux VM writing local pages that are then synced to Tigris. **A `usize` underflow.** `PAGE_SIZE - written` is fine while a header serialises to exactly `GENERAL_HEADER_SIZE`, and is a panic in debug and an absurd seek in release the moment one does not. The inner length is already guarded, so reaching it needs a header change, which is exactly the kind of change that arrives without anyone thinking about this function. It is `checked_sub` now and returns `PageOverflow`. The old code had the same hole with a different ending: it computed the padding in `i64` and would have seeked *backwards* over the page it had just written. **An unbounded buffer.** A run of ten thousand pages was a 160 MB allocation. On a VM whose memory is not ours to spend that is not a trade worth making for a few more megabytes per write, so a run flushes every 512 pages, which is 8 MiB. It costs nothing measurable: 47.7 ms against 49.4 and 36.2 before the bound. **And one behaviour difference that is real and now written down.** Writing page by page *seeks* over the space between a page's content and the next page's start, so on a filesystem with holes that space is never allocated. Writing a run in one call puts explicit zeroes there. The bytes read back are identical, which is what `a_batch_writes_what_one_at_a_time_writes` holds; what differs is blocks allocated, and on `ext4` a file of half-empty pages will now occupy what it claims to. Data pages are full and pad to nothing; index and space pages do not. `a_run_longer_than_the_buffer_bound_still_joins_up` is the guard on the bound: 520 pages, crossing a seam, byte for byte against writing them one at a time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The benchmark opened every file with `truncate(true)` and wrote ids 0..6400 in order, so both arms rewrote a whole file from empty. That is the batch path's best case: one uninterrupted consecutive run, which is exactly what the coalescing was written for and close to the least of what a database does. The new arm updates every tenth page of a file that already exists. The run breaks at every page, so the batch path falls back to one write per page and can only win by what it saves per page. It does: 1.28x, and the saving is the removed `stream_position` call rather than any joining up. Against 7.7x on the whole-file arm, and at 352 MB/s against 4019, a scattered page also costs more than a sequential one before either path helps it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 5.6x for `persist_pages_batch` is one call carrying a whole 104 MB file. Swept against the sizes a caller actually passes, it is 0.98x at one page, 1.08x at sixteen, and does not reach 2x until 256. Below about sixty-four pages both paths sit at 4-5 ms on both versions, which is `sync_all` and not the write path: there is nothing there for batching to save. So the change is worth what the callers make it worth, and today two of the three pass `HashMap::values()`, whose order is arbitrary, so their runs break at every page and they would see 1.0x at any size. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Run in a fixed order, whichever arm goes second inherits a file the first just wrote. That is what made an fsync'd write measure faster than an undurable one in another benchmark this morning, and this one had the same shape: the batch arm always ran second. Reversed, it holds - 3272 and 4167 MB/s batch-first against 2023 and 3961 batch-second - so the order was not flattering it. What the check did show is that the arm swings 2023 to 4167 MB/s across runs, so it is a 2 to 4 GB/s range and not the single figure it had been quoted as. The other arms are steady to a few percent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
Folded into #75, which now carries this branch's commits plus the page-stride work rebased on top, linear and with no merge commit. DataBucket carries one PR. |
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.
Three changes to the write path. None of them changes a byte of what lands on disk, and there are two new tests holding that.
What was there
persist_pageseeks to the page start, writes, and then pads to the next page boundary — and to work out the padding it calledstream_position(), a system call, for a number that was already in hand.persist_page_in_placecomputes the inner length two lines earlier and the header length is right beside it.persist_pages_batchseeked and wrote once per page.seek_to_page_start_relativelyasked the file where it was and seeked by the difference, arriving at exactly the offsetseek_to_page_startreaches in one call.Why it was like that
Nobody introduced this recently and it was never faster.
git log -S 'stream_position'puts it inc33dfd1, "finalise persist methods", November 2024 — the original implementation.It was defensible then: both writes were inline in
persist_page, and asking the file stays correct however the header or the helper later changes.863e3b5in June 2025 is that later change — it extracted the two writes intopersist_page_in_place— and after it the lengths genuinely were hidden. The call that was merely cautious became load-bearing. Returning the length gives them back.The changes
persist_page_in_placereturns how many bytes it wrote;persist_pagecomputes the padding instead of asking.persist_pages_batchlays out a run of consecutive pages in memory and writes it in one call. A run ends wherever the page ids stop being consecutive, so a non-contiguous batch still produces a correct file, one write per run.seek_to_page_start_relativelyis deleted; both batch read paths used it too and now use the one-syscall seek.Measured
On the real async path,
examples/write-pages.rs, 6,400 pages and 104.9 MB, interleaved over two passes:persist_page, one at a timepersist_pages_batchOld one-at-a-time against the new batch is about 5.4x.
The tests are the point
a_batch_writes_what_one_at_a_time_writescompares the two files byte for byte. It caught the first version of this change: I padded the final page, and writing one at a time only seeks past the last page — a seek past the end of a file does not extend it — so the file legitimately ends at the last page's content rather than on a boundary. Without that test the change would have silently altered every file's length.a_batch_with_a_gap_puts_each_page_at_its_own_offsetholds the run-breaking behaviour, including that skipped pages stay zero rather than being written over.The existing over-budget guard is shared by the new in-memory path, so a page too large for its slot is still refused on both — otherwise the batch path would have become a way around it.
What this does not touch
The page format, the read path, and
persist_page's signature.examples/write-pages.rsis new and is the benchmark the numbers above come from.🤖 Generated with Claude Code