Skip to content

perf: avoid flattening and shifting result batches - #577

Merged
vuanhphung merged 4 commits into
mainfrom
perf/result-slicer-offsets
Nov 14, 2026
Merged

vuanhphung merged 4 commits into
mainfrom
perf/result-slicer-offsets

Conversation

@vuanhphung

@vuanhphung vuanhphung commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator

Before: Accumulate provider batches until the requested chunk size (100,000 rows by default), splice() the needed prefix from the final batch, save its tail, and join pieces with flat(). Small fetches repeatedly shift the saved tail; fetchAll() also joins with flat().

After: Take the needed prefix with slice() and copy the unread tail once into an owned buffer. Serve leftovers with slice() plus an offset, clearing consumed slots immediately. Compact after more than half the buffer is consumed and reset when empty. This releases returned rows without mutating provider batches or repeatedly shifting the tail. Reuse a single piece or join pieces with a copy loop shared by fetchAll(), capping initial allocation for large joins. Chunk sizes, order, and buffering behavior stay the same.

Buffer after each fetch from a 12-row batch, fetching 3 at a time:

Before: splice removes the prefix and shifts the unread tail
Return 1–3:  [4 5 6 7 8 9 10 11 12]
Return 4–6:  [7 8 9 10 11 12]
Return 7–9:  [10 11 12]
Return rest: []

After: clear consumed slots and advance the offset
Return 1–3:  [4 5 6 7 8 9 10 11 12]  offset=0
Return 4–6:  [· · · 7 8 9 10 11 12]  offset=3
Return 7–9:  [10 11 12]                offset=0 (compacted)
Return rest: []                        offset=0

· means a cleared slot: the buffer no longer references that row.

Local Node 24.14 results against rebased main (93a9112), with identical locked dependencies. Medians of five sequential fresh-process samples per build after warmup; synthetic providers, no network:

Workload Wall time, main → PR CPU time, main → PR Peak RSS, main → PR
80M-row Arrow pipeline, materialization off 2.010 → 0.912 s 2.259 → 1.152 s 301 → 280 MiB
1M materialized rows, fetch 1,000 0.374 → 0.040 s 0.444 → 0.108 s 215 → 231 MiB
fetchAll(), 80M rows 2.965 → 2.008 s 3.741 → 2.768 s 3,350 → 2,901 MiB

The review's 10,000-row retention case now keeps zero consumed objects, matching main; the prior PR kept 5,000 at halfway. Retained heap at halfway falls from 20.84 to 0.88 MiB. The owned copy costs ~16 MiB peak RSS in the 1M-row stress case.

Validation: 1,772 unit tests, lint, formatting, type checking, and build pass. Another 2,000 deterministic scenarios match main for rows, source fetches, and hasMore().

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No issues identified by the review bot.

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No issues identified by the review bot.

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Nit

Looks good, no blocking issues. I compared the new offset-based ResultSlicer with the old splice/flat logic for leftovers, exact fits, fractional limits, NaN, zero and negative limits, undefined limits, unbuffered mode and the concatChunks growth past 2**25, and the results match. One benign difference: concatChunks now keeps holes as undefined where flat() dropped them; no current source produces sparse chunks.

Other findings

  • ⚪ Nit — The new CHANGELOG entry has no PR reference. Neighbouring entries end with one (for example (#471)), so this one should end with (#577).

@aakash-saravanan-db

Copy link
Copy Markdown
Collaborator

Advancing remainingOffset keeps already returned row objects strongly referenced until more than half the batch is consumed. I reproduced this with 10,000 materialized rows fetched in 1,000-row chunks: after the caller discards the first 5,000 rows, this implementation still retains all 5,000; the previous implementation retains none.

For slow consumers of wide rows, this keeps substantial processed data alive between fetches. The 2× unread-row-count bound doesn’t bound retained bytes when row sizes vary, and the benchmarks with materialization disabled won’t expose this.

Could we preserve the offset optimization while releasing consumed references on each fetch? One option is to copy the unread tail once into owned storage, then clear consumed slots without modifying the source array.

Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No issues identified by the review bot.

@vuanhphung

Copy link
Copy Markdown
Collaborator Author

@aakash-saravanan-db PTAL!

@aakash-saravanan-db aakash-saravanan-db left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The updated owned-tail buffer and immediate clearing of consumed slots resolve the row-retention concern without mutating provider batches or previously returned chunks. Chunking, pagination, and the public fetch/iterator/stream flows remain preserved.

Validated on Node 24.22.1: 1,772 unit tests, build, and type checking passed; 2,000 differential scenarios matched main, all 11 offline Arrow CUJ checks passed, and the halfway GC check retained zero consumed objects, matching main.

@vuanhphung
vuanhphung added this pull request to the merge queue Nov 14, 2026
Merged via the queue into main with commit 8ccddd0 Nov 14, 2026
23 checks passed
@vuanhphung
vuanhphung deleted the perf/result-slicer-offsets branch November 14, 2026 00:45

This branch was previously deployed

1 inactive deployment
azure-prod — 5ea7104a Deployed Nov 13, 2026 by vuanhphung via e2e-test (24.22.1, 24_22_1) #2010
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants