Direct-IO fast paths + page-cache correctness and robustness fixes#138
Open
OBrezhniev wants to merge 13 commits into
Open
Direct-IO fast paths + page-cache correctness and robustness fixes#138OBrezhniev wants to merge 13 commits into
OBrezhniev wants to merge 13 commits into
Conversation
readToBuffer loaded every page into a cache buffer and then copied page->destination, so a large sequential read (e.g. a 100-350MB zkey/ptau section) cost a full extra copy and held the whole section in page buffers (maxPagesLoaded is bumped to fit the read). For reads >= directReadThreshold (1MB) with no overlapping dirty pages, read straight from the fd into the destination buffer instead. Small reads (section headers, ULE32 navigation) keep using the cache. _rangeHasDirtyPages() guards correctness on read-write files; positioned fd.read (pread) keeps concurrent section reads safe. On a sha256 groth16 prove this cut peak memory ~4.7GB -> ~3.9GB by dropping the duplicate page buffers, and removed the page->destination memcpy that was ~13% of main-thread self-time in the flame profile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ache Mirrors the direct-read path. The cached write() loads pages, copies buff->page, marks them dirty, and flushes page->disk later, so a large write (e.g. a zkey/ptau section) costs an extra copy and holds the whole region in page buffers. For writes >= directWriteThreshold (1MB) to a region with no cached pages, write straight to disk via positioned fd.write instead. The guard is stricter than the read path's: it falls back to the cache when ANY page in range is cached (even clean), since a clean cached page would go stale after a direct write. Small writes (headers, ULE32) keep using the cache. The direct write awaits completion, so it leaves no deferred flush. Verified byte-exact on real files (large+small mixed writes, mid-file overwrite, read-back) and end to end via the snarkjs Full process suite (49/49). Symmetric with the read path's memory win for large sequential I/O. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fastfile.js imported `{ constants }` from "node:fs", which bundlers that stub
the "fs" module for the browser (e.g. snarkjs's rollup config) do not cover,
so the browser build left node:fs as an undefined external global and crashed
with "node_fs is not defined".
Read O_TRUNC/O_CREAT/O_RDWR/O_EXCL/O_RDONLY off `fs.constants` via the existing
`import fs from "fs"` instead. The "fs" module is already stubbed for browser
builds, and these flags are only used on the Node file path (browsers use
MemFile), so `fs.constants || {}` is harmless there.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…p_build # Conflicts: # build/main.cjs # src/fastfile.js
_rangeHasDirtyPages / _rangeHasCachedPages walked every page index in
[firstPage, lastPage], so a large direct read/write scanned the whole range
(e.g. ~12.5M iterations for a 100GB span with small pages) just to look for
conflicting cached pages.
Iterate the actually-cached pages via Object.keys(this.pages) and test range
membership instead. The cache holds few pages, so the check is now O(cached
pages) regardless of range size or page size, and order-independent. Using
Object.keys (own enumerable keys) also keeps it immune to prototype pollution.
Result set is unchanged: { cached pages with index in [firstPage, lastPage] }.
Verified byte-exact via a direct-io guard test (large+small mixed writes,
mid-file overwrite with a cached page in range, read-back) plus fastfile 15/15
and the snarkjs node suite 49/49.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n) + tests The direct read/write fast paths hand the buffer straight to fd.read / fd.write, which require a real TypedArray/DataView. fastfile is also called with a BigBuffer (paged, not an ArrayBufferView) for large sections; the cached path handles those via BigBuffer's own .set()/.slice(), but the direct path passed the BigBuffer to fd.read and silently left it unfilled -> corrupted data. This surfaced as `powersoftau verify` on a prepared ptau failing with "Phase2 caclutation does not match with powers of tau": the verify reads section chunks into a BigBuffer, and the direct read returned garbage. The in-memory test suites never caught it because they use MemFile, not OsFile; only on-disk ceremony/zkey I/O hits this path (it's exercised by the snarkjs tutorial). Gate both fast paths on ArrayBuffer.isView(buff): a BigBuffer now falls back to the cached path. Plain Uint8Array sections (e.g. the groth16 prove zkey reads) still get the direct-io speed/memory win. Adds two on-disk OsFile regression tests (read into / write from a 2MB BigBuffer, above the 1MB direct threshold). Verified they fail without the gate and pass with it. fastfile suite 17/17. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
readExisting used `process.browser` to decide whether a string argument is a URL to fetch or a file path to open. `process.browser` is a webpack-ism and is undefined under Vite/esbuild/SES -> ReferenceError. Replace with a non-throwing `isNode` (process.versions.node, true for Node/Bun/Deno). Node-like envs open the file; everything else falls back to fetch, which is universal (Node 18+, Bun, Deno, edge, browser) -- not a browser-only API. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… hand-stubbing Browser bundlers got the raw src, which imports `fs` (osfile) and the open-mode flags from "constants" -- unresolved without consumer-side stubs. Add a package.json "browser" field: fs -> false (empty; only used on the Node OsFile path, browsers use MemFile), and constants -> a small browser-stub file with NAMED exports (O_TRUNC etc. = 0, unused in browser). The named stub matters because fastfile.js does a NAMED import; an empty `false` stub has no named exports and would fail that import (and a default import would break snarkjs's named-only virtual constants stub at build time). Node behavior unchanged (browser field ignored by Node; real constants used). Verified: clean browser bundle resolution, full snarkjs build, browser + Node tutorial e2e. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
npm audit fix for the in-range bumps (ajv, brace-expansion, cross-spawn, flatted, ...); serialize-javascript ^7.0.5, diff ^8.0.3 and minimatch ^3.1.4 overridden (mocha 10 pins vulnerable ranges). npm audit clean; 17 tests pass.
readToBuffer's cached-page path (reads below directReadThreshold, i.e. the common case for most zkey/ptau/wtns sections) computed the destination-buffer write offset as `offset + len - r`, where `r` had already been clamped to the EOF-truncated byte count on the previous line. That put the on-disk bytes at a nonzero offset in the output buffer instead of at the start -- the START of the returned buffer came back as zeros (the caller's fresh-allocated buffer contents) while the truncated tail held garbage/misplaced real data, rather than the read failing cleanly or zero-padding only the genuinely missing tail. Reproduced with a section whose declared size exceeds the actual (truncated/corrupted) file: readSection returned bytes shifted by exactly the truncated amount. Fixed by tracking bytes-written with an independent counter instead of deriving the offset from the EOF-clamped remaining-count.
…o close() Two error-propagation bugs in osfile's async page cache, the same class as the recently-fixed ThreadManager hangs: 1. A failed page read rejected only the FIRST waiter; co-readers queued on page.loading were never settled (awaited forever), and the page stayed cached with a dead loading list so every future reader of that page hung too. Now every waiter is rejected and the page is dropped so a retry re-reads it. 2. A failed background page flush was recorded in self.error and only surfaced at close(); page.writing also stayed true, pinning the page. A prover that skipped close() on its error path silently produced a truncated file while 'succeeding'. Now write()/ readToBuffer() fail fast once a flush error is recorded, writing is cleared, and _tryClose no longer falls through to resolve after rejecting. Both covered by regression tests (fail pre-fix: one as a hang, one as a silent success).
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.
Direct-IO fast paths + page-cache correctness and robustness fixes
Summary
Two performance fast paths for large transfers, followed by a series of
correctness and error-propagation fixes for the page cache, all covered by
new tests. snarkjs/binfileutils drive every zkey/ptau/wtns through this
library, so large sequential section reads/writes dominate its real-world
profile.
Performance
copy straight from disk into the caller's buffer, skipping the page cache
and its page→destination copy (the dominant cost of large sequential
section reads).
cached pages; any cached page in range falls back to the cached path so
nothing goes stale.
paths iterate the actually-cached pages instead of every page index in the
range, keeping them cheap for huge ranges with small pages.
Correctness
fd.read/fd.write, which requires a real TypedArray/DataView. ABigBuffer (paged, not a view) silently corrupted data; both paths are now
gated on
ArrayBuffer.isViewand BigBuffers take the cached path (whichcopies via their own
.set()). Regression-tested with >1 GiB-styledsection reads/writes.
truncated file computed the destination offset from the EOF-clamped
remaining count, silently shifting the valid bytes to the wrong position in
the output buffer. Tracked independently now; the real bytes land at the
start.
first waiter — co-readers of the same page were queued on
page.loadingand awaited forever; the dead page also blocked retries. A failed
background flush left
page.writingset forever (pinning the page andwedging
close()) and surfaced the error only atclose()— which errorpaths often skip, silently truncating the file. Now: every waiter is
rejected and the page dropped for retry; flush errors clear the flag, latch
self.error, fail the nextread/writefast, and rejectclose().Compatibility
process.versions.node) instead of thewebpack-only
process.browser, which is undefined under Vite/esbuild/SES.browserfield stubsfs/constantsso bundlers need no hand-writtenstubs; open flags come from
fs.constantsrather than importingnode:fsin the shared module.
Validation
npm test: 21 passing, including the new regression tests for the BigBufferdirect-io gate, the EOF offset shift, multi-waiter page-read failure, and
background-flush error surfacing. Each fix's test was verified to fail
against the pre-fix code.
Stacked work
The HTTP Range / Blob streaming backends build on this branch and are PR'd
separately from
feature/url-blob-streaming.🤖 Generated with Claude Code
https://claude.ai/code/session_01Wo6AVSAwvL9mHTpvnREZPR