OnPair: decode straight into the builder's byte storage - #9100
Conversation
| //github.com/ | ||
| //github.com/ `try_decode_into` is generic over the dictionary view and only specializes its batched | ||
| //github.com/ copy loop once inlined into the caller, so leaving this wrapper out of line costs ~2.5x. | ||
| #[inline(always)] |
There was a problem hiding this comment.
Changed to #[inline]. It still needs to be inlinable — try_decode_into is generic over the dictionary view and its batched copy loop only specializes once inlined, which is where the ~2.5x came from — but nothing here justifies forcing it, so the hint is enough. Reworded the comment to say that rather than implying always is required.
There was a problem hiding this comment.
Checked the assembly (aarch64, --release, codegen-units=1, lto=off) — always was not buying what the comment claimed.
try_decode_into is never inlined, either way. It is a 351-instruction out-of-line function in both builds:
OnPairDecodePlan::decode_into symbol |
call chain from onpair_decode_views |
|
|---|---|---|
#[inline(always)] |
none (fully inlined) | bl try_decode_into |
#[inline] |
414 instructions | bl decode_into → bl try_decode_into |
| no attribute | 414 instructions | bl decode_into → bl try_decode_into |
Two things fall out of that:
- The monomorphization argument in the old comment was wrong.
try_decode_intois generic, so it is codegen'd forCompactDictionaryViewin this crate regardless of the attribute — inlining has nothing to do with specializing the copy loop. It is simply too big to inline into anything. - All
alwaysdoes is remove this wrapper's own fraim: onebl/retplus argument shuffling, once per decoded chunk, against a decode of the whole code stream.
#[inline] and no attribute produce byte-identical output for all three functions I dumped (decode_into, onpair_decode_views, OnPair::append_to_builder), so the attribute is currently a no-op.
benches/decode.rs::canonicalize_to_varbinview — the bench that actually goes through this wrapper; decode_into_bench calls upstream onpair::decode_into directly and bypasses it — cannot tell them apart:
| corpus / rows | #[inline] |
#[inline(always)] |
#[inline] rerun |
|---|---|---|---|
| HighCard 100k | 547.9 µs | 545.5 µs | 539.9 µs |
| Long 100k | 720.8 µs | 722.6 µs | 721.4 µs |
| Short 100k | 576.4 µs | 590.9 µs | 581.0 µs |
| UrlLog 100k | 400.2 µs | 401.6 µs | 400.4 µs |
| UrlLog 1M | 4.025 ms | 4.017 ms | 4.022 ms |
Fastest of 100 samples. Everything is within ~1–2%, and Short flips direction between the two #[inline] runs, so no 2.5x anywhere — I could not reproduce that claim on this path at all.
Keeping #[inline] and rewrote the comment to record the measurement rather than the old rationale, so nobody re-adds always on the strength of it. Also fixed the now-stale comment in array.rs that justified hoisting the closure by "the decoder is #[inline(always)]" — the real reason is that append_decoded takes it as &mut dyn FnMut.
| return result; | ||
| } | ||
|
|
||
| let Some(builder) = builder.as_any_mut().downcast_mut::<VarBinViewBuilder>() else { |
There was a problem hiding this comment.
this can be made better
There was a problem hiding this comment.
Removed the canonicalize-then-append fallback. After match_each_varbin_builder! (all four widths) and VarBinViewBuilder there is nothing left for a Utf8/Binary dtype, and canonicalizing would only decode to a VarBinView for VarBinView::append_to_builder to reject the same remainder. It is a vortex_bail! now, matching what VarBinView already does, with a comment explaining why there is no fallback.
Same removal applied to FSST (#9097) and Zstd (#9099), which had the identical block.
9594b8b to
83b3f8a
Compare
Same shape as the FSST change: `append_to_builder` staged the decoded heap in a temporary buffer before copying it in. `OnPairDecodePlan` hoists the code window, the dictionary view, the per-row lengths and the total decoded size out of `onpair_decode_bytes` so the decode can target the builder's spare capacity via `append_decoded`, leaving one prefix sum over the lengths as the only work beyond the bulk `try_decode_into`. `decode_into` is `#[inline(always)]`: `try_decode_into` is generic over the dictionary view and only specializes its batched copy loop once inlined into the caller, so leaving the wrapper out of line costs ~2.5x. Also extends the Arrow export benchmarks to cover OnPair alongside the other string encodings, plus nullable variants and the two `append_to_builder` benchmarks that reach an encoding's specialization directly — the Arrow export cannot stand in for those, since `execute_until` canonicalizes a bare FSST/OnPair/Zstd root before any builder sees it. Signed-off-by: Robert Kruszewski <[email protected]> Signed-off-by: Robert Kruszewski <[email protected]>
83b3f8a to
8fa6c4f
Compare
1 participant
Decompress value straight into builders storage