adriangb commented on PR #10549: URL: https://github.com/apache/arrow-rs/pull/10549#issuecomment-5183152739
## Benchmark results Short version: the scan itself is **~15x faster** on the data `DELTA_BYTE_ARRAY` exists for, but the **writer-level benchmark on `main` cannot measure that**, for a reason worth recording. ### The writer benchmark shows nothing, and here is why `bench_delta_byte_array_writers` (added in #10512) writes 128 x 2 MiB values against the default 1 MiB `data_page_size_limit`. That is exactly the #10489 regime: every value lands on its own page, the encoder's `previous` is cleared at each page boundary, so every `prefix_length` is 0 and the scan exits after a couple of bytes. Writing that bench's `large_string_shared_prefix` data on `main`: | encoding | `data_page_size_limit` | output | | --- | --- | --- | | `PLAIN` | default (1 MiB) | 256.02 MiB | | `DELTA_BYTE_ARRAY` | default (1 MiB) | 256.02 MiB | | `DELTA_BYTE_ARRAY` | 4 MiB | 2.00 MiB | Raw input is 256 MiB. At the default limit the `DELTA_BYTE_ARRAY` output is byte-for-byte what `PLAIN` produces. There is no deep prefix scan happening for this PR to speed up. Measured anyway, A/B/A with the baseline run twice to quantify machine drift, on aarch64: | bench | base (pre) | this PR | base (post) | | --- | --- | --- | --- | | `large_string_shared_prefix/plain` (control) | 64.95 ms | 65.22 ms | 62.51 ms | | `large_string_shared_prefix/delta_byte_array` | 70.16 ms | 72.26 ms | 69.36 ms | | `large_string_distinct/plain` (control) | 42.02 ms | 42.83 ms | 40.81 ms | | `large_string_distinct/delta_byte_array` | 50.50 ms | 50.08 ms | 52.38 ms | The `plain` rows are controls: `PLAIN` goes through `FallbackEncoderImpl::Plain` and never calls `common_prefix_length`, so it executes identical code on both sides. Baseline-to-baseline drift on those controls is 2.9-3.8%. `distinct/delta_byte_array` comes out faster than both baselines while `shared_prefix/delta_byte_array` comes out slower than both, which is the signature of noise rather than effect. **No conclusion should be drawn from this table** beyond "no regression large enough to escape the noise floor." (A first run without the A/B/A control reported 10-12% "regressions" on the `delta_byte_array` benches - but also a 11.0% "regression" on `shared_prefix/plain` and a 7.1% one on `distinct/plain`, both of which are untouched code. That was thermal drift across sequential runs, not a real effect.) ### Measuring the function directly `#[inline(never)]` on both implementations, `black_box` on inputs and outputs (without it LLVM hoists the pure call out of the timing loop and reports six-digit GB/s), aarch64, `-C target-cpu=native`: | shared prefix | byte-wise | block-wise (this PR) | speedup | | --- | --- | --- | --- | | 2 MiB, identical | 669.4 us | 44.8 us | **14.9x** | | 2 MiB, differ in last 8 bytes | 666.2 us | 46.1 us | 14.4x | | 64 KiB, identical | 17.5 us | 0.91 us | 19.2x | | 1 KiB, identical | 0.279 us | 0.015 us | 18.6x | | 64 B, identical | 0.018 us | 0.002 us | 9.0x | | none (mismatch at byte 0) | ~1 ns | ~2 ns | ~free | The byte-wise loop holds a flat 3.1-3.7 GB/s at every length, as a byte-at-a-time scan must; the block-wise version runs at 37-72 GB/s. The last row is the adversarial case: one 32-byte compare fails and the tail loop takes over, costing a nanosecond or so. ### What this means for review The honest justification for this PR is the microbenchmark, not the writer benchmark. The writer-level win only becomes visible once values larger than the page limit stop being cut into one-value pages - i.e. after #10505 - or if you raise `data_page_size_limit` above your value size today, as the 4 MiB row above shows. I would rather land this on its own merits as a self-contained, behavior-preserving change with differential tests against a naive reference, than have it inflate the diff of a correctness fix. -- 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]
