adriangb commented on PR #10505: URL: https://github.com/apache/arrow-rs/pull/10505#issuecomment-5334900303
### Benchmark summary after rebase (`2d2d4ad` vs merge-base `7c306e4`) 11 runner jobs, all on `c4a-highmem-16` (Neoverse-V2): the delta-byte-array benches ×3, full `arrow_writer` ×3, `writer_overhead` ×3, plus one **main-vs-main control** of each of `arrow_writer` and `writer_overhead` (both sides pinned to `7c306e4`, so any delta the control reports is pure runner noise). Since the earlier rounds, the block-wise prefix scan (#10549) has merged into `main`, so this round isolates the page-floor fix on its own. #### The benches this PR is about — `^(small|medium|large)_string_(shared_prefix|partial_prefix|distinct)` Three ~8-minute runs ([1](https://github.com/apache/arrow-rs/pull/10505#issuecomment-5333764167), [2](https://github.com/apache/arrow-rs/pull/10505#issuecomment-5333764809), [3](https://github.com/apache/arrow-rs/pull/10505#issuecomment-5333764953)). Short jobs, tight numbers: everything not listed below is within ±2.5% in all three runs. | benchmark | base | branch | per-run delta | control | |---|---|---|---|---| | `large_string_shared_prefix/delta_byte_array` | 108.6 ms | 56.4 ms | −48.2 / −48.4 / −47.4 | −3.4 | | `large_string_shared_prefix_nullable_dense/delta_byte_array` | 54.3 ms | 28.3 ms | −47.9 / −48.1 / −47.5 | −3.4 | | `large_string_shared_prefix_nullable/delta_byte_array` | 70.1 ms | 47.1 ms | −32.3 / −33.5 / −32.8 | −1.7 | | `large_string_distinct/delta_byte_array` | 75.5 ms | 65.7 ms | −13.6 / −13.0 / −12.3 | −1.2 | | `large_string_shared_prefix/plain` (control, PLAIN untouched) | 94.3 ms | 94.5 ms | +0.8 / −0.2 / +0.2 | −4.1 | | `large_string_distinct/plain` (control) | 60.5 ms | 60.7 ms | −0.2 / +0.5 / +0.5 | −2.1 | | `large_string_shared_prefix_nullable_trailing/delta_byte_array` | 67.6 ms | 67.8 ms | +0.1 / −0.1 / +0.7 | −1.9 | | `small_string_*/delta_byte_array` (3 groups) | ~1 ms | ~1 ms | −2.5 … +1.2 | | | `small_string_distinct/plain` | 0.61 ms | 0.68 ms | +12.6 / +10.0 / +11.7 | −2.2 | Reading: - The wins are exactly where the fix applies and scale with how much dedup the page-flush was destroying: values sharing a 2 MiB prefix go from being re-encoded in full on every page to one full copy per page, so the encoder does half the work (**−48%**). The nullable variant gets **−33%** rather than −48% because of the known partial-dedup limitation with nulls (#10538 / #10554). `distinct` gets **−13%** even though there is no prefix to keep — the pages simply flush half as often. - `_trailing` is flat because its 8 trailing nulls make the page open with a multi-value mini-batch, so it misses the exemption today; that is the case #10554 targets. - PLAIN is flat on every 60–95 ms bench, as it should be — PLAIN does not take the new path. - `small_string_distinct/plain` is the one thing that reads as a regression here: +11% in all three filtered runs. It does **not** reproduce in the three full runs (−1.5 / −18.3 / −2.4, next to a −2.2 control) and the only PLAIN-reachable change is a `page_was_empty && values_to_write == 1` check per mini-batch plus a `saturating_sub(0)`. On a 0.6 ms benchmark I read it as code layout, not cost, but I am flagging it rather than hiding it. #### Full `arrow_writer` ×3 and the control Runs [1](https://github.com/apache/arrow-rs/pull/10505#issuecomment-5334764374), [2](https://github.com/apache/arrow-rs/pull/10505#issuecomment-5334778316), [3](https://github.com/apache/arrow-rs/pull/10505#issuecomment-5334863139); control [main-vs-main](https://github.com/apache/arrow-rs/pull/10505#issuecomment-5334878780). The delta-byte-array groups agree with the filtered runs (`shared_prefix` −49.6 / −60.4 / −47.4, `nullable_dense` −49.9 / −60.9 / −47.1, `nullable` −34.2 / −48.1 / −39.3, `distinct` −13.3 / −30.2 / −9.4). Beyond that, the ~1h45m jobs are too noisy to resolve small deltas, and the control shows it directly — **identical code on both sides** reports: | control (main vs main) | delta | |---|---| | `string/parquet_2` | +56.6% | | `list_primitive_non_null/default` | +22.0% | | `list_primitive_non_null/bloom_filter` | +11.4% | | `list_primitive/parquet_2` | +10.0% | | `string_non_null/default` | −13.9% | The same drift is visible inside the PR runs: run 2 has a blanket −15…−30% on unrelated groups (`decimal`, `fsb`, `small_string_*/plain`), and run 3 has `string_ree` at +28…+52% while runs 1–2 have it at −0.5…−5.6%. Everything CI reported as a regression on the earlier rounds (`string_ree`, `list_primitive_non_null`, `string_dictionary`) sits inside that envelope: none of them move the same direction in all three runs, and the control moves the same groups on identical code. There is also a systematic tilt worth naming so nobody reads it as a win: groups this PR cannot reach — `decimal` (−7…−25%), `fsb` (−1…−12%), `struct_non_null` / `struct_sparse_99pct_null` (−3…−17%), `primitive_sparse_99pct_null` (−1…−13%) — come out *faster* on the branch in all three runs while their control deltas are within ±1%. The runner benchmarks the baseline first and the branch second, so a baseline that ran while neighbouring jobs were still compiling shows up as a branch "improvement". It means the sign of a small delta on a long run can't be trusted in either direction, which is why the short filtered runs above are the numbers to lean on. #### `writer_overhead` ×3 and control Runs [1](https://github.com/apache/arrow-rs/pull/10505#issuecomment-5333796244), [2](https://github.com/apache/arrow-rs/pull/10505#issuecomment-5333797091), [3](https://github.com/apache/arrow-rs/pull/10505#issuecomment-5333808465); control [main-vs-main](https://github.com/apache/arrow-rs/pull/10505#issuecomment-5333848365). | benchmark | base | branch | per-run delta | control | |---|---|---|---|---| | `writer_overhead/1000_cols` | 3.70 ms | 3.77 ms | +2.7 / +0.0 / +2.7 | +0.0 | | `writer_overhead/5000_cols` | 23.8 ms | 19.5 ms | −17.7 / −18.9 / −18.0 | +0.8 | | `writer_overhead/10000_cols` | 40.5 ms | 40.8 ms | +0.2 / +1.2 / +0.5 | +8.0 | The −18% on `5000_cols` is reproducible (it also showed as −11% on the very first round) but has no mechanism in this PR: the bench writes one `Float32` per column, so the only new code it reaches is the `#[cold]` `set_page_size_floor` early-return. With 1000 and 10000 cols flat, and the control swinging 10000 cols by +8% on identical code, I'd treat it as layout luck rather than a claim. **Bottom line:** the fix delivers −48% / −33% / −13% on the DELTA_BYTE_ARRAY shapes it targets, PLAIN is flat, and nothing outside those groups moves in a way the main-vs-main control cannot also produce. -- 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]
