[C++][Parquet] Keep DELTA_BINARY_PACKED's running value out of memory - #51249
Draft
prtkgaur wants to merge 1 commit into
Draft
[C++][Parquet] Keep DELTA_BINARY_PACKED's running value out of memory#51249prtkgaur wants to merge 1 commit into
prtkgaur wants to merge 1 commit into
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.
|
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
min_delta_andlast_value_are members of the same type asGetInternal'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 the loop body is 8 instructions with 4 memory operations per value,
where 6 and 2 are enough.
What changes are included in this PR?
In the non-zero-bit-width branch of
DeltaBitPackDecoder<DType>::GetInternal, hold the runningvalue and the frame in locals across the loop and write
last_value_back once when the loopends. The arithmetic is unchanged -- every term stays in the unsigned type, so the documented
wrapping behaviour is preserved and no decoded value changes.
Are these changes tested?
By the existing tests: this is a compilation concern, not a behavioural one, and
parquet-encoding-testalready round trips DELTA_BINARY_PACKED over both integer widthsincluding the overflow cases that exercise the wrapping.
Benchmark
parquet-encoding-benchmark, the DELTA_BINARY_PACKED decode arms already in the tree. AWSGraviton4, GCC 11.5,
Release, pinned to one core, 9 repetitions, medians, 65,536 values; mainand this PR built and measured in the same sitting. The two
Fixedarms are thedelta_bit_width_ == 0fast path, which this diff cannot execute.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.)