Repository navigation
perf: pre-size collections and reuse per-iteration buffers - #26192
Merged
Merged
Conversation
Use `with_capacity` where the final size is known or tightly bounded by in-memory data, and hoist temporaries that were allocated on every row, batch, partition or column out of their loops so they are reused.
github-actions
Bot
added
sql
Contributor
Author
|
The PR has many LOC changed, but in fact it is the same pattern to use bounded datastructure where possible instead of unbounded |
Contributor
Author
|
run benchmarks tpch tpcds |
comphead
marked this pull request as ready for review
October 10, 2026 19:41
Contributor
Author
|
run benchmarks tpch tpcds |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #26192 +/- ##
==========================================
- Coverage 82.83% 82.83% -0.01%
==========================================
Files 1149 1149
Lines 452344 452467 +123
Branches 452344 452467 +123
==========================================
+ Hits 374680 374781 +101
- Misses 54864 54883 +19
- Partials 22800 22803 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing alloc-opt (a511f02) to 3310fd5 (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing alloc-opt (a511f02) to 3310fd5 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing alloc-opt (a511f02) to 3310fd5 (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing alloc-opt (a511f02) to 3310fd5 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing alloc-opt (a511f02) to 3310fd5 (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing alloc-opt (a511f02) to 3310fd5 (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing alloc-opt (a511f02) to 3310fd5 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing alloc-opt (a511f02) to 3310fd5 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
kosiew
approved these changes
Oct 12, 2026
kosiew
left a comment
kosiew
left a comment
Contributor
There was a problem hiding this comment.
Thanks for working on these allocation optimizations. I reviewed the buffer ownership, scratch-state reset behavior, and loop-hoisting changes and did not find any blocking correctness issues.
The focused validation also passed 3,221 tests. I have one optional suggestion for additional array-slicing coverage, but it is not a merge blocker.
| let mut offsets = vec![O::usize_as(0)]; | ||
| let mut offsets = Vec::with_capacity(array.len() + 1); | ||
| offsets.push(O::usize_as(0)); | ||
| let mut indices = Vec::new(); |
Contributor
There was a problem hiding this comment.
Could we add a test that mixes indexed slicing, contiguous slicing, empty and null rows, and negative strides within the same batch, for both List and ListView? This would help verify that reusing the indices buffer works correctly when switching between slicing modes, even though I haven't found a correctness issue.
Contributor
Author
There was a problem hiding this comment.
Thanks, added in b67eecd. The new array_slice.slt case mixes strided, contiguous, empty, NULL and negative-stride rows in one batch for both List and ListView. It fails if the indices.clear() calls are removed.
jayzhan211
added a commit
to jayzhan211/datafusion
that referenced
this pull request
Oct 14, 2026
14 tasks
bvolpato
pushed a commit
to bvolpato/datafusion
that referenced
this pull request
Oct 16, 2026
## Which issue does this PR close? - N/A ## Rationale for this change For input arrays with at least one non-ASCII string, apache#23588 changed `upper` on `Utf8` and `LargeUtf8` to convert each value one character at a time, writing straight into the output instead of calling `str::to_uppercase`, to avoid allocating a `String` per row. apache#26192 did the same for `Utf8View`. However, `str::to_uppercase` has a fast-path for inputs with leading ASCII bytes, so inputs with a mix of ASCII and non-ASCII characters got slower: for example, 128-byte ASCII `Utf8View` values with one non-ASCII value in the array became 19x slower. This PR reverts that change, but in the course of doing so, we also add several related optimizations to `upper`, `lower`, and the optimized string builders; details below. Benchmarks: (x86, AMD EPYC Milan) upper - Some non-ASCII rows: - Utf8View, 4096 rows: 952.7 µs → 254.5 µs (−73.3%) - Utf8View, 4096 rows, 10% nulls: 881.7 µs → 270.6 µs (−69.3%) - Utf8View, 8192 rows: 1.94 ms → 539.7 µs (−72.2%) - Utf8View, 8192 rows, 10% nulls: 1.79 ms → 559.4 µs (−68.7%) - Non-ASCII words: - Utf8: 825.5 µs → 726.6 µs (−12.0%) - LargeUtf8: 824.7 µs → 726.3 µs (−11.9%) - Utf8View: 759.5 µs → 727.6 µs (−4.2%) - Mostly ASCII: - Utf8, len 8: 204.0 µs → 19.6 µs (−90.4%) - Utf8View, len 8: 214.6 µs → 36.5 µs (−83.0%) - Utf8, len 32: 789.6 µs → 23.2 µs (−97.1%) - Utf8View, len 32: 757.4 µs → 36.2 µs (−95.2%) - Utf8, len 128: 3.13 ms → 51.7 µs (−98.3%) - Utf8View, len 128: 2.89 ms → 47.5 µs (−98.4%) - Late non-ASCII: - Utf8, len 8: 268.2 µs → 154.1 µs (−42.5%) - Utf8View, len 8: 259.4 µs → 156.5 µs (−39.7%) - Utf8, len 32: 881.8 µs → 136.4 µs (−84.5%) - Utf8View, len 32: 798.5 µs → 138.1 µs (−82.7%) - Utf8, len 128: 3.20 ms → 164.5 µs (−94.9%) - Utf8View, len 128: 2.95 ms → 163.9 µs (−94.4%) lower - Some non-ASCII rows: - Utf8View, 4096 rows: 270.3 µs → 232.8 µs (−13.9%) - Utf8View, 4096 rows, 10% nulls: 271.0 µs → 235.8 µs (−13.0%) - Utf8View, 8192 rows: 568.8 µs → 495.8 µs (−12.8%) - Utf8View, 8192 rows, 10% nulls: 572.5 µs → 502.4 µs (−12.3%) - Non-ASCII words: - Utf8: 514.2 µs → 511.0 µs (−0.6%) - LargeUtf8: 507.8 µs → 516.4 µs (+1.7%) - Utf8View: 510.2 µs → 509.4 µs (−0.2%) - Mostly ASCII: - Utf8, len 8: 67.8 µs → 18.6 µs (−72.5%) - Utf8View, len 8: 73.9 µs → 36.5 µs (−50.7%) - Utf8, len 32: 60.5 µs → 22.9 µs (−62.2%) - Utf8View, len 32: 68.4 µs → 36.5 µs (−46.6%) - Utf8, len 128: 76.7 µs → 52.1 µs (−32.1%) - Utf8View, len 128: 82.6 µs → 46.8 µs (−43.3%) - Late non-ASCII: - Utf8, len 8: 136.9 µs → 138.8 µs (+1.4%) - Utf8View, len 8: 140.0 µs → 139.8 µs (−0.1%) - Utf8, len 32: 115.3 µs → 124.8 µs (+8.2%) - Utf8View, len 32: 117.4 µs → 125.5 µs (+6.9%) - Utf8, len 128: 138.9 µs → 150.0 µs (+8.0%) - Utf8View, len 128: 142.4 µs → 152.3 µs (+7.0%) translate (ASCII scalar from/to, no deletions) - Utf8: - 1024 rows, len 8: 10.1 µs → 9.1 µs (−9.8%) - 1024 rows, len 32: 17.9 µs → 15.9 µs (−11.2%) - 1024 rows, len 128: 48.8 µs → 39.2 µs (−19.6%) - 1024 rows, len 1024: 363.8 µs → 270.7 µs (−25.6%) - 4096 rows, len 8: 37.5 µs → 33.7 µs (−10.1%) - 4096 rows, len 32: 68.6 µs → 62.8 µs (−8.5%) - 4096 rows, len 128: 195.8 µs → 154.6 µs (−21.1%) - 4096 rows, len 1024: 1.42 ms → 1.07 ms (−24.7%) - Utf8View: - 1024 rows, len 8: 9.1 µs → 8.4 µs (−8.2%) - 1024 rows, len 32: 14.5 µs → 13.8 µs (−4.6%) - 1024 rows, len 128: 36.2 µs → 35.0 µs (−3.3%) - 1024 rows, len 1024: 236.2 µs → 235.4 µs (−0.3%) - 4096 rows, len 8: 34.4 µs → 31.1 µs (−9.5%) - 4096 rows, len 32: 55.4 µs → 52.5 µs (−5.2%) - 4096 rows, len 128: 142.2 µs → 137.0 µs (−3.7%) - 4096 rows, len 1024: 937.7 µs → 940.8 µs (+0.3%) replace (single ASCII byte, Utf8) - 1024 rows, len 32: 26.2 µs → 9.0 µs (−65.6%) - 1024 rows, len 128: 92.6 µs → 16.4 µs (−82.3%) - 1024 rows, len 32, 20% nulls: 22.8 µs → 9.1 µs (−60.3%) - 1024 rows, len 128, 20% nulls: 76.5 µs → 14.6 µs (−80.9%) - 4096 rows, len 32: 103.2 µs → 31.2 µs (−69.7%) - 4096 rows, len 128: 372.6 µs → 61.9 µs (−83.4%) - 4096 rows, len 32, 20% nulls: 89.9 µs → 33.5 µs (−62.7%) - 4096 rows, len 128, 20% nulls: 316.5 µs → 56.8 µs (−82.1%) ## What changes are included in this PR? * Add `StringViewArrayBuilder::try_append_byte_map`, a fallible variant of the existing `append_byte_map`. (We need this to avoid regressing error handling behavior on overflow when we use this method below.) * `lower`, `upper`, for non-ASCII input arrays: we now check if each row is ASCII. If it is, we construct the result value directly via the string builder's `try_append_byte_map` method, which vectorizes. Non-ASCII rows are converted via `std::to_uppercase / std::to_lowercase`. * Make `GenericStringArrayBuilder::try_append_byte_map` copy the bytes into the value buffer and then map them in place, instead of calling Arrow's `MutableBuffer::extend` with a mapping iterator. The compiler turns the in-place loop into vector code, while the `extend` loop stayed scalar. This also speeds up `translate` and `replace`, which call this method. * Comment cleanups / improvements * Test consolidation: remove redundant tests, move more tests to SLT, improve test coverage * Benchmark consolidation: move `upper` and `lower` benchmarks to the same place, add benchmarks for `upper` and `lower` on mixed ASCII and non-ASCII inputs, add benchmark for `translate` on `Utf8View` ## What is the testing strategy for this PR? Existing tests pass; new tests added. ## Are there any user-facing changes? No.
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
Many execution paths allocate more than they need. Collections start empty although their final size is known, and some temporaries are allocated again on every row, batch, partition or column. Each case is small, but together they add avoidable allocator traffic on hot paths.
What changes are included in this PR?
Two mechanical rules applied across the workspace:
with_capacitywhere the size is known or tightly bounded by in-memory data.clear()when they don't escape the iteration.For example,
RepartitionExecno longer allocates aVecper batch per output partition,TDigestmerging reuses one buffer instead of a full-sizeVecper merge step, and hive-partitioned writes no longer build aVec<String>key per row.What is the testing strategy for this PR?
No behavior change, so existing tests cover it. The workspace
--lib --tests --binssuite and sqllogictest pass locally. Criterion benches againstmain(regexp_replace,reverse,array_slice,bounded_window) are neutral to a few percent faster.Are there any user-facing changes?
No.