perf(zql): de-generator the pure-passthrough push delegations - #6524
Draft
arv wants to merge 5 commits into
Draft
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
BTreeSet.toArray() can silently truncate sets containing an undefined key.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Optimizes ZQL push pipelines and B-tree scans by removing unnecessary generator and iterator-protocol overhead.
Changes:
- Converts pass-through filter pushes to direct stream delegation.
- Makes
BTreeSetpull-only and adds array draining. - Removes the unused
Storage.scanAPI and tests.
File summaries
| File | Description |
|---|---|
packages/zqlite/src/database-storage.ts |
Removes storage scanning. |
packages/zqlite/src/database-storage.test.ts |
Replaces scan assertions with gets. |
packages/zql/src/ivm/stream.ts |
Adds shared empty push stream. |
packages/zql/src/ivm/operator.ts |
Removes Storage.scan. |
packages/zql/src/ivm/memory-storage.ts |
Uses BTreeSet.toArray. |
packages/zql/src/ivm/memory-storage.test.ts |
Removes scan tests. |
packages/zql/src/ivm/memory-source.ts |
Avoids iterable-based index construction. |
packages/zql/src/ivm/maybe-split-and-push-edit-change.ts |
Directly delegates push streams. |
packages/zql/src/ivm/filter.ts |
Removes generator delegation. |
packages/zql/src/ivm/filter-push.ts |
Returns downstream or empty streams directly. |
packages/zql/src/ivm/filter-operators.ts |
Removes pass-through generators. |
packages/shared/src/btree-set.ts |
Replaces iterability with pull-only traversal. |
packages/shared/src/btree-set.test.ts |
Adapts tests to pull traversal. |
packages/shared/src/btree-set.bench.ts |
Benchmarks pull traversal and toArray. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /** Collects the rest of `it` into an array. */ | ||
| export function drainValues<K>(it: ValueIterator<K>): K[] { | ||
| const result: K[] = []; | ||
| for (let v = it.nextValue(); v !== undefined; v = it.nextValue()) { |
Comment on lines
159
to
161
| get: (key, def?) => this.#get(cgID, opID, key, def), | ||
| set: (key, val) => this.#set(cgID, opID, key, val), | ||
| del: key => this.#del(cgID, opID, key), |
The whole filter chain's push path never yields on its own: Filter, FilterStart, FilterEnd, filterPush and maybeSplitAndPushEditChange are all pure `yield*` delegations. Each was a generator layer costing a resume per yield and buying nothing, the same shape as the old `generateRows`. Also adds BTreeSet#toArray so building a secondary index and cloning storage stop allocating an IteratorResult per row.
The iterator protocol allocates a result object per value, which is the cost the IVM fetch path exists to avoid, and leaving an iterable API in place is an easy way to reintroduce it by accident. ValueIterator is now pull-only (`nextValue()`), BTreeSet exposes `toArray()` instead of `[Symbol.iterator]`, and `drainValues()` collects a scan where a test needs an array. Also drops Storage#scan, which had no production callers -- only its own tests -- and was the last thing in zql that iterated a BTreeSet. RowScan#close() no longer calls `return?.()`: the BTree iterators never defined it, so it was always a no-op, and an index scan holds no external resource to release.
`nextValue()` uses `undefined` as its exhaustion sentinel, so a set whose keys
can be `undefined` is now ambiguous -- that was fine while `next()` still
returned an IteratorResult, and is not any more.
map.ts already had a private `Defined = {} | null` for the same reason (a
`Map.get` miss); it moves to defined.ts so both can use it.
arv
force-pushed
the
arv/ivm-push-fns
branch
from
September 10, 2026 14:46
b05f52c to
42906ce
Compare
arv
marked this pull request as draft
September 10, 2026 14:46
Every close was a hand-written try/finally, and the ones that were forgotten are what d3fbd4c had to go back and fix. This adds the scope-based form and closes the leaks the audit turned up. - `withPull(stream, fn)` closes on the normal path and on a throw, the pull analogue of `withRead`/`withWrite`. Uses try/catch rather than a plain `finally` so a failing close cannot replace the operation's error. - `forEachPull(stream, fn)` adds the loop; `fn` returns `'break'` to stop early. A `break` out of a hand-written loop is exactly the abrupt completion that needed the `finally`. - `forEachSkippingYields` for the Node-stream case, next to `skipYields`. Named for what it drops: a consumer that can suspend must propagate the markers, so the choice should be visible at the call site. Leaks found while writing the tests, all the same shape: - `filterPull`, `takeWhilePull` and `mapPull` left the source open when the caller's callback threw. A combinator has no scope to close in and the exception propagates past the caller's own close(), so it has to release the source itself. - `limitedScan` set `done` in its catch without closing, so the caller's close() then skipped the source -- the same bug d3fbd4c fixed in FilterStartPull. - `drainPull` and `drainPullMap` never closed at all. Six of the thirteen new tests fail without these fixes. Converts the eight call sites whose loop body can move into a callback. The remaining thirteen are inside push-path generators and use `yield`, which cannot cross a function boundary; those keep their try/finally.
`void | 'break'` rejected a shorthand arrow whose body produces a value (`v => rows.push(v)`): TypeScript only lets a function return anything when the expected type is exactly `void`, and any union -- including `void | undefined | 'break'` -- drops that. The trade is not avoidable. The rule that permits the shorthand is 'ignore the returned value', so nothing catches a misspelled sentinel and also accepts the shorthand; overloads restore the shorthand but swallow the typo just as quietly. Documented on the function and pinned by a test.
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.
Stacked on
mlaw/ivm-pull-fns(was #6523 /arv/ivm-pull-fns).Two changes to what is left of the iterator protocol after the fetch path moved
to pull functions.
1. The filter chain's push path was pure
yield*Output.pushreturnsStream<'yield'>so that a slow push can suspend. ButFilter,FilterStart,FilterEnd,filterPushandmaybeSplitAndPushEditChangenever yield on their own — every branch of allfive is a plain delegation. Each was a generator layer costing a resume per
yield and buying nothing, the same shape as the old
generateRows.They are now ordinary functions that return the downstream stream:
Downstream stays lazy — calling a generator function does not run its body —
and the empty cases share one frozen
EMPTY_YIELDSinstead of each allocatinga generator.
buildFilterPipelineinserts a FilterStart/FilterEnd pair intoevery pipeline, so this removes at least two generator layers from every
push, filtered or not.
The operators that genuinely suspend (
take,exists,cap,skip,union-fan-in, the sources) are untouched. ReplacingStream<'yield'>itselfwants a resumable-continuation type rather than
PullStream<T>— push carriesno values — and is a separate design.
One semantic change: the predicate now runs at
push()call time ratherthan at first
next(). No call site anywhere stores a push stream withoutconsuming it at the same site — all 67 consumers are inline
yield*/for...of/consume— and the predicate is required pure, so it is not observable.2. BTreeSet is no longer iterable
The iterator protocol allocates a result object per value, which is the cost
the fetch path exists to avoid, and an iterable API is an easy way to
reintroduce it by accident.
Finding the real callers by making
next()throw turned up one on a hot path:toSorted(tree, cmp)is[...tree].sort(), so every secondary-index buildallocated one
IteratorResultper row. That is nowBTreeSet#toArray(),which drains via
nextValue().With that gone,
next()had no production callers left, soValueIteratorispull-only,
[Symbol.iterator]is gone from the iterators and fromBTreeSet,and
drainValues()collects a scan where a test needs an array.Two things fell out:
Storage#scanis deleted. It had no production callers — only its owntests — and was the last thing in zql iterating a BTreeSet. zqlite's
deltest used it only to verify deletion and now asserts with
get.RowScan#close()was callingthis.#rows.return?.(), which was always ano-op — the BTree iterators never defined
return. Removing iterabilitymade that visible. An index scan holds no external resource, so
close()now just sets
#done. Worth noting given how load-bearingclose()is forthe SQLite cursors; this particular call never did anything.
Results
Android emulator (Hermes),
--run push, 4 paired interleaved rounds, medians.ops/sec, so higher is better:
The two
Filter / pushbenchmarks are the direct target and every round waspositive. The
MemorySource pushgains follow from the same cause: even afilterless push crosses the FilterStart/FilterEnd pair.
Everything dominated by join or relationship work is flat —
Join / push edit issue title−0.3%,push: add issue (with creator join)−0.2%, therelationship-heavy view 2.2–2.5% against ±3% noise. Those layers still suspend
for real and were untouched.
This does not fix #6523's push regression.
push: add comment (child relation)is +0.6% against ±2.4% noise — flat, not recovered.On the measurement
V8 could not resolve this at all. Five paired rounds on the host produced
nothing usable:
push: add issue (no join)read −12.4% with all four of thefirst rounds negative, then round five came back +13.0%. One
hydrate: issues with creatorround swung +40.4%. Had I stopped at four roundsI would have reported a win that is not there. The emulator's in-run spreads
are ±0.3–0.4% on the benchmarks that matter here, so the Android numbers are
the ones to trust.
Caveat: one
add open issueround came in at +16.3% against +6.7/+8.4/+9.0.The median ignores it, but treat 8.7% as approximate.
Verification
shared 772 · zql 1447 · zqlite 198 — each down by exactly the tests removed
(two iterator-protocol tests, the
MemoryStorage.scantest, thescan prefixtest). Types and lint clean.
The hydration A/B did not produce a usable result: the Android emulator was
too degraded after hours of benchmarking, with in-run noise at a median of
±23.9% and a uniform -76% swing across all ten benchmarks in one round. It
needs a cold emulator and a
pm clearbetween arms, and should be re-runtogether with the push suite against the new base.