Repository navigation
perf: Optimize appending out-of-line views in concat, BatchCoalescer - #11770
Open
neilconway wants to merge 3 commits into
Open
neilconway wants to merge 3 commits into
neilconway wants to merge 3 commits into
Conversation
neilconway
changed the title
concat, BatchCoalescerconcat, BatchCoalescer
neilconway
force-pushed
the
neilc/view-buffer-index-shift
branch
from
October 23, 2026 14:54
eae820b to
1c28b8e
Compare
neilconway
marked this pull request as ready for review
October 23, 2026 14:54
github-actions
Bot
added
arrow-core
Contributor
Author
|
run benchmark coalesce_kernels |
|
🤖 Benchmark starting (GKE) | trigger Target: coalesce_kernels Sharding factor: 1 (1 workers). Comparing neilc/view-buffer-index-shift (1c28b8e) to df4dc3e (merge-base) diff Run configurationrun benchmark coalesce_kernels
env:
BENCH_FILTER: '^(filter|take): .*view'
shards: 1
Results will be posted when all workers finish. File an issue against this benchmark runner |
Contributor
Author
|
run benchmark concatenate_kernel |
|
🤖 Benchmark starting (GKE) | trigger Target: concatenate_kernel Sharding factor: 1 (1 workers). Comparing neilc/view-buffer-index-shift (1c28b8e) to df4dc3e (merge-base) diff Run configurationrun benchmark concatenate_kernel
env:
BENCH_FILTER: utf8_view
shards: 1
Results will be posted when all workers finish. File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Target: concatenate_kernel Comparing neilc/view-buffer-index-shift (1c28b8e) to df4dc3e (merge-base) diff Run configurationrun benchmark concatenate_kernel
env:
BENCH_FILTER: utf8_view
shards: 1
DetailsPer-runner informationconcatenate_kernel — shard 1/1Node: gk3-benchmark-cluster-nap-yar4jndv-034ff9fd-wmds Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench concatenate_kernelCPU Details (lscpu)Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
mbutrovich
approved these changes
Oct 23, 2026
mbutrovich
left a comment
mbutrovich
left a comment
Contributor
There was a problem hiding this comment.
Thanks @neilconway! The codegen matches what we saw on #11656, and on an M5 Max with thin LTO the concat utf8_view cases with out-of-line strings are 9.5% to 11% faster, with and without code-alignment flags.
Contributor
Author
|
Thanks for the review, @mbutrovich ! |
The `Utf8` and `Dictionary(Int32, Utf8)` inputs of the `coalesce` benchmarks ignored their seed, so all 10 input batches of each held the same strings, and only the filters and indices changed from batch to batch. Each batch now draws its strings from its own seed, with the same distribution of lengths, nulls and characters as before. The other string and binary inputs already used the seed.
…lescer` When `BatchCoalescer` keeps an input's string view data buffers instead of copying its strings, it adds the number of buffers already in the output to the buffer index of each view longer than 12 bytes. It unpacked each such view into a `ByteView`, added the offset to the `buffer_index` field, and packed the fields back into a `u128`. Now it picks the offset or 0 from the view's length, shifts it to the position of the buffer index, bits 64 to 95, and adds it to the whole view. The increased index is less than the number of buffers in the output, so the sum never carries into bit 96 as long as that number fits in 32 bits. The coalescer checked the number before adding the input's buffers; it now checks it after, so the check covers every increased index. `coalesce_kernels` cases that take this path, each version run twice in alternating order. The `mixed_binaryview` cases without nulls that keep 80% of rows copy their strings instead, and are left out. - AMD EPYC Milan: all 26 cases are faster, by 0.4% to 11%. Filters that keep 80% of rows: -2.5% to -10%; `take` of 80%: -1.4% to -11%; filters that keep 1%: -0.4% to -4%. Strings of 13 to 39 bytes, all stored out of line: -2% to -7.8%. - Apple M4 Max: 25 cases are faster, by 0.3% to 15%. One is 1% slower: a filter that keeps 1% of `BinaryView` rows with nulls, whose two runs were 6% apart. Filters that keep 80%: -3.2% to -7.7%; `take` of 80%: -3.9% to -15%; filters that keep 1%: -6.9% to +1%. Strings of 13 to 39 bytes: -1.4% to -7.6%.
…yteViewBuilder::append_array` `GenericByteViewBuilder::append_array`, which `concat` uses for string and binary views, adds the number of data buffers already in the builder to the buffer index of each appended view longer than 12 bytes. It unpacked each such view into a `ByteView`, added the offset to the `buffer_index` field, and packed the fields back into a `u128`. Now it picks the offset or 0 from the view's length, shifts it to the position of the buffer index, bits 64 to 95, and adds it to the whole view. The increased index is less than the number of buffers in the builder, so the sum never carries into bit 96 as long as that number fits in 32 bits. `append_array` truncated the number to 32 bits without checking it; it now panics if the number doesn't fit, as the builder already does when it adds a single buffer. `concat` of 10 string view arrays of 8192 rows, each version run twice in alternating order: - AMD EPYC Milan: -34% to -35% when the arrays have out-of-line strings, whether all of their strings are (20 or 128 bytes) or about 40% are (3 to 19 bytes), with or without nulls. Arrays of inline strings take a path that doesn't change, and are within 0.2%. - Apple M4 Max: -11% to -12% for the same cases. Arrays of inline strings are within 0.4%.
neilconway
force-pushed
the
neilc/view-buffer-index-shift
branch
from
October 23, 2026 15:49
1c28b8e to
d666b34
Compare
|
🤖 Benchmark completed (GKE) | trigger Target: coalesce_kernels Comparing neilc/view-buffer-index-shift (1c28b8e) to df4dc3e (merge-base) diff Run configurationrun benchmark coalesce_kernels
env:
BENCH_FILTER: '^(filter|take): .*view'
shards: 1
DetailsPer-runner informationcoalesce_kernels — shard 1/1Node: gk3-benchmark-cluster-nap-144p8rlm-f3485326-9ndp Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench coalesce_kernelsCPU Details (lscpu)Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
Author
|
@Rich-T-kid Any interest in reviewing? |
Contributor
will take a look today |
Rich-T-kid
reviewed
Oct 25, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
turns out I took a brief glance at this before and forgot to submit the review
Comment on lines
+227
to
+241
| /// | ||
| /// # Panics | ||
| /// | ||
| /// Panics if the builder would hold more than `u32::MAX` data buffers | ||
| pub fn append_array(&mut self, array: &GenericByteViewArray<T>) { | ||
| self.flush_in_progress(); | ||
| // keep original views if this array is the first to be added or if there are no data buffers (all inline views) | ||
| let keep_views = self.completed.is_empty() || array.data_buffers().is_empty(); | ||
| let starting_buffer = self.completed.len() as u32; | ||
|
|
||
| self.completed.extend(array.data_buffers().iter().cloned()); | ||
| assert!( | ||
| u32::try_from(self.completed.len()).is_ok(), | ||
| "Too many blocks" | ||
| ); |
Contributor
There was a problem hiding this comment.
before this assertion what happened if the builder overflowed?
Contributor
Author
There was a problem hiding this comment.
On an overflow, the behavior before this PR was to panic in debug mode. In release mode, the buffer index would wrap and point at the wrong buffer.
Rich-T-kid
self-requested a review
October 26, 2026 02:49
Rich-T-kid
reviewed
Oct 26, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
this is quite a cool find, thanks @neilconway.
I am not sure if this qualifies as a breaking change. the API doesn't change but programs will behave very different with this change.
not sure if we need to wait for the next major release for this. cc @alamb
Comment on lines
+227
to
+230
| /// | ||
| /// # Panics | ||
| /// | ||
| /// Panics if the builder would hold more than `u32::MAX` data buffers |
Contributor
There was a problem hiding this comment.
🤔 this makes sense to do but it is a breaking change right? code the previously worked (produced incorrect results) ran but now this will cause a panic.
Contributor
Author
There was a problem hiding this comment.
I don't think this is a breaking change. Code that produces incorrect results should not be considered "code that previously worked" -- if that was the definition of a breaking change, many straightforward bugfixes would not be allowed in minor releases.
Additional considerations:
- In debug builds, the previous coding resulted in a panic anyway, so there is no change in behavior.
- In release builds, the previous behavior is actually worse than just incorrect results:
buffer_indexpoints at one buffer but the view'slengthandoffsetwere validated against a different buffer. Reader methods likeGenericByteViewArray::value(k)do an unchecked read somewhere off into heap memory, resulting in undefined behavior. - Overflow requires an array with
u32::MAXdata buffers, which is extremely unlikely to occur in practice.
Contributor
There was a problem hiding this comment.
Code that produces incorrect results should not be considered "code that previously worked" -- if that was the definition of a breaking change, many straightforward bugfixes would not be allowed in minor releases.
good point.
Rich-T-kid
approved these changes
Oct 26, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
makes sense to me, thanks @neilconway & @mbutrovich for the review.
This branch has not been deployed
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.
Which issue does this PR close?
Rationale for this change
When views are appended to an array that already has data buffers, the view of each out-of-line string must have its buffer index increased by the number of buffers already in the array. The obvious way to do this is to unpack the view (
u128->ByteView), update thebuffer_indexfield, and re-pack it. During the course of working on #11656, we found that it is significantly faster to instead update the corresponding bits in the raw view directly, via shifting and addition.This PR applies this optimization to two additional places:
GenericByteViewBuilder::append_array, whichconcatuses for string and binary view arrays, andInProgressByteViewArray::append_views_and_update_buffer_index, which is used byBatchCoalescer.What changes are included in this PR?
See above. We also add a sanity-check that the output buffer count fits in 32 bits (which will always be true in practice; the previous behavior of
GenericByteViewBuilder::append_arraywas to silently truncate the buffer count in this situation). The benchmark improvements forconcatandcoalesce_kernelslanded separately in #11768. This PR also passes the RNG seed to theUtf8andDictionary(Int32, Utf8)inputs ofcoalesce_kernels, which previously generated the same strings for every batch.Benchmarks
Are these changes tested?
Covered by existing tests.
Are there any significant user-facing changes?
Only in a corner case: if the output buffer count exceeds
u32::MAX, we now panic rather than silently truncating the count.