You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zhuqi-lucas (via GitHub)" <gi...@apache.org> on 2026/08/28 03:33:53 UTC
[PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
zhuqi-lucas opened a new pull request, #10901:
URL: https://github.com/apache/arrow-rs/pull/10901
## Which issue does this PR close?
- Part of #10774.
## Rationale for this change
While profiling #10774 (`CachedArrayReader::consume_batch` on ClickBench Q12/Q25 shapes, where the filter column is also the projected column), the largest avoidable cost turned out not to be the `filter`+`concat` pair itself but `cleanup_consumed_batches`:
```rust
for batch_id_to_remove in 0..(current_batch_id.val - 1) {
cache.remove(...);
}
```
It rescans batch ids from 0 on every `consume_batch` call, 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 batches this is O(N²) `remove` calls; 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 `cleaned_up_to..end`. When the frontier has not advanced, return without touching the lock.
## Are these changes tested?
Covered by the existing `cached_array_reader` unit tests and the parquet `--lib` suite (1331 passed; the one failure, `test_int96_interop`, is a missing `parquet-testing` file unrelated to this change).
Local `arrow_reader_clickbench` Q12 runs on my machine showed `async` / `async_object_store` improving ~1–2% with `sync` neutral, but the machine's run-to-run drift is of the same order, so I'd rather rely on the benchmark bot for numbers.
## Are there any user-facing changes?
No. `cleanup_consumed_batches` now takes `&mut self`, but it is a private method of a `pub(crate)` type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#issuecomment-5495089747
Do you think this one ready to review @zhuqi-lucas ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#issuecomment-5872014704
I made a small follow on PR here
- https://github.com/apache/arrow-rs/pull/11252
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#discussion_r4119103008
##########
parquet/src/arrow/array_reader/cached_array_reader.rs:
##########
@@ -168,22 +172,33 @@ impl CachedArrayReader {
/// Remove batches from cache that have been completely consumed
/// This is only called for Consumer role readers
- fn cleanup_consumed_batches(&self) {
+ fn cleanup_consumed_batches(&mut self) {
let current_batch_id = self.get_batch_id_from_position(self.outer_position);
Review Comment:
Added in c3e3a4ec5, as `debug_assert!(current_batch_id.val >= self.cleaned_up_to)`. That is the invariant the whole incremental scan rests on — `outer_position` only moves forward, so the watermark can never run ahead of the batch the reader is on.
##########
parquet/src/arrow/array_reader/cached_array_reader.rs:
##########
@@ -168,22 +172,33 @@ impl CachedArrayReader {
/// Remove batches from cache that have been completely consumed
/// This is only called for Consumer role readers
- fn cleanup_consumed_batches(&self) {
+ fn cleanup_consumed_batches(&mut self) {
let current_batch_id = self.get_batch_id_from_position(self.outer_position);
// Remove batches that are at least one batch behind the current position
// This ensures we don't remove batches that might still be needed for the current batch
// We can safely remove batch_id if current_batch_id > batch_id + 1
- if current_batch_id.val > 1 {
- let mut cache = self.shared_cache.write().unwrap();
- for batch_id_to_remove in 0..(current_batch_id.val - 1) {
- cache.remove(
- self.column_idx,
- BatchID {
- val: batch_id_to_remove,
- },
- );
- }
+ if current_batch_id.val <= 1 {
+ return;
+ }
+ let end = current_batch_id.val - 1;
+ // Everything below `cleaned_up_to` was removed by an earlier call.
+ // Rescanning from 0 each time made this quadratic in the number of
Review Comment:
Removed, you are right — it described the loop that is no longer there. Moved to the commit message.
##########
parquet/src/arrow/array_reader/cached_array_reader.rs:
##########
@@ -168,22 +172,33 @@ impl CachedArrayReader {
/// Remove batches from cache that have been completely consumed
/// This is only called for Consumer role readers
- fn cleanup_consumed_batches(&self) {
+ fn cleanup_consumed_batches(&mut self) {
let current_batch_id = self.get_batch_id_from_position(self.outer_position);
// Remove batches that are at least one batch behind the current position
// This ensures we don't remove batches that might still be needed for the current batch
// We can safely remove batch_id if current_batch_id > batch_id + 1
- if current_batch_id.val > 1 {
- let mut cache = self.shared_cache.write().unwrap();
- for batch_id_to_remove in 0..(current_batch_id.val - 1) {
- cache.remove(
- self.column_idx,
- BatchID {
- val: batch_id_to_remove,
- },
- );
- }
+ if current_batch_id.val <= 1 {
+ return;
+ }
+ let end = current_batch_id.val - 1;
+ // Everything below `cleaned_up_to` was removed by an earlier call.
+ // Rescanning from 0 each time made this quadratic in the number of
+ // batches in the row group, and took the shared cache's write lock on
+ // every `consume_batch` even when there was nothing left to remove.
+ if end <= self.cleaned_up_to {
+ return;
+ }
+ let start = self.cleaned_up_to;
+ self.cleaned_up_to = end;
+ let mut cache = self.shared_cache.write().unwrap();
+ for batch_id_to_remove in start..end {
Review Comment:
Done, it collapses to `cache.remove(self.column_idx, BatchID { val });`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#discussion_r4106121587
##########
parquet/src/arrow/array_reader/cached_array_reader.rs:
##########
@@ -168,22 +172,33 @@ impl CachedArrayReader {
/// Remove batches from cache that have been completely consumed
/// This is only called for Consumer role readers
- fn cleanup_consumed_batches(&self) {
+ fn cleanup_consumed_batches(&mut self) {
let current_batch_id = self.get_batch_id_from_position(self.outer_position);
// Remove batches that are at least one batch behind the current position
// This ensures we don't remove batches that might still be needed for the current batch
// We can safely remove batch_id if current_batch_id > batch_id + 1
- if current_batch_id.val > 1 {
- let mut cache = self.shared_cache.write().unwrap();
- for batch_id_to_remove in 0..(current_batch_id.val - 1) {
- cache.remove(
- self.column_idx,
- BatchID {
- val: batch_id_to_remove,
- },
- );
- }
+ if current_batch_id.val <= 1 {
+ return;
+ }
+ let end = current_batch_id.val - 1;
+ // Everything below `cleaned_up_to` was removed by an earlier call.
+ // Rescanning from 0 each time made this quadratic in the number of
Review Comment:
"rescanning from 0 .." describes removed code and is probably not all that useful to leave in code comments (should probably be in commit comments)
##########
parquet/src/arrow/array_reader/cached_array_reader.rs:
##########
@@ -168,22 +172,33 @@ impl CachedArrayReader {
/// Remove batches from cache that have been completely consumed
/// This is only called for Consumer role readers
- fn cleanup_consumed_batches(&self) {
+ fn cleanup_consumed_batches(&mut self) {
let current_batch_id = self.get_batch_id_from_position(self.outer_position);
Review Comment:
Perhaps we can add a debug assert to check the invariant:
```rust
debug_assert!(batch_id.val >= self.cleaned_up_to)
```
##########
parquet/src/arrow/array_reader/cached_array_reader.rs:
##########
@@ -168,22 +172,33 @@ impl CachedArrayReader {
/// Remove batches from cache that have been completely consumed
/// This is only called for Consumer role readers
- fn cleanup_consumed_batches(&self) {
+ fn cleanup_consumed_batches(&mut self) {
let current_batch_id = self.get_batch_id_from_position(self.outer_position);
// Remove batches that are at least one batch behind the current position
// This ensures we don't remove batches that might still be needed for the current batch
// We can safely remove batch_id if current_batch_id > batch_id + 1
- if current_batch_id.val > 1 {
- let mut cache = self.shared_cache.write().unwrap();
- for batch_id_to_remove in 0..(current_batch_id.val - 1) {
- cache.remove(
- self.column_idx,
- BatchID {
- val: batch_id_to_remove,
- },
- );
- }
+ if current_batch_id.val <= 1 {
+ return;
+ }
+ let end = current_batch_id.val - 1;
+ // Everything below `cleaned_up_to` was removed by an earlier call.
+ // Rescanning from 0 each time made this quadratic in the number of
+ // batches in the row group, and took the shared cache's write lock on
+ // every `consume_batch` even when there was nothing left to remove.
+ if end <= self.cleaned_up_to {
+ return;
+ }
+ let start = self.cleaned_up_to;
+ self.cleaned_up_to = end;
+ let mut cache = self.shared_cache.write().unwrap();
+ for batch_id_to_remove in start..end {
Review Comment:
using `val` here rather than `batch_id_to_remove` would result in a single line loop here (as that is the field name)
##########
parquet/src/arrow/array_reader/cached_array_reader.rs:
##########
@@ -168,22 +172,33 @@ impl CachedArrayReader {
/// Remove batches from cache that have been completely consumed
/// This is only called for Consumer role readers
- fn cleanup_consumed_batches(&self) {
+ fn cleanup_consumed_batches(&mut self) {
let current_batch_id = self.get_batch_id_from_position(self.outer_position);
// Remove batches that are at least one batch behind the current position
// This ensures we don't remove batches that might still be needed for the current batch
// We can safely remove batch_id if current_batch_id > batch_id + 1
- if current_batch_id.val > 1 {
- let mut cache = self.shared_cache.write().unwrap();
- for batch_id_to_remove in 0..(current_batch_id.val - 1) {
- cache.remove(
- self.column_idx,
- BatchID {
- val: batch_id_to_remove,
- },
- );
- }
+ if current_batch_id.val <= 1 {
+ return;
+ }
+ let end = current_batch_id.val - 1;
Review Comment:
I think this could be ` let end = current_batch_id.val` as the loop is exclusive below
claude code suggests this simplification
```rust
/// Remove batches from the shared cache that have been completely consumed.
/// This is only called for Consumer role readers.
fn cleanup_consumed_batches(&mut self) {
// `outer_position` only ever moves forward and every lookup is keyed
// off `outer_position / batch_size`, so no batch below the current one
// can be requested again (see the matching `local_cache.retain` in
// `consume_batch`). The current batch may be partially consumed, so it
// is kept.
let end = self.get_batch_id_from_position(self.outer_position).val;
// Everything below `cleaned_up_to` was removed by an earlier call.
if end <= self.cleaned_up_to {
return;
}
let start = self.cleaned_up_to;
self.cleaned_up_to = end;
let mut cache = self.shared_cache.write().unwrap();
for val in start..end {
cache.remove(self.column_idx, BatchID { val });
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "adriangbot (via GitHub)" <gi...@apache.org>.
adriangbot commented on PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#issuecomment-5448133176
🤖 Arrow criterion benchmark running (GKE) | [trigger](https://github.com/apache/arrow-rs/pull/10901#issuecomment-5448118799)
**Instance:** `c4a-highmem-16` (12 vCPU / 65 GiB) | `Linux bench-c5448118799-2031-94b8l 6.12.85+ #1 SMP Sat Jun 27 09:31:30 UTC 2026 aarch64 GNU/Linux`
<details><summary>CPU Details (lscpu)</summary>
```
Architecture: aarch64
CPU op-mode(s): 64-bit
Byte Order: Little Endian
CPU(s): 16
On-line CPU(s) list: 0-15
Vendor ID: ARM
Model name: Neoverse-V2
Model: 1
Thread(s) per core: 1
Core(s) per cluster: 16
Socket(s): -
Cluster(s): 1
Stepping: r0p1
BogoMIPS: 2000.00
Flags: fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 sm3 sm4 asimddp sha512 sve asimdfhm dit uscat ilrcpc flagm sb paca pacg dcpodp sve2 sveaes svepmull svebitperm svesha3 svesm4 flagm2 frint svei8mm svebf16 i8mm bf16 dgh rng bti
L1d cache: 1 MiB (16 instances)
L1i cache: 1 MiB (16 instances)
L2 cache: 32 MiB (16 instances)
L3 cache: 80 MiB (1 instance)
NUMA node(s): 1
NUMA node0 CPU(s): 0-15
Vulnerability Gather data sampling: Not affected
Vulnerability Indirect target selection: Not affected
Vulnerability Itlb multihit: Not affected
Vulnerability L1tf: Not affected
Vulnerability Mds: Not affected
Vulnerability Meltdown: Not affected
Vulnerability Mmio stale data: Not affected
Vulnerability Reg file data sampling: Not affected
Vulnerability Retbleed: Not affected
Vulnerability Spec rstack overflow: Not affected
Vulnerability Spec store bypass: Mitigation; Speculative Store Bypass disabled via prctl
Vulnerability Spectre v1: Mitigation; __user pointer sanitization
Vulnerability Spectre v2: Mitigation; CSV2, BHB
Vulnerability Srbds: Not affected
Vulnerability Tsa: Not affected
Vulnerability Tsx async abort: Not affected
Vulnerability Vmscape: Not affected
```
</details>
Comparing fix-cached-reader-quadratic-cleanup (e3f00206f62c863a267c71127803fe3d6cfc88f3) to 7e65400 (merge-base) [diff](https://github.com/apache/arrow-rs/compare/7e65400964976dc09d4d8415fbae60f296a15054..e3f00206f62c863a267c71127803fe3d6cfc88f3)
<details><summary>Run configuration</summary>
```yaml
run benchmark arrow_reader_clickbench
```
</details>
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_reader_clickbench
Results will be posted here when complete
---
[File an issue](https://github.com/adriangb/datafusion-benchmarking/issues) against this benchmark runner
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#issuecomment-5448118799
run benchmarks arrow_reader_clickbench
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#issuecomment-5550357970
Yes, ready for review — two bot runs are attached above (neutral at this dataset's scale; ~15 batches per row group, so the quadratic costs microseconds here — the win is the pattern not growing with row-group size, plus one less write-lock acquisition per consume_batch).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "adriangbot (via GitHub)" <gi...@apache.org>.
adriangbot commented on PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#issuecomment-5448306178
🤖 Arrow criterion benchmark completed (GKE) | [trigger](https://github.com/apache/arrow-rs/pull/10901#issuecomment-5448118799)
**Instance:** `c4a-highmem-16` (12 vCPU / 65 GiB)
Comparing fix-cached-reader-quadratic-cleanup (e3f00206f62c863a267c71127803fe3d6cfc88f3) to 7e65400 (merge-base) [diff](https://github.com/apache/arrow-rs/compare/7e65400964976dc09d4d8415fbae60f296a15054..e3f00206f62c863a267c71127803fe3d6cfc88f3)
<details><summary>Run configuration</summary>
```yaml
run benchmark arrow_reader_clickbench
```
</details>
<details><summary>CPU Details (lscpu)</summary>
```
Architecture: aarch64
CPU op-mode(s): 64-bit
Byte Order: Little Endian
CPU(s): 16
On-line CPU(s) list: 0-15
Vendor ID: ARM
Model name: Neoverse-V2
Model: 1
Thread(s) per core: 1
Core(s) per cluster: 16
Socket(s): -
Cluster(s): 1
Stepping: r0p1
BogoMIPS: 2000.00
Flags: fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 sm3 sm4 asimddp sha512 sve asimdfhm dit uscat ilrcpc flagm sb paca pacg dcpodp sve2 sveaes svepmull svebitperm svesha3 svesm4 flagm2 frint svei8mm svebf16 i8mm bf16 dgh rng bti
L1d cache: 1 MiB (16 instances)
L1i cache: 1 MiB (16 instances)
L2 cache: 32 MiB (16 instances)
L3 cache: 80 MiB (1 instance)
NUMA node(s): 1
NUMA node0 CPU(s): 0-15
Vulnerability Gather data sampling: Not affected
Vulnerability Indirect target selection: Not affected
Vulnerability Itlb multihit: Not affected
Vulnerability L1tf: Not affected
Vulnerability Mds: Not affected
Vulnerability Meltdown: Not affected
Vulnerability Mmio stale data: Not affected
Vulnerability Reg file data sampling: Not affected
Vulnerability Retbleed: Not affected
Vulnerability Spec rstack overflow: Not affected
Vulnerability Spec store bypass: Mitigation; Speculative Store Bypass disabled via prctl
Vulnerability Spectre v1: Mitigation; __user pointer sanitization
Vulnerability Spectre v2: Mitigation; CSV2, BHB
Vulnerability Srbds: Not affected
Vulnerability Tsa: Not affected
Vulnerability Tsx async abort: Not affected
Vulnerability Vmscape: Not affected
```
</details>
<details><summary>Details</summary>
<p>
```
group fix-cached-reader-quadratic-cleanup main
----- ----------------------------------- ----
arrow_reader_clickbench/async/Q1 1.01 1091.0±4.61µs ? ?/sec 1.00 1085.3±6.19µs ? ?/sec
arrow_reader_clickbench/async/Q10 1.00 6.2±0.11ms ? ?/sec 1.02 6.3±0.16ms ? ?/sec
arrow_reader_clickbench/async/Q11 1.00 7.1±0.06ms ? ?/sec 1.05 7.4±0.12ms ? ?/sec
arrow_reader_clickbench/async/Q12 1.00 13.3±0.22ms ? ?/sec 1.01 13.5±0.15ms ? ?/sec
arrow_reader_clickbench/async/Q13 1.00 16.0±0.18ms ? ?/sec 1.01 16.2±0.19ms ? ?/sec
arrow_reader_clickbench/async/Q14 1.00 14.8±0.10ms ? ?/sec 1.01 14.9±0.10ms ? ?/sec
arrow_reader_clickbench/async/Q19 1.00 3.1±0.03ms ? ?/sec 1.00 3.1±0.03ms ? ?/sec
arrow_reader_clickbench/async/Q20 1.21 88.9±10.67ms ? ?/sec 1.00 73.6±0.39ms ? ?/sec
arrow_reader_clickbench/async/Q21 1.00 81.9±0.73ms ? ?/sec 1.17 95.9±0.98ms ? ?/sec
arrow_reader_clickbench/async/Q22 1.00 125.0±4.68ms ? ?/sec 1.13 141.6±1.37ms ? ?/sec
arrow_reader_clickbench/async/Q23 1.00 254.0±2.12ms ? ?/sec 1.01 256.3±4.54ms ? ?/sec
arrow_reader_clickbench/async/Q24 1.00 18.5±0.19ms ? ?/sec 1.01 18.6±0.16ms ? ?/sec
arrow_reader_clickbench/async/Q27 1.00 58.4±0.43ms ? ?/sec 1.00 58.3±0.38ms ? ?/sec
arrow_reader_clickbench/async/Q28 1.00 58.3±0.63ms ? ?/sec 1.00 58.2±0.41ms ? ?/sec
arrow_reader_clickbench/async/Q30 1.02 17.8±0.10ms ? ?/sec 1.00 17.5±0.09ms ? ?/sec
arrow_reader_clickbench/async/Q36 1.02 15.3±0.39ms ? ?/sec 1.00 15.0±0.34ms ? ?/sec
arrow_reader_clickbench/async/Q37 1.00 5.0±0.03ms ? ?/sec 1.00 5.0±0.03ms ? ?/sec
arrow_reader_clickbench/async/Q38 1.00 13.5±0.30ms ? ?/sec 1.01 13.6±0.28ms ? ?/sec
arrow_reader_clickbench/async/Q39 1.01 25.2±0.55ms ? ?/sec 1.00 25.0±0.62ms ? ?/sec
arrow_reader_clickbench/async/Q40 1.00 5.3±0.05ms ? ?/sec 1.01 5.3±0.08ms ? ?/sec
arrow_reader_clickbench/async/Q41 1.00 4.6±0.03ms ? ?/sec 1.00 4.6±0.03ms ? ?/sec
arrow_reader_clickbench/async/Q42 1.01 3.4±0.02ms ? ?/sec 1.00 3.4±0.02ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q1 1.01 1072.8±6.84µs ? ?/sec 1.00 1063.1±4.10µs ? ?/sec
arrow_reader_clickbench/async_object_store/Q10 1.01 5.9±0.12ms ? ?/sec 1.00 5.8±0.04ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q11 1.00 6.8±0.06ms ? ?/sec 1.00 6.9±0.05ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q12 1.01 13.2±0.07ms ? ?/sec 1.00 13.1±0.08ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q13 1.00 15.9±0.14ms ? ?/sec 1.03 16.4±0.21ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q14 1.00 14.7±0.11ms ? ?/sec 1.03 15.2±0.13ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q19 1.00 3.0±0.02ms ? ?/sec 1.01 3.0±0.04ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q20 1.00 72.6±0.87ms ? ?/sec 1.00 72.6±0.63ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q21 1.00 81.3±0.79ms ? ?/sec 1.00 81.6±0.86ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q22 1.00 100.3±0.76ms ? ?/sec 1.00 100.3±1.05ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q23 1.00 231.6±0.69ms ? ?/sec 1.01 233.8±3.40ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q24 1.00 18.0±0.23ms ? ?/sec 1.01 18.1±0.13ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q27 1.01 57.2±1.82ms ? ?/sec 1.00 57.0±0.64ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q28 1.00 57.0±0.73ms ? ?/sec 1.00 57.0±0.80ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q30 1.00 17.2±0.12ms ? ?/sec 1.00 17.2±0.10ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q36 1.00 14.5±0.21ms ? ?/sec 1.02 14.8±0.26ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q37 1.00 4.9±0.03ms ? ?/sec 1.00 4.9±0.04ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q38 1.00 13.2±0.22ms ? ?/sec 1.01 13.4±0.31ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q39 1.00 24.3±0.53ms ? ?/sec 1.00 24.3±0.55ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q40 1.03 5.2±0.05ms ? ?/sec 1.00 5.1±0.03ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q41 1.01 4.5±0.04ms ? ?/sec 1.00 4.5±0.03ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q42 1.01 3.3±0.02ms ? ?/sec 1.00 3.2±0.02ms ? ?/sec
arrow_reader_clickbench/sync/Q1 1.00 910.3±2.15µs ? ?/sec 1.01 919.2±2.18µs ? ?/sec
arrow_reader_clickbench/sync/Q10 1.00 4.6±0.02ms ? ?/sec 1.00 4.6±0.01ms ? ?/sec
arrow_reader_clickbench/sync/Q11 1.00 5.7±0.05ms ? ?/sec 1.01 5.7±0.05ms ? ?/sec
arrow_reader_clickbench/sync/Q12 1.00 20.5±0.26ms ? ?/sec 1.01 20.8±0.09ms ? ?/sec
arrow_reader_clickbench/sync/Q13 1.00 23.4±0.50ms ? ?/sec 1.00 23.5±0.13ms ? ?/sec
arrow_reader_clickbench/sync/Q14 1.01 22.3±0.14ms ? ?/sec 1.00 22.2±0.08ms ? ?/sec
arrow_reader_clickbench/sync/Q19 1.03 2.8±0.04ms ? ?/sec 1.00 2.7±0.02ms ? ?/sec
arrow_reader_clickbench/sync/Q20 1.02 126.2±0.71ms ? ?/sec 1.00 124.0±0.24ms ? ?/sec
arrow_reader_clickbench/sync/Q21 1.02 100.5±0.70ms ? ?/sec 1.00 98.2±1.19ms ? ?/sec
arrow_reader_clickbench/sync/Q22 1.02 149.5±0.82ms ? ?/sec 1.00 146.3±0.33ms ? ?/sec
arrow_reader_clickbench/sync/Q23 1.00 305.8±8.44ms ? ?/sec 1.01 307.5±12.99ms ? ?/sec
arrow_reader_clickbench/sync/Q24 1.00 26.3±0.13ms ? ?/sec 1.00 26.2±0.13ms ? ?/sec
arrow_reader_clickbench/sync/Q27 1.01 110.2±0.21ms ? ?/sec 1.00 109.6±0.29ms ? ?/sec
arrow_reader_clickbench/sync/Q28 1.01 108.4±0.62ms ? ?/sec 1.00 107.2±0.28ms ? ?/sec
arrow_reader_clickbench/sync/Q30 1.01 17.8±0.15ms ? ?/sec 1.00 17.6±0.08ms ? ?/sec
arrow_reader_clickbench/sync/Q36 1.01 22.3±0.17ms ? ?/sec 1.00 22.2±0.12ms ? ?/sec
arrow_reader_clickbench/sync/Q37 1.01 6.7±0.02ms ? ?/sec 1.00 6.6±0.04ms ? ?/sec
arrow_reader_clickbench/sync/Q38 1.01 11.6±0.09ms ? ?/sec 1.00 11.5±0.06ms ? ?/sec
arrow_reader_clickbench/sync/Q39 1.02 21.2±0.07ms ? ?/sec 1.00 20.8±0.08ms ? ?/sec
arrow_reader_clickbench/sync/Q40 1.02 4.9±0.04ms ? ?/sec 1.00 4.8±0.03ms ? ?/sec
arrow_reader_clickbench/sync/Q41 1.00 5.5±0.04ms ? ?/sec 1.00 5.5±0.06ms ? ?/sec
arrow_reader_clickbench/sync/Q42 1.00 4.4±0.03ms ? ?/sec 1.00 4.4±0.03ms ? ?/sec
```
</p>
</details>
<details><summary>Resource Usage</summary>
**base (merge-base)**
| Metric | Value |
|--------|-------|
| Wall time | 790.2s |
| Peak memory | 286.4 MiB |
| Avg memory | 140.6 MiB |
| CPU user | 698.5s |
| CPU sys | 86.1s |
| Peak spill | 0 B |
**branch**
| Metric | Value |
|--------|-------|
| Wall time | 790.2s |
| Peak memory | 286.3 MiB |
| Avg memory | 140.5 MiB |
| CPU user | 700.3s |
| CPU sys | 84.9s |
| Peak spill | 0 B |
</details>
---
[File an issue](https://github.com/adriangb/datafusion-benchmarking/issues) against this benchmark runner
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "adriangbot (via GitHub)" <gi...@apache.org>.
adriangbot commented on PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#issuecomment-5449157705
🤖 Arrow criterion benchmark running (GKE) | [trigger](https://github.com/apache/arrow-rs/pull/10901#issuecomment-5449135497)
**Instance:** `c4a-highmem-16` (12 vCPU / 65 GiB) | `Linux bench-c5449135497-2032-bgr86 6.12.85+ #1 SMP Sat Jun 27 09:31:30 UTC 2026 aarch64 GNU/Linux`
<details><summary>CPU Details (lscpu)</summary>
```
Architecture: aarch64
CPU op-mode(s): 64-bit
Byte Order: Little Endian
CPU(s): 16
On-line CPU(s) list: 0-15
Vendor ID: ARM
Model name: Neoverse-V2
Model: 1
Thread(s) per core: 1
Core(s) per cluster: 16
Socket(s): -
Cluster(s): 1
Stepping: r0p1
BogoMIPS: 2000.00
Flags: fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 sm3 sm4 asimddp sha512 sve asimdfhm dit uscat ilrcpc flagm sb paca pacg dcpodp sve2 sveaes svepmull svebitperm svesha3 svesm4 flagm2 frint svei8mm svebf16 i8mm bf16 dgh rng bti
L1d cache: 1 MiB (16 instances)
L1i cache: 1 MiB (16 instances)
L2 cache: 32 MiB (16 instances)
L3 cache: 80 MiB (1 instance)
NUMA node(s): 1
NUMA node0 CPU(s): 0-15
Vulnerability Gather data sampling: Not affected
Vulnerability Indirect target selection: Not affected
Vulnerability Itlb multihit: Not affected
Vulnerability L1tf: Not affected
Vulnerability Mds: Not affected
Vulnerability Meltdown: Not affected
Vulnerability Mmio stale data: Not affected
Vulnerability Reg file data sampling: Not affected
Vulnerability Retbleed: Not affected
Vulnerability Spec rstack overflow: Not affected
Vulnerability Spec store bypass: Mitigation; Speculative Store Bypass disabled via prctl
Vulnerability Spectre v1: Mitigation; __user pointer sanitization
Vulnerability Spectre v2: Mitigation; CSV2, BHB
Vulnerability Srbds: Not affected
Vulnerability Tsa: Not affected
Vulnerability Tsx async abort: Not affected
Vulnerability Vmscape: Not affected
```
</details>
Comparing fix-cached-reader-quadratic-cleanup (e3f00206f62c863a267c71127803fe3d6cfc88f3) to 7e65400 (merge-base) [diff](https://github.com/apache/arrow-rs/compare/7e65400964976dc09d4d8415fbae60f296a15054..e3f00206f62c863a267c71127803fe3d6cfc88f3)
<details><summary>Run configuration</summary>
```yaml
run benchmark arrow_reader_clickbench
```
</details>
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_reader_clickbench
Results will be posted here when complete
---
[File an issue](https://github.com/adriangb/datafusion-benchmarking/issues) against this benchmark runner
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#discussion_r3939897944
##########
parquet/src/arrow/array_reader/cached_array_reader.rs:
##########
@@ -168,23 +172,34 @@ impl CachedArrayReader {
/// Remove batches from cache that have been completely consumed
/// This is only called for Consumer role readers
- fn cleanup_consumed_batches(&self) {
+ fn cleanup_consumed_batches(&mut self) {
let current_batch_id = self.get_batch_id_from_position(self.outer_position);
// Remove batches that are at least one batch behind the current position
// This ensures we don't remove batches that might still be needed for the current batch
// We can safely remove batch_id if current_batch_id > batch_id + 1
- if current_batch_id.val > 1 {
- let mut cache = self.shared_cache.write().unwrap();
- for batch_id_to_remove in 0..(current_batch_id.val - 1) {
- cache.remove(
- self.column_idx,
- BatchID {
- val: batch_id_to_remove,
- },
- );
- }
+ if current_batch_id.val <= 1 {
+ return;
+ }
+ let end = current_batch_id.val - 1;
+ // Everything below `cleaned_up_to` was removed by an earlier call.
+ // Rescanning from 0 each time made this quadratic in the number of
+ // batches in the row group, and took the shared cache's write lock on
+ // every `consume_batch` even when there was nothing left to remove.
+ if end <= self.cleaned_up_to {
+ return;
+ }
+ let mut cache = self.shared_cache.write().unwrap();
+ for batch_id_to_remove in self.cleaned_up_to..end {
+ cache.remove(
+ self.column_idx,
+ BatchID {
+ val: batch_id_to_remove,
+ },
+ );
}
+ drop(cache);
Review Comment:
No reason — `cleaned_up_to` is reader-local so it doesn't need the lock at all. Moved the store above the lock in 840d75db so the ordering question doesn't arise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "adriangbot (via GitHub)" <gi...@apache.org>.
adriangbot commented on PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#issuecomment-5449358884
🤖 Arrow criterion benchmark completed (GKE) | [trigger](https://github.com/apache/arrow-rs/pull/10901#issuecomment-5449135497)
**Instance:** `c4a-highmem-16` (12 vCPU / 65 GiB)
Comparing fix-cached-reader-quadratic-cleanup (e3f00206f62c863a267c71127803fe3d6cfc88f3) to 7e65400 (merge-base) [diff](https://github.com/apache/arrow-rs/compare/7e65400964976dc09d4d8415fbae60f296a15054..e3f00206f62c863a267c71127803fe3d6cfc88f3)
<details><summary>Run configuration</summary>
```yaml
run benchmark arrow_reader_clickbench
```
</details>
<details><summary>CPU Details (lscpu)</summary>
```
Architecture: aarch64
CPU op-mode(s): 64-bit
Byte Order: Little Endian
CPU(s): 16
On-line CPU(s) list: 0-15
Vendor ID: ARM
Model name: Neoverse-V2
Model: 1
Thread(s) per core: 1
Core(s) per cluster: 16
Socket(s): -
Cluster(s): 1
Stepping: r0p1
BogoMIPS: 2000.00
Flags: fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 sm3 sm4 asimddp sha512 sve asimdfhm dit uscat ilrcpc flagm sb paca pacg dcpodp sve2 sveaes svepmull svebitperm svesha3 svesm4 flagm2 frint svei8mm svebf16 i8mm bf16 dgh rng bti
L1d cache: 1 MiB (16 instances)
L1i cache: 1 MiB (16 instances)
L2 cache: 32 MiB (16 instances)
L3 cache: 80 MiB (1 instance)
NUMA node(s): 1
NUMA node0 CPU(s): 0-15
Vulnerability Gather data sampling: Not affected
Vulnerability Indirect target selection: Not affected
Vulnerability Itlb multihit: Not affected
Vulnerability L1tf: Not affected
Vulnerability Mds: Not affected
Vulnerability Meltdown: Not affected
Vulnerability Mmio stale data: Not affected
Vulnerability Reg file data sampling: Not affected
Vulnerability Retbleed: Not affected
Vulnerability Spec rstack overflow: Not affected
Vulnerability Spec store bypass: Mitigation; Speculative Store Bypass disabled via prctl
Vulnerability Spectre v1: Mitigation; __user pointer sanitization
Vulnerability Spectre v2: Mitigation; CSV2, BHB
Vulnerability Srbds: Not affected
Vulnerability Tsa: Not affected
Vulnerability Tsx async abort: Not affected
Vulnerability Vmscape: Not affected
```
</details>
<details><summary>Details</summary>
<p>
```
group fix-cached-reader-quadratic-cleanup main
----- ----------------------------------- ----
arrow_reader_clickbench/async/Q1 1.00 1086.5±4.73µs ? ?/sec 1.00 1088.6±5.65µs ? ?/sec
arrow_reader_clickbench/async/Q10 1.00 5.9±0.05ms ? ?/sec 1.00 5.9±0.04ms ? ?/sec
arrow_reader_clickbench/async/Q11 1.00 7.0±0.04ms ? ?/sec 1.00 7.0±0.06ms ? ?/sec
arrow_reader_clickbench/async/Q12 1.00 13.2±0.09ms ? ?/sec 1.00 13.2±0.08ms ? ?/sec
arrow_reader_clickbench/async/Q13 1.00 15.9±0.11ms ? ?/sec 1.01 16.0±0.16ms ? ?/sec
arrow_reader_clickbench/async/Q14 1.00 14.7±0.08ms ? ?/sec 1.00 14.8±0.07ms ? ?/sec
arrow_reader_clickbench/async/Q19 1.00 3.1±0.03ms ? ?/sec 1.01 3.1±0.03ms ? ?/sec
arrow_reader_clickbench/async/Q20 1.01 73.8±0.57ms ? ?/sec 1.00 73.2±0.37ms ? ?/sec
arrow_reader_clickbench/async/Q21 1.00 93.0±4.43ms ? ?/sec 1.02 95.2±0.53ms ? ?/sec
arrow_reader_clickbench/async/Q22 1.00 123.8±16.08ms ? ?/sec 1.11 136.8±6.65ms ? ?/sec
arrow_reader_clickbench/async/Q23 1.02 255.1±1.65ms ? ?/sec 1.00 251.3±1.24ms ? ?/sec
arrow_reader_clickbench/async/Q24 1.01 18.3±0.18ms ? ?/sec 1.00 18.0±0.11ms ? ?/sec
arrow_reader_clickbench/async/Q27 1.02 58.3±0.48ms ? ?/sec 1.00 57.3±0.41ms ? ?/sec
arrow_reader_clickbench/async/Q28 1.02 58.2±0.51ms ? ?/sec 1.00 57.1±0.31ms ? ?/sec
arrow_reader_clickbench/async/Q30 1.01 17.7±0.11ms ? ?/sec 1.00 17.5±0.10ms ? ?/sec
arrow_reader_clickbench/async/Q36 1.00 14.7±0.26ms ? ?/sec 1.02 15.0±0.34ms ? ?/sec
arrow_reader_clickbench/async/Q37 1.00 5.0±0.03ms ? ?/sec 1.00 5.0±0.03ms ? ?/sec
arrow_reader_clickbench/async/Q38 1.00 13.6±0.25ms ? ?/sec 1.00 13.6±0.30ms ? ?/sec
arrow_reader_clickbench/async/Q39 1.00 24.7±0.59ms ? ?/sec 1.00 24.8±0.44ms ? ?/sec
arrow_reader_clickbench/async/Q40 1.01 5.3±0.05ms ? ?/sec 1.00 5.3±0.03ms ? ?/sec
arrow_reader_clickbench/async/Q41 1.00 4.7±0.04ms ? ?/sec 1.00 4.7±0.02ms ? ?/sec
arrow_reader_clickbench/async/Q42 1.01 3.4±0.03ms ? ?/sec 1.00 3.4±0.02ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q1 1.01 1075.1±6.88µs ? ?/sec 1.00 1064.2±4.91µs ? ?/sec
arrow_reader_clickbench/async_object_store/Q10 1.00 5.8±0.03ms ? ?/sec 1.00 5.8±0.03ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q11 1.00 6.9±0.04ms ? ?/sec 1.00 6.8±0.04ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q12 1.01 13.2±0.12ms ? ?/sec 1.00 13.1±0.07ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q13 1.00 15.8±0.12ms ? ?/sec 1.00 15.8±0.11ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q14 1.01 14.8±0.11ms ? ?/sec 1.00 14.6±0.05ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q19 1.01 3.0±0.03ms ? ?/sec 1.00 2.9±0.02ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q20 1.01 72.9±0.65ms ? ?/sec 1.00 71.9±0.54ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q21 1.01 81.7±0.77ms ? ?/sec 1.00 80.7±0.53ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q22 1.00 100.0±0.81ms ? ?/sec 1.00 99.5±0.66ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q23 1.01 232.8±0.52ms ? ?/sec 1.00 230.6±0.79ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q24 1.00 17.9±0.14ms ? ?/sec 1.00 18.0±0.14ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q27 1.00 56.8±0.59ms ? ?/sec 1.00 56.7±0.53ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q28 1.01 56.7±0.73ms ? ?/sec 1.00 56.3±0.44ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q30 1.00 17.2±0.10ms ? ?/sec 1.00 17.1±0.08ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q36 1.00 14.2±0.24ms ? ?/sec 1.01 14.4±0.30ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q37 1.00 4.9±0.02ms ? ?/sec 1.00 4.9±0.04ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q38 1.01 12.9±0.24ms ? ?/sec 1.00 12.7±0.18ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q39 1.02 23.9±0.60ms ? ?/sec 1.00 23.5±0.39ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q40 1.00 5.1±0.05ms ? ?/sec 1.00 5.1±0.04ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q41 1.00 4.5±0.02ms ? ?/sec 1.00 4.5±0.03ms ? ?/sec
arrow_reader_clickbench/async_object_store/Q42 1.01 3.3±0.01ms ? ?/sec 1.00 3.2±0.01ms ? ?/sec
arrow_reader_clickbench/sync/Q1 1.00 918.4±3.14µs ? ?/sec 1.00 915.6±2.26µs ? ?/sec
arrow_reader_clickbench/sync/Q10 1.00 4.6±0.02ms ? ?/sec 1.01 4.6±0.04ms ? ?/sec
arrow_reader_clickbench/sync/Q11 1.00 5.6±0.02ms ? ?/sec 1.01 5.7±0.08ms ? ?/sec
arrow_reader_clickbench/sync/Q12 1.00 20.6±0.26ms ? ?/sec 1.01 20.7±0.09ms ? ?/sec
arrow_reader_clickbench/sync/Q13 1.00 23.7±0.49ms ? ?/sec 1.00 23.6±0.07ms ? ?/sec
arrow_reader_clickbench/sync/Q14 1.00 22.0±0.13ms ? ?/sec 1.00 22.1±0.08ms ? ?/sec
arrow_reader_clickbench/sync/Q19 1.01 2.7±0.03ms ? ?/sec 1.00 2.7±0.02ms ? ?/sec
arrow_reader_clickbench/sync/Q20 1.02 125.8±0.31ms ? ?/sec 1.00 123.3±0.24ms ? ?/sec
arrow_reader_clickbench/sync/Q21 1.02 99.8±0.19ms ? ?/sec 1.00 97.6±1.12ms ? ?/sec
arrow_reader_clickbench/sync/Q22 1.02 148.6±0.86ms ? ?/sec 1.00 145.9±0.37ms ? ?/sec
arrow_reader_clickbench/sync/Q23 1.01 306.0±10.44ms ? ?/sec 1.00 303.1±11.92ms ? ?/sec
arrow_reader_clickbench/sync/Q24 1.00 25.5±0.15ms ? ?/sec 1.01 25.8±0.12ms ? ?/sec
arrow_reader_clickbench/sync/Q27 1.01 109.6±0.31ms ? ?/sec 1.00 108.6±0.22ms ? ?/sec
arrow_reader_clickbench/sync/Q28 1.02 107.0±0.31ms ? ?/sec 1.00 105.4±0.24ms ? ?/sec
arrow_reader_clickbench/sync/Q30 1.00 17.4±0.11ms ? ?/sec 1.00 17.5±0.05ms ? ?/sec
arrow_reader_clickbench/sync/Q36 1.00 22.0±0.12ms ? ?/sec 1.02 22.4±0.08ms ? ?/sec
arrow_reader_clickbench/sync/Q37 1.00 6.7±0.02ms ? ?/sec 1.00 6.7±0.02ms ? ?/sec
arrow_reader_clickbench/sync/Q38 1.00 11.5±0.10ms ? ?/sec 1.00 11.4±0.06ms ? ?/sec
arrow_reader_clickbench/sync/Q39 1.01 20.8±0.18ms ? ?/sec 1.00 20.7±0.13ms ? ?/sec
arrow_reader_clickbench/sync/Q40 1.00 4.8±0.03ms ? ?/sec 1.00 4.8±0.02ms ? ?/sec
arrow_reader_clickbench/sync/Q41 1.00 5.4±0.03ms ? ?/sec 1.00 5.4±0.03ms ? ?/sec
arrow_reader_clickbench/sync/Q42 1.00 4.3±0.02ms ? ?/sec 1.00 4.3±0.03ms ? ?/sec
```
</p>
</details>
<details><summary>Resource Usage</summary>
**base (merge-base)**
| Metric | Value |
|--------|-------|
| Wall time | 790.2s |
| Peak memory | 286.5 MiB |
| Avg memory | 141.3 MiB |
| CPU user | 706.0s |
| CPU sys | 80.1s |
| Peak spill | 0 B |
**branch**
| Metric | Value |
|--------|-------|
| Wall time | 785.2s |
| Peak memory | 288.4 MiB |
| Avg memory | 136.7 MiB |
| CPU user | 699.6s |
| CPU sys | 81.3s |
| Peak spill | 0 B |
</details>
---
[File an issue](https://github.com/adriangb/datafusion-benchmarking/issues) against this benchmark runner
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#discussion_r3904849877
##########
parquet/src/arrow/array_reader/cached_array_reader.rs:
##########
@@ -87,6 +87,9 @@ pub struct CachedArrayReader {
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
Review Comment:
👍
##########
parquet/src/arrow/array_reader/cached_array_reader.rs:
##########
@@ -168,23 +172,34 @@ impl CachedArrayReader {
/// Remove batches from cache that have been completely consumed
/// This is only called for Consumer role readers
- fn cleanup_consumed_batches(&self) {
+ fn cleanup_consumed_batches(&mut self) {
let current_batch_id = self.get_batch_id_from_position(self.outer_position);
// Remove batches that are at least one batch behind the current position
// This ensures we don't remove batches that might still be needed for the current batch
// We can safely remove batch_id if current_batch_id > batch_id + 1
- if current_batch_id.val > 1 {
- let mut cache = self.shared_cache.write().unwrap();
- for batch_id_to_remove in 0..(current_batch_id.val - 1) {
- cache.remove(
- self.column_idx,
- BatchID {
- val: batch_id_to_remove,
- },
- );
- }
+ if current_batch_id.val <= 1 {
+ return;
+ }
+ let end = current_batch_id.val - 1;
+ // Everything below `cleaned_up_to` was removed by an earlier call.
+ // Rescanning from 0 each time made this quadratic in the number of
+ // batches in the row group, and took the shared cache's write lock on
+ // every `consume_batch` even when there was nothing left to remove.
+ if end <= self.cleaned_up_to {
+ return;
+ }
+ let mut cache = self.shared_cache.write().unwrap();
+ for batch_id_to_remove in self.cleaned_up_to..end {
+ cache.remove(
+ self.column_idx,
+ BatchID {
+ val: batch_id_to_remove,
+ },
+ );
}
+ drop(cache);
Review Comment:
Why drop the write lock before setting cleaned_up_to?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#discussion_r4119104400
##########
parquet/src/arrow/array_reader/cached_array_reader.rs:
##########
@@ -168,22 +172,33 @@ impl CachedArrayReader {
/// Remove batches from cache that have been completely consumed
/// This is only called for Consumer role readers
- fn cleanup_consumed_batches(&self) {
+ fn cleanup_consumed_batches(&mut self) {
let current_batch_id = self.get_batch_id_from_position(self.outer_position);
// Remove batches that are at least one batch behind the current position
// This ensures we don't remove batches that might still be needed for the current batch
// We can safely remove batch_id if current_batch_id > batch_id + 1
- if current_batch_id.val > 1 {
- let mut cache = self.shared_cache.write().unwrap();
- for batch_id_to_remove in 0..(current_batch_id.val - 1) {
- cache.remove(
- self.column_idx,
- BatchID {
- val: batch_id_to_remove,
- },
- );
- }
+ if current_batch_id.val <= 1 {
+ return;
+ }
+ let end = current_batch_id.val - 1;
Review Comment:
Left as is — this changes behaviour rather than simplifies. `end = current_batch_id.val` removes up to `current - 1`; the `-1` keeps it at `current - 2`, which is what `main` does today (`for batch_id_to_remove in 0..(current_batch_id.val - 1)`). This PR only makes that same range incremental.
Widening it asserts that no consumer can still ask for `current - 1`. Plausible — the `local_cache.retain` just above keeps `>= current` — but worth its own PR. Happy to open one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#issuecomment-5864529301
> Thanks -- looks good to me @zhuqi-lucas
>
> I left some comments, but I don't think any are blocking
Thanks @alamb for review, addressed review comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#issuecomment-5449135497
run benchmarks arrow_reader_clickbench
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#discussion_r4119103008
##########
parquet/src/arrow/array_reader/cached_array_reader.rs:
##########
@@ -168,22 +172,33 @@ impl CachedArrayReader {
/// Remove batches from cache that have been completely consumed
/// This is only called for Consumer role readers
- fn cleanup_consumed_batches(&self) {
+ fn cleanup_consumed_batches(&mut self) {
let current_batch_id = self.get_batch_id_from_position(self.outer_position);
Review Comment:
Added in c3e3a4ec5 — `debug_assert!(current_batch_id.val >= self.cleaned_up_to)`. That is what makes the incremental scan sound.
##########
parquet/src/arrow/array_reader/cached_array_reader.rs:
##########
@@ -168,23 +172,34 @@ impl CachedArrayReader {
/// Remove batches from cache that have been completely consumed
/// This is only called for Consumer role readers
- fn cleanup_consumed_batches(&self) {
+ fn cleanup_consumed_batches(&mut self) {
let current_batch_id = self.get_batch_id_from_position(self.outer_position);
// Remove batches that are at least one batch behind the current position
// This ensures we don't remove batches that might still be needed for the current batch
// We can safely remove batch_id if current_batch_id > batch_id + 1
- if current_batch_id.val > 1 {
- let mut cache = self.shared_cache.write().unwrap();
- for batch_id_to_remove in 0..(current_batch_id.val - 1) {
- cache.remove(
- self.column_idx,
- BatchID {
- val: batch_id_to_remove,
- },
- );
- }
+ if current_batch_id.val <= 1 {
+ return;
+ }
+ let end = current_batch_id.val - 1;
+ // Everything below `cleaned_up_to` was removed by an earlier call.
+ // Rescanning from 0 each time made this quadratic in the number of
+ // batches in the row group, and took the shared cache's write lock on
+ // every `consume_batch` even when there was nothing left to remove.
+ if end <= self.cleaned_up_to {
+ return;
+ }
+ let mut cache = self.shared_cache.write().unwrap();
+ for batch_id_to_remove in self.cleaned_up_to..end {
+ cache.remove(
+ self.column_idx,
+ BatchID {
+ val: batch_id_to_remove,
+ },
+ );
}
+ drop(cache);
Review Comment:
From the earlier round; the code has since moved. `cleaned_up_to` is now set before the lock is taken — it is per-reader state, so it needs no lock, and taking the lock last keeps the critical section to the removals.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#issuecomment-5871655610
Thanks -- looks good to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CachedArrayReader cache cleanup incremental instead of quadratic [arrow-rs]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #10901:
URL: https://github.com/apache/arrow-rs/pull/10901#discussion_r4119104400
##########
parquet/src/arrow/array_reader/cached_array_reader.rs:
##########
@@ -168,22 +172,33 @@ impl CachedArrayReader {
/// Remove batches from cache that have been completely consumed
/// This is only called for Consumer role readers
- fn cleanup_consumed_batches(&self) {
+ fn cleanup_consumed_batches(&mut self) {
let current_batch_id = self.get_batch_id_from_position(self.outer_position);
// Remove batches that are at least one batch behind the current position
// This ensures we don't remove batches that might still be needed for the current batch
// We can safely remove batch_id if current_batch_id > batch_id + 1
- if current_batch_id.val > 1 {
- let mut cache = self.shared_cache.write().unwrap();
- for batch_id_to_remove in 0..(current_batch_id.val - 1) {
- cache.remove(
- self.column_idx,
- BatchID {
- val: batch_id_to_remove,
- },
- );
- }
+ if current_batch_id.val <= 1 {
+ return;
+ }
+ let end = current_batch_id.val - 1;
Review Comment:
I left this one as is, because it would change behaviour rather than simplify. With `end = current_batch_id.val` the loop removes up to `current - 1`; the `-1` keeps it at `current - 2`, and that boundary is what `main` does today:
```rust
if current_batch_id.val > 1 {
for batch_id_to_remove in 0..(current_batch_id.val - 1) {
```
This PR only makes that same range incremental, so widening it by one batch is a separate claim — that no consumer can still ask for `current - 1`. That may well be true (the `local_cache.retain` just above keeps `>= current`, which points the same way), but it deserves its own PR and its own reasoning rather than riding along with a cleanup change. Happy to open one if you think the wider range is right.
##########
parquet/src/arrow/array_reader/cached_array_reader.rs:
##########
@@ -168,23 +172,34 @@ impl CachedArrayReader {
/// Remove batches from cache that have been completely consumed
/// This is only called for Consumer role readers
- fn cleanup_consumed_batches(&self) {
+ fn cleanup_consumed_batches(&mut self) {
let current_batch_id = self.get_batch_id_from_position(self.outer_position);
// Remove batches that are at least one batch behind the current position
// This ensures we don't remove batches that might still be needed for the current batch
// We can safely remove batch_id if current_batch_id > batch_id + 1
- if current_batch_id.val > 1 {
- let mut cache = self.shared_cache.write().unwrap();
- for batch_id_to_remove in 0..(current_batch_id.val - 1) {
- cache.remove(
- self.column_idx,
- BatchID {
- val: batch_id_to_remove,
- },
- );
- }
+ if current_batch_id.val <= 1 {
+ return;
+ }
+ let end = current_batch_id.val - 1;
+ // Everything below `cleaned_up_to` was removed by an earlier call.
+ // Rescanning from 0 each time made this quadratic in the number of
+ // batches in the row group, and took the shared cache's write lock on
+ // every `consume_batch` even when there was nothing left to remove.
+ if end <= self.cleaned_up_to {
+ return;
+ }
+ let mut cache = self.shared_cache.write().unwrap();
+ for batch_id_to_remove in self.cleaned_up_to..end {
+ cache.remove(
+ self.column_idx,
+ BatchID {
+ val: batch_id_to_remove,
+ },
+ );
}
+ drop(cache);
Review Comment:
This one is from the earlier round and the code has since moved — `cleaned_up_to` is now set *before* the write lock is taken:
```rust
let start = self.cleaned_up_to;
self.cleaned_up_to = end;
let mut cache = self.shared_cache.write().unwrap();
```
The watermark is per-reader state, not shared, so it does not need the lock; taking the lock last also keeps the critical section down to the removals themselves.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org