perf(zql): stop MemorySource#fetch being a generator - #6509
Closed
arv wants to merge 1 commit into
Closed
Conversation
Chained generators are the most expensive way to move a row on Hermes. A four-deep pipeline over 5000 rows measured 31.9 ms against 16.2 ms for the same chain written as manual iterators and 1.2 ms for a plain loop, and a CPU profile of `hydrate: issues with creator` put 5.4% of the benchmark in generator resume plumbing alone, before counting what hides inside the generator bodies themselves. Three changes, all confined to the fetch path: - `generateRows` delegated with `yield*` to something that is already an `IterableIterator`. That whole generator layer bought nothing and cost a resume per row; it now returns the BTree's iterator directly. Single-use semantics are unchanged -- an `IterableIterator` returns itself from `[Symbol.iterator]()`, exactly as a generator does. - `#fetch` is no longer a generator. Its setup contains no yields, so the three exits become plain returns. The hot path -- no overlay, no start, no filters, which is what a plain scan and every join child-lookup take -- is now `ConstrainedRowIterator` rather than a generator loop. - `generateRows` picked its method with `data[reverse ? a : b]()`. The computed member forces a dynamic lookup Hermes cannot inline-cache; the ternary-of-calls form measured 18% faster on the same object. `#fetch` keeps its laziness. A generator body does not run until the first `next()`, and this one reads `#overlay` and `conn.lastPushedEpoch`, so a caller may fetch and only iterate after a push. `LazyStream` preserves that timing exactly, and propagates `return()` so early termination still closes the underlying scan -- a leaked SQLite cursor makes later writes on the same connection fail. Android emulator, median of 3 runs (rn-bench --repeat 3): hydrate: issues filtered open 87.95 -> 76.46 ms -13.1% hydrate: issues only 77.74 -> 72.64 ms -6.6% hydrate: issues with creator + comments 888.98 -> 849.31 ms -4.5% hydrate: issues with creator 405.42 -> 389.99 ms -3.8% hydrate: issues limit 50 11.72 -> 11.48 ms -2.0% Push is flat, as expected -- it does not run this path. Baseline spreads were 0.3-3.5% except 'add comment' at 6.9%.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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
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.
Chained generators are the most expensive way to move a row through the IVM
pipeline, on both engines we care about. This removes them from
MemorySource's fetch path.Why
Profiling the ZQL benchmarks on Hermes (harness in #6507) put 5.4% of
hydrate: issues with creatorin generator resume plumbing alone —nextandgeneratorPrototypeResumeframes — before counting what hides inside thegenerator bodies themselves. A four-deep pipeline over 5000 rows, measured in
the app's own runtime:
Relative to a plain loop, four chained generators cost 31x on Hermes and
74x on V8. V8 optimizes plain loops and hand-written iterators very
aggressively and generators are a barrier it will not cross, so removing a
generator layer pays more there. This is not a Hermes-specific fix.
What changed
All of it confined to the fetch path in
memory-source.ts:generateRowswas a pureyield*passthrough over something that isalready an
IterableIterator. That whole generator layer bought nothing andcost a resume per row. It now returns the BTree's iterator directly.
Single-use semantics are unchanged: an
IterableIteratorreturns itself from[Symbol.iterator](), exactly as a generator does.#fetchis no longer a generator. Its setup contains no yields, so thethree exits become plain returns. The hot path — no overlay, no
start, nofilters, which is what a plain scan and every join child-lookup take — is now
a hand-written
ConstrainedRowIteratorrather than a generator loop.generateRowspicked its method withdata[reverse ? a : b](). Thecomputed member forces a dynamic lookup Hermes cannot inline-cache; the
ternary-of-calls form measured 18% faster there. See the caveat below.
The part worth reviewing closely
#fetchkeeps its laziness. A generator body does not run until the firstnext(), and this one reads#overlayandconn.lastPushedEpoch— a callermay legitimately
fetch()and only iterate after a push. Making#fetchanordinary eager function would move that snapshot earlier and change overlay
behaviour in exactly the cases the surrounding comments warn about.
LazyStreampreserves the timing exactly: setup on first
next(), single-use,[Symbol.iterator]()returns itself.It also forwards
return(), so early termination still closes the underlyingscan. The
mergeSortedStreamscomment is explicit about why that matters: aleaked SQLite cursor makes later writes on the same connection fail with
"database connection is busy executing a query".
Measurements
Android emulator, median of 3 runs (
rn-bench --repeat 3, both sides rebuilt;baseline spreads 0.3–3.5% except
add commentat 6.9%). V8 ispnpm --filter zql-benchmarks run bench ivm-memory -t hydration, median of 3.Push is flat on Hermes (−1.2% to +3.7%, inside the noise) — it does not run
this path.
Caveat on the third change
The
o[b ? 'a' : 'b']()→b ? o.a() : o.b()result is Hermes-measuredonly. I could not get a trustworthy V8 number: the computed form timed 1.0 ms
for one branch and 19.9 ms for the other while the ternary sat at ~9.6 ms both
ways, stable across runs and two probe designs. 1.0 ms for 2M allocating calls
is 0.5 ns each, so V8's escape analysis is scalar-replacing the returned object
asymmetrically and the microbenchmark is not measuring the property access.
Treat that edit as Hermes-motivated and V8-unknown. It is subsumed by the
end-to-end numbers above, which do show a V8 win overall.
Verification
zql1448 ·zqlite199 ·zero-cache5078 ·zero-client659 tests pass.check-types,lint,check-formatclean.zero-clienthas one pre-existing failure (logged-out client uses a private storage sentinel for idb naming) that fails identically onmain.Scope
Deliberately limited to
memory-source.ts's fetch path — that is where theprofile pointed and where the measurement confirms the win.
generateWithOverlay,mergeSortedStreamsand the operator-level generatorsare untouched; the repo has ~120
yield*sites and sweeping them would be aseparate change needing its own measurement.