adriangb commented on PR #10505: URL: https://github.com/apache/arrow-rs/pull/10505#issuecomment-5174076622
### Local benchmark results (Apple M4 Pro, aarch64) Since the CI runner has been noisy on this PR, I ran the same benchmarks locally as **base (`f8a57f8`) → branch (`6190b5a`) → base again**. The branch pass is bracketed in time so drift cancels, and comparing the two base passes against each other gives a per-benchmark noise floor to judge deltas against. 40 benchmarks, covering every group CI flagged plus controls. **Real changes** — delta far outside that benchmark's own noise floor: | benchmark | base | branch | delta | noise floor | |---|---|---|---|---| | `large_string_shared_prefix/delta_byte_array` | 64.4 ms | 25.8 ms | **−60.0%** | 0.3% | | `string_ree/parquet_2` | 86.1 ms | 45.0 ms | **−47.8%** | 1.4% | | `string_ree/zstd_parquet_2` | 94.4 ms | 53.1 ms | **−43.7%** | 0.9% | | `large_string_distinct/delta_byte_array` | 43.9 ms | 26.2 ms | **−40.4%** | 0.1% | The `string_ree` numbers were not something I expected, and they are worth calling out: **the block-wise prefix scan helps ordinary data, not just the synthetic 2 MiB values.** Run-end-encoded string columns expand to runs of *identical* consecutive values, so every value's shared-prefix scan runs the full length of the value — the same pathology, at normal string sizes. Splitting the two commits apart confirms where it comes from: | | `string_ree/parquet_2` | `string_ree/zstd_parquet_2` | |---|---|---| | page-size fix alone | −2.3% | −1.3% | | \+ block-wise prefix scan | **−47.8%** | **−43.7%** | **Everything else is flat.** Of the remaining 36 benchmarks, 33 land within ±2%, and the three that don't (`bool/default` +4.2%, `bool/zstd` +4.4%, `bool/bloom_filter` +3.8%) have base-vs-base noise floors of 4–5% — they are 6–9 ms benchmarks and simply do not resolve at this magnitude. **The regressions CI reported do not reproduce.** Specifically: | benchmark | CI | local (vs 3-pass noise floor) | |---|---|---| | `list_primitive_non_null/parquet_2` | +12.9% then +29.3% | **+0.0%** (noise 0.9%) | | `string_dictionary/parquet_2` | +34.9% then −0.2% | **+2.0%** (noise 2.1%) | | `string_ree/bloom_filter` | +49.4% | **+0.6%** (noise 2.0%) | | `string_ree/zstd` | +38.2% | **+0.8%** (noise 1.2%) | | `string_ree/default` | +12.1% | **+0.2%** (noise 1.8%) | Worth noting that `string_ree/parquet_2` and `string_ree/zstd_parquet_2` — which CI reported as *regressions* in one round — are in fact the second- and third-largest *improvements* in the suite. <details> <summary>Profile: where the time actually went</summary> On `large_string_shared_prefix/delta_byte_array` before the prefix-scan change, **76% of total runtime** was a single self-loop inside `ByteArrayEncoder::write_gather` — the byte-at-a-time shared-prefix scan. It cost nothing before the page-size fix only because flushing a page after every value cleared `last_value`, so the scan always exited at 0 and the encoding silently degenerated to `PLAIN`. Making the encoding work is what exposed the scan. After the change that loop is gone from the profile, and the benchmark is 2.5× faster than it was before either commit. </details> <details> <summary>Why 32-byte blocks</summary> It is the widest block that both aarch64 and x86-64 still expand inline: at 64 bytes x86-64 (baseline SSE2) drops to an out-of-line `bcmp` call, which costs more than the extra width buys. On aarch64 every width from 16 up measures the same (~15× a byte-wise scan over a 2 MiB shared prefix), so 32 sits in the middle of a flat optimum rather than on a tuned peak. Going wider also hurts short prefixes badly — at 128 bytes, a 64-byte prefix falls back to the byte loop entirely and is as slow as no optimization at all. </details> -- 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]
