[WIP][POC] Delta binary packed coalesce miniblocks - #51250
Draft
prtkgaur wants to merge 2 commits into
Draft
Conversation
min_delta_ and last_value_ have the same type as GetInternal's output buffer, and that buffer points into memory the caller owns, so the compiler cannot prove the prefix-sum store does not land on either member: it reloads the frame and stores the running value on every value. On aarch64 with GCC 11.5 the loop body is 8 instructions with 4 memory operations per value, where 6 and 2 are enough. Hold the running value and the frame in locals across the loop and write last_value_ back once when it ends. The arithmetic is unchanged - every term stays in the unsigned type, so the wrapping the existing comment documents is preserved and decoded values are identical. On the DELTA_BINARY_PACKED decode benchmarks already in the tree this is 1.26x to 1.29x wherever the running sum is a meaningful share of the work.
…ne call The miniblocks of a DELTA_BINARY_PACKED block are packed back to back with no padding between them, so a run of miniblocks that share a bit width is bit-identical to a single longer run at that width. GetInternal called the bit unpacker once per miniblock all the same, which with the default geometry is one call per 32 values - mostly per-call setup. Look ahead over the block's stored bit widths and extend the current call over each following miniblock that has the same width and that the caller has room for in full. A miniblock joins the run only when its width equals the current delta_bit_width_, which InitMiniBlock has already validated, and the run also stops at the end of the block. Add a test over the width patterns that decide where a run starts and stops, and read at a batch size that stops partway through a coalesced run. On the decode benchmarks already in the tree this is 1.17x to 1.33x on top of the previous commit wherever the unpacker's per-call cost is a meaningful share of the work. Decoded values are identical; no encoded byte changes.
prtkgaur
changed the title
|
Thanks for opening a pull request! This pull request has been automatically converted to a draft because its title doesn't match Arrow's required format. If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or After updating the title, you can mark the pull request as ready for review. See also: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
The miniblocks of a DELTA_BINARY_PACKED block are packed back to back with no padding between
them, so a run of miniblocks that share a bit width is bit-identical to a single longer run at
that width.
GetInternalcalls the bit unpacker once per miniblock all the same, which withthe default geometry is one call per 32 values -- at narrow widths, mostly per-call setup.
What changes are included in this PR?
A look-ahead over the block's stored bit widths, in a small private helper, reports how many of
the following miniblocks may be folded into the current unpack call, so a run of four asks the
unpacker for 128 values instead of 32. A miniblock joins the run only when its stored width
equals the current
delta_bit_width_, whichInitMiniBlockhas already validated, and therun also stops at the end of the block and at what the caller has room for.
Are these changes tested?
A new typed test covers the width patterns that decide where a run starts and stops, and the
fixture's read batch sizes gain one that stops partway through a coalesced run, so the existing
round trips cover it too. Three mutations of the implementation -- ignoring the neighbour's
width, ignoring the caller's room, and failing to advance the block cursor -- each turn the new
test red.
Benchmark
Same setup as the previous PR: AWS Graviton4, GCC 11.5,
Release, one core, 9 repetitions,medians, 65,536 values, both arms built and measured in one sitting. The two
Fixedarms arethe
delta_bit_width_ == 0path, which has no unpack call to coalesce.Decode_Int32_NarrowDecode_Int32_WideDecode_Int64_NarrowDecode_Int64_WideDecode_Int32_FixedDecode_Int64_FixedAre there any user-facing changes?
No. No API change, no format change, and decoded values are identical.
This PR includes breaking changes to public APIs. (If there are any breaking changes to public APIs, please explain which changes are breaking. If not, you can remove this.)
This PR contains a "Critical Fix". (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)