adriangb commented on PR #10554: URL: https://github.com/apache/arrow-rs/pull/10554#issuecomment-5339165026
### Full benchmark run after rebase (`6584407`), vs `main` and vs #10505, with controls 24 runner jobs on `c4a-highmem-16`: the delta-byte-array benches (`BENCH_FILTER: ^(small|medium|large)_string_(shared_prefix|partial_prefix|distinct)`), full `arrow_writer`, and `writer_overhead`, each ×3 against `main` (merge-base `7c306e4`) and ×3 against #10505's head (`2d2d4ad`, `baseline.ref`), plus a main-vs-main and a 10505-vs-10505 control of each. Everything below is the mean of three runs with the per-run deltas shown; a control column shows what identical code reports on the same job type. #### The benches this PR is about vs main: [1](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5335841634) [2](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5335842239) [3](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5335842591) · vs #10505: [1](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5335934628) [2](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5335954268) [3](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5335967151) · controls: [main](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5336619058) [10505](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5337368193) | benchmark | vs main (per run) | mean | vs #10505 (per run) | mean | ctrl main | ctrl 10505 | |---|---|---|---|---|---|---| | `large_string_shared_prefix_nullable_trailing/delta_byte_array` | −22.5 / −23.3 / −21.8 | **−22.5%** | −27.1 / −23.4 / −24.1 | **−24.9%** | −1.9 | +10.7 | | `large_string_shared_prefix_nullable/delta_byte_array` | −22.2 / −26.5 / −26.5 | **−25.1%** | +26.1 / +13.2 / +15.6 | **+18.3%** | −1.7 | −7.4 | | `large_string_distinct_nullable/delta_byte_array` | +12.5 / +11.3 / +13.3 | **+12.4%** | +13.2 / +13.7 / +17.2 | **+14.7%** | −0.9 | −0.9 | | `large_string_shared_prefix_nullable_dense/delta_byte_array` | −47.0 / −48.7 / −48.6 | −48.1% | +8.3 / −0.3 / −2.1 | +2.0% | −3.4 | +1.6 | | `medium_string_shared_prefix_nullable/delta_byte_array` | +0.3 / −3.0 / +5.7 | +1.0% | +1.8 / +4.4 / +2.7 | +3.0% | −4.7 | +2.6 | | `large_string_shared_prefix/delta_byte_array` (non-null) | −48.3 / −48.8 / −48.2 | −48.4% | +4.0 / +0.2 / −1.6 | +0.9% | −3.4 | −1.3 | | `large_string_distinct/delta_byte_array` (non-null) | −15.3 / −15.4 / −13.8 | −14.9% | −0.7 / −3.1 / −5.1 | −3.0% | −1.2 | +1.5 | | `large_string_shared_prefix_list/delta_byte_array` | +0.2 / −0.9 / −1.1 | −0.6% | +6.7 / −2.4 / +5.9 | +3.4% | +0.0 | +0.6 | | `large_string_*/plain` (3 groups) | −0.5 … −3.9 | | −1.1 … +2.8 | | | | | `small_string_*/delta_byte_array` (3 groups) | −1.9 … −0.4 | | −1.5 … +1.7 | | | | | `small_string_distinct/plain` (0.6 ms) | +3.3 / +7.0 / +4.0 | +4.8% | +11.4 / +15.2 / +13.4 | +13.3% | +6.7 | +14.1 | Reading: - **`_trailing` is the win this PR exists for**: −25% in all six comparisons against controls of ±2%, on top of the file going 120 MiB → 2 MiB. #10505 alone leaves this shape flat (its own runs today: +0.1 / −0.1 / +0.7), so it is entirely this PR. - **`_nullable` nets −25% vs main but costs +6…+18% relative to #10505**, for the 16.8 MB → 2.1 MB size reduction. The three vs-10505 runs agree on sign; the 10505 control moved this bench −7.4% on identical code, so the magnitude is loose. - **`distinct_nullable` is the one real regression**: +12% vs main, +15% vs #10505, in all six runs, with both controls at −0.9%. No size reduction to show for it (values share no prefix). This is the premium noted in the description — and it now has a cause and a fix, see below. - Everything else in the filtered set is inside its own control's spread. `small_string_distinct/plain` moves +13% vs #10505 and +5% vs main, but the 10505-vs-10505 control moves it **+14.1% on identical code** — a 0.6 ms benchmark that is bimodal on this runner. #### Where the regression comes from, and #10745 Profiled locally (`samply`, #10505 head vs this head, `large_string_distinct_nullable/delta_byte_array` and `..._shared_prefix_nullable/delta_byte_array`). The whole shift is `memmove` under `ByteArrayEncoder::write_gather` (+4.8 and +13.1 points of write-path samples respectively); the prefix-scan `memcmp` share is unchanged, and per-value encoding work is identical between the two — so the extra cost had to be per-*mini-batch*, and this PR roughly doubles the mini-batch count on a nullable column. It is `compute_min_max` in `arrow_writer/byte_array.rs`: it copies **both** the min and the max of every mini-batch into fresh `ByteArray`s (`to_vec()`) before `encode` checks whether either beats the running page min/max. With 2 MiB values and one value per mini-batch that is 4 MiB of alloc + copy per value, the same order as the encoding itself. Fixed in **#10745** by comparing on borrowed slices and copying only when the running value changes. Measured stacked on this PR (runner, filtered, three runs each way, mean shown; details and per-run numbers on that PR): | benchmark | this PR vs #10505 | this PR + #10745 vs this PR | this PR + #10745 vs #10505 | |---|---|---|---| | `large_string_distinct_nullable/delta_byte_array` | +14.7% | −5.4% | **+5.4%** (+1.8 / +5.1 / +9.4) | | `large_string_shared_prefix_nullable/delta_byte_array` | +18.3% | −24.4% | **−13.6%** | | `large_string_shared_prefix_nullable_dense/delta_byte_array` | +2.0% | −22.0% | **−24.2%** | | `large_string_shared_prefix_nullable_trailing/delta_byte_array` | −24.9% | −23.2% | **−41.9%** | | `medium_string_shared_prefix_nullable/delta_byte_array` | +3.0% | −7.2% | −8.0% | | `large_string_shared_prefix/delta_byte_array` (non-null) | +0.9% | −21.9% | −24.4% | #10745 is independent of both PRs and a strict no-loss change on `main` (it only ever removes copies); on `main` it cannot show a win on the *over-limit* DBA benches, because there every such value already opens its own page (the bug #10505 fixes) and the running min/max is taken on every mini-batch regardless. Proposed order: #10505 → #10745 → this PR; this branch is restacked accordingly. #### Full `arrow_writer` ×3 each way, and `writer_overhead` Full runs: vs main [1](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5336540403) [2](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5336547108) [3](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5336600179), vs #10505 [1](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5336638077) [2](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5336647342) [3](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5336657796), controls [main](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5337266457) [10505](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5338100896). Across the ~200 other benchmarks, the only two that move the same direction in all six comparisons are the two above (`distinct_nullable` +9.9% / +14.6%, `_trailing` −24.9% / −23.9%). The rest is inside what the controls produce on identical code: main-vs-main reports `string_non_null/default` −12.0%, `string/zstd_parquet_2` −13.1%, `string_non_null/cdc` +10.6%; 10505-vs-10505 reports `string_ree/cdc` +14.1%, `string/bloom_filter` +13.0%. `large_string_non_null/*` reads −5…−9% vs #10505 in all three runs, but the 10505 control shows the same −5.5% on identical code, and it is +0.6% vs main — baseline-first tilt, not the PR. `writer_overhead` (vs main [1](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5335864670) [2](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5335864710) [3](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5335880060), vs #10505 [1](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5336568555) [2](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5336568866) [3](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5336579747), controls [main](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5337396118) [10505](https://github.com/apache/arrow-rs/pull/10554#issuecomment-5337391258)): 1000 and 10000 cols flat (0 … −2.3%). `5000_cols` reads −20% in all six PR runs — and **−12.3% in the 10505-vs-10505 control on identical code**, so it is a bimodal benchmark on this runner, not this PR (it did the same on #10505). **Bottom line:** one intended win (`_trailing` −25%, 120 MiB → 2 MiB), one net win with a premium (`_nullable` −25% vs main, +6…18% vs #10505, 8× smaller file), one real regression (`distinct_nullable` +12…15%, no size change) whose cause is a per-mini-batch min/max copy that #10745 removes, taking it to +5% and turning the others into further wins. Nothing else in the suite moves outside what identical code moves. -- 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]
