-
Notifications
You must be signed in to change notification settings - Fork 4.3k
GH-51268: [C++][Parquet] Avoid reloading DELTA_BINARY_PACKED decoder state for every value - #51249
Draft
prtkgaur wants to merge 1 commit into
Draft
GH-51268: [C++][Parquet] Avoid reloading DELTA_BINARY_PACKED decoder state for every value #51249prtkgaur wants to merge 1 commit into
prtkgaur wants to merge 1 commit into
Conversation
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?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
After updating the title, you can mark the pull request as ready for review.
See also:
...every value 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.28x wherever the running sum is a meaningful share of the work.
prtkgaur
force-pushed
the
delta-binary-packed-prefix-sum-locals
branch
from
September 9, 2026 03:06
ec830e8 to
74f8742
Compare
@prtkgaur
prtkgaur
changed the title
(削除) [C++][Parquet] Keep DELTA_BINARY_PACKED's running value out of memory (削除ここまで)
(追記) GH-51268: [C++][Parquet] Avoid reloading DELTA_BINARY_PACKED decoder state for every value (追記ここまで)
Sep 9, 2026
This was referenced Sep 9, 2026
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
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.
Uh oh!
There was an error while loading. Please reload this page.
Rationale for this change
The frame of reference and the running value are members of the same type as the 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 for every value decoded: 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, hold the running value and the frame in locals across the
loop and write the running value back once when the loop ends. The arithmetic is unchanged --
every term stays in the unsigned type, so the wrapping the format specifies is preserved and
no decoded value changes.
Are these changes tested?
By the existing tests: this is a compilation concern rather than a behavioural one, and
parquet-encoding-testalready round trips DELTA_BINARY_PACKED over both integer widths,including the overflow cases that exercise the wrapping.
Benchmark
Graviton4, GCC 11.5,
Release, one core, 9 repetitions, medians, 65,536 values, both pointsbuilt twice with the builds interleaved. The two builds agree within 0.6% on every arm except
Int64_Fixed, where one read 0.95x; that arm is the zero bit width path, which this diffcannot execute, so 1.00x is the figure to trust. (An earlier revision of this description
published the 0.95x from a single build.)
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.