hhhizzz commented on PR #10443: URL: https://github.com/apache/arrow-rs/pull/10443#issuecomment-5088149777
Thanks for working on this optimization. I independently reproduced the substantial warm-cache improvement, and the cache-reuse approach looks effective for the intended `iterate`-then-consume path. 🚀 I did, however, find a clear and reproducible regression in the cold-cache `mask_consume/run64` case: the PR is approximately **11.4% slower than the base** on my test machine. The absolute difference is small—about **1.6 µs per conversion of a 300k-row mask**—so this may not be a serious real-world regression, and it does not invalidate the main improvement. However, the result is stable and contradicts the PR's statement that all cold-cache cases remain within 1.5% of `main`. I think it is worth understanding the cause before concluding that the cold path is unchanged. Could you please try reproducing this case in your environment? # Details ### Reproduction setup - base: `8e7043bf937b60e3be8586ceb3cd00349b989e1e` - PR head: `20eb12415e62f86cf31883cc637c09ed304a8896` - x86_64 Linux - Rust 1.96.1 - CPU pinned to one core - separate Cargo target directories for base and head - copied only the PR's `parquet/benches/row_selector.rs` to the base worktree, so both sides used the same benchmark harness - no implementation code was copied - alternating base/head execution order - no Criterion sample-time warnings First, I ran the complete `mask_` group three times per side: ```bash cargo bench -p parquet --bench row_selector -- mask_ ``` Criterion settings: ```text warm-up time: 1s measurement time: 2s sample size: 20 ``` The cold-cache geometric means were: | benchmark | base | PR | change | | --- | ---: | ---: | ---: | | `mask_consume/run01` | 638.630 µs | 639.072 µs | +0.07% | | `mask_consume/run04` | 167.478 µs | 167.519 µs | +0.02% | | `mask_consume/run16` | 48.721 µs | 48.971 µs | +0.51% | | `mask_consume/run64` | 14.391 µs | 16.071 µs | **+11.67%** | | `mask_consume/random` | 288.669 µs | 289.050 µs | +0.13% | I then isolated `mask_consume/run64` and ran five alternating repetitions per side with a longer measurement time: ```bash cargo bench -p parquet --bench row_selector -- 'mask_consume/run64' ``` Focused Criterion settings: ```text warm-up time: 1s measurement time: 3s sample size: 30 ``` Focused result: - base geometric mean: **14.427 µs** - PR geometric mean: **16.070 µs** - change: **+11.39%** The values were stable across all five repetitions. As a small diagnostic, adding `#[inline]` to `MaskSelection::into_selectors` did not remove the regression, so it does not appear to be caused solely by an additional non-inlined function call. The main warm-cache improvement was reproduced and remains substantial. My concern is specifically the unexpected cold `run64` behavior. It may be useful to test neighboring run lengths such as 32, 48, 96, and 128 to determine whether this is related to vector capacity, allocation behavior, or another boundary effect. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
