mbutrovich commented on code in PR #12177:
URL: https://github.com/apache/arrow-rs/pull/12177#discussion_r4550665399
##########
parquet/src/arrow/buffer/offset_buffer.rs:
##########
@@ -48,6 +48,9 @@ impl<I: OffsetSizeTrait> OffsetBuffer<I> {
/// Note: This does not verify that the entirety of `data` is valid
/// UTF-8. This should be done by calling [`Self::check_valid_utf8`] after
/// all data has been written
+ // Always inlined, as it is called for every value by the PLAIN and
+ // DELTA_BYTE_ARRAY decoders, and is otherwise not inlined into their loops
+ #[inline(always)]
Review Comment:
On Neoverse-V2, `StringArray/const prefix delta byte array encoded,
mandatory, no NULLs` is slower with this PR, in both the default and the
aligned bot runs. Speedup is merge-base time divided by PR time:
| Runner | Merge-base | This PR | Speedup | `delta length` (doesn't call
`try_push`) |
|---|---|---|---|---|
|
[Neoverse-V2](https://github.com/apache/arrow-rs/pull/12177#issuecomment-6816824906)
| 835.4 us | 900.5 us | 0.93x | 1.00x |
| [Neoverse-V2,
aligned](https://github.com/apache/arrow-rs/pull/12177#issuecomment-6823960506)
| 836.2 us | 896.8 us | 0.93x | 0.97x |
| [Xeon
8581C](https://github.com/apache/arrow-rs/pull/12177#issuecomment-6823982964) |
941.1 us | 836.6 us | 1.12x | 0.99x |
| [Xeon 8581C,
aligned](https://github.com/apache/arrow-rs/pull/12177#issuecomment-6823980420)
| 938.2 us | 921.5 us | 1.02x | 1.01x |
I compared the delta loop in `ByteArrayDecoder::read::<i32>` for
`aarch64-unknown-linux-gnu` on both sides. Each value does the same two copies
on both sides. The suffix is copied into `last_value`, then `last_value` is
copied into `values`. This PR removes the `bl try_push` between the two copies,
along with the reloads of its arguments from the stack and the check of the
returned `Result`:
```asm
; merge-base, after the suffix memcpy
ldr x1, [sp, #40] ; reload validate_utf8 and output from the stack
ldr w4, [sp, #36]
...
bl <parquet::arrow::buffer::offset_buffer::OffsetBuffer<i32>>::try_push
ldur x8, [x29, #-32] ; load the Result
cmn x8, #1
b.ne .LBB52_46
```
So the loop runs fewer instructions with this PR, and the slowdown comes
from how Neoverse-V2 runs the two copies back to back. In this benchmark each
value is 36 bytes with a 3-byte suffix, so both copies run. In `const delta
byte array encoded` the suffix is empty and the first copy is skipped. That
case is faster on every runner (1.03x to 1.39x).
The regression looks real, but it may be acceptable given the wins
elsewhere. PLAIN gains 1.08x to 1.42x on every runner. The arrow-rs writer
falls back to DELTA_BYTE_ARRAY only with `WriterVersion::PARQUET_2_0`
([`fallback_encoding`](https://github.com/apache/arrow-rs/blob/b2b7d4705191a200ac7e395596c0946786d6f69c/parquet/src/column/writer/mod.rs#L1981-L1990)),
and the default is `PARQUET_1_0`
([`DEFAULT_WRITER_VERSION`](https://github.com/apache/arrow-rs/blob/b2b7d4705191a200ac7e395596c0946786d6f69c/parquet/src/file/properties.rs#L22)),
so PLAIN is the common fallback.
--
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]
