Reduce read amplification when opening large models - #1597
Reduce read amplification when opening large models#1597Hyungkeun-Park-Nota wants to merge 1 commit into
Conversation
19102d1 to
936b22e
Compare
|
@Hyungkeun-Park-Nota this change causes regressions in existing test files:
|
FileStream._fill and the stream-backed readers always filled a fixed 256MB window on a cache miss, regardless of how many bytes were requested, so a 14-byte signature peek read 256MB from disk and formats whose entries are scattered across the file re-filled the window once per entry. Size the window to the request instead, with a 64KB minimum. zip.InflaterStream restarts inflation from the beginning of the entry whenever the requested end offset exceeds what is already inflated, so a reader that advances in small steps re-inflates the whole entry on every step. Grow the target geometrically, which bounds the total inflate work to about twice the entry size however the reader steps through it.
517b1ad to
c16e38f
Compare
|
Thanks — reproduced both, and the cause was not the window size itself.
Fixed by growing the inflate target geometrically, which bounds the total inflate work to if (size !== undefined && this._buffer !== undefined) {
size = Math.max(size, this._buffer.length * 2);
}Both files are now faster than
On models with an identical graph and ~1GB of weights,
One thing worth flagging: our numbers for the same file diverged a lot — you measured My original benchmarks only covered uncompressed containers, which is why the inflate path |
0708c0b to
ca43c4e
Compare
Fixes the read amplification described in #1596.
FileStream._filland the two stream-backed readers always filled a fixed 256MB window on acache miss, regardless of how many bytes were requested. A 14-byte signature peek read 256MB
from disk, and archive formats whose entries are scattered across the file re-filled the
window once per entry.
This sizes the window to the request instead, with a 64KB minimum, in
node.FileStream._fill,browser.FileStream._fill,protobuf.StreamReader._fillandpython.StreamReader._fill.Results
Models with an identical 16-node graph and ~1GB of weights, so the difference is purely
data-path cost:
.pt2.npz.pt(state_dict).pt(torchscript).safetensors.gguf.onnxVerification
node package.js validatepasses (lint,tag:validation, desktop and browser Playwrighttests).
test/models.jsadditionally passes for one model of each of the 71types in
models.json..tar/.tar.gz/gzip-wrapped variants) a fingerprint over the graph structure plus achecksum of every initializer's bytes is identical before and after the change.
driven side by side with the same randomized sequence of
seek/skip/read/peek(deterministic seed), asserting they agree with each other and with the underlying bytes:
node.FileStream20,060 read/peek ops plusstream()sub-views,browser.FileStream29,737 read/peek ops, all byte-identical. Happy to contribute this as a test if there is a
place for it.
.pt20.15s -> 0.12s, 1MB.onnx0.05s -> 0.05s).amplification returns, so 64KB is used as the minimum.
The timings are from the Node/desktop path.
browser.FileStreamcarries the same change andis covered by the differential test and the browser Playwright test, but I have no browser
timings for it.