Make CachedArrayReader cache cleanup incremental instead of quadratic - #10901
Draft
zhuqi-lucas wants to merge 1 commit into
Draft
Make CachedArrayReader cache cleanup incremental instead of quadratic#10901zhuqi-lucas wants to merge 1 commit into
zhuqi-lucas wants to merge 1 commit into
Conversation
cleanup_consumed_batches rescanned batch ids from 0 on every consume_batch call, re-removing ids that earlier calls had already removed, and took the shared cache's write lock each time even when there was nothing left to do. Over a row group with N batches this is O(N^2) remove calls on the shared cache. Track the already-cleaned frontier and only remove the new range, skipping the lock entirely when the frontier has not advanced. Found while profiling apache#10774: the consumer-role reader calls this once per consume_batch, i.e. once per output batch per cached column.
Contributor
Author
|
run benchmarks arrow_reader_clickbench |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix-cached-reader-quadratic-cleanup (e3f0020) to 7e65400 (merge-base) diff Run configurationrun benchmark arrow_reader_clickbenchBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_reader_clickbench File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing fix-cached-reader-quadratic-cleanup (e3f0020) to 7e65400 (merge-base) diff Run configurationrun benchmark arrow_reader_clickbenchCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
Author
|
run benchmarks arrow_reader_clickbench |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix-cached-reader-quadratic-cleanup (e3f0020) to 7e65400 (merge-base) diff Run configurationrun benchmark arrow_reader_clickbenchBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_reader_clickbench File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing fix-cached-reader-quadratic-cleanup (e3f0020) to 7e65400 (merge-base) diff Run configurationrun benchmark arrow_reader_clickbenchCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the performance characteristics of CachedArrayReader’s shared-cache cleanup by making it incremental rather than repeatedly rescanning from batch 0 on every consume_batch call. This reduces unnecessary remove calls and avoids taking the shared cache write lock when there is no new work to do, aligning with the cache’s intended “consume and release” lifecycle during the Consumer phase.
Changes:
- Add
cleaned_up_toto track the already-cleaned batch-id frontier for a Consumer reader. - Update
cleanup_consumed_batchesto remove only newly-consumed batch IDs and to early-return without acquiring the write lock when appropriate. - Adjust
cleanup_consumed_batchesto take&mut selfto safely update the cleanup frontier.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
zhuqi-lucas
marked this pull request as draft
August 28, 2026 07:12
Contributor
|
Do you think this one ready to review @zhuqi-lucas ? |
alamb
reviewed
Sep 1, 2026
| local_cache: HashMap<BatchID, ArrayRef>, | ||
| /// Statistics to report on the Cache behavior | ||
| metrics: ArrowReaderMetrics, | ||
| /// Exclusive upper bound of the batch ids already removed from the shared |
| }, | ||
| ); | ||
| } | ||
| drop(cache); |
Contributor
There was a problem hiding this comment.
Why drop the write lock before setting cleaned_up_to?
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.
Which issue does this PR close?
Rationale for this change
cleanup_consumed_batchesrescans batch ids from 0 on everyconsume_batchcall, re-removing ids that earlier calls already removed, and takes the shared cache's write lock each time even when there is nothing left to remove:Over a row group with N cached batches this is O(N²)
removecalls; the consumer-role reader runs it once per output batch per cached column.What changes are included in this PR?
Track the already-cleaned frontier (
cleaned_up_to) and remove only the new range. When the frontier has not advanced, return without touching the lock.Are these changes tested?
Covered by the existing
cached_array_readerunit tests and the parquet--libsuite (1331 passed; the one failure,test_int96_interop, is a missingparquet-testingfile unrelated to this change).Performance
Two
arrow_reader_clickbenchbot runs below: neutral. Withbatch_size = 8192, N is small enough (~15 per row group on this dataset) that the quadratic costs microseconds — so this is a hygiene fix, not a measurable win at this scale. The pattern grows quadratically with row-group size, and dropping the per-call write-lock acquisition also matters more under concurrent readers than in this single-stream bench.(Per-run flags that did not reproduce: run 1 showed async Q20/Q21 swings that vanished in run 2; async/Q22 shows +11% in both runs, but the same query on the
syncandasync_object_storevariants — same reader code — is neutral-to-faster in both runs, and the async baseline's ±16ms variance points at that variant's flakiness rather than this change.)Are there any user-facing changes?
No.
cleanup_consumed_batchesnow takes&mut self, but it is a private method of apub(crate)type.