Perf: Pre-size buffer allocations to avoid intermediate allocations#10262
Open
Rich-T-kid wants to merge 5 commits into
Open
Perf: Pre-size buffer allocations to avoid intermediate allocations#10262Rich-T-kid wants to merge 5 commits into
Rich-T-kid wants to merge 5 commits into
Conversation
Contributor
Author
|
pretty big descriptions for a (+3,-2) PR 😅 |
Jefffrey
reviewed
Jul 2, 2026
Jefffrey
left a comment
Jefffrey
left a comment
Contributor
There was a problem hiding this comment.
pretty big descriptions for a (+3,-2) PR 😅
you love to see it 🙂
Contributor
|
run benchmark flight |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/re-use-buffers (53f1c95) to 32bba5a (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
|
Updating to get the "use single threaded executor" in the benchmarks |
Contributor
|
run benchmark flight |
1 similar comment
Contributor
|
run benchmark flight |
Contributor
|
Running twice to make sure results are reproducable |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/re-use-buffers (7197cd2) to 8c7df18 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/re-use-buffers (7197cd2) to 8c7df18 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Rich-T-kid
force-pushed
the
rich-T-kid/re-use-buffers
branch
from
81783c4 to
a44192f
Compare
Contributor
|
run benchmark flight |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/re-use-buffers (a44192f) to 8c7df18 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
|
run benchmark flight |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/re-use-buffers (a44192f) to 8c7df18 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
Author
|
with 🤔 benchmarks aren't as strong as id expect but the amount of data being worked with isn't huge. Seems to be mostly noise due to |
Contributor
Author
|
Taking a second look at this, I'm a bit curious about the benchmark results, specifically in the cases of larger record batches, which this should ideally benefit the most. Let's say a 10MB record batch is split into 10 1MB chunks to be encoded into This means that after 🤔 Even in the case where we don't pre-allocate the vectors, this happens anyway, since the vector will eventually grow to this size regardless, so the The only additional overhead I can think of with pre-allocation is that a single But even that difference washes out: in the non-pre-allocated case, the vector still eventually crosses the mmap threshold and triggers that same mmap-backed allocation once it grows large enough.
(pre-alloc) vec: ->[(1mb)] in both cases drop doesnt return the buffer to the allocator and |
Contributor
|
perhaps because we write with arrow-rs/arrow-ipc/src/writer.rs Lines 127 to 130 in d8d980b maybe we need to use some flamegraph profiling to actually nail down whether this can be a hotspot or if its being dominated by other causes |
Contributor
Author
|
[main] Notably the percentage of calls to malloc/ time taken by malloc are cut in half, which is expected since we are doing fewer, large calls upfront. 🤔 [this branch] [main] [my branch] Show a decrease in malloc allocations that get filled in with |
Contributor
|
run benchmark flight |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/re-use-buffers (a44192f) to 8c7df18 (merge-base) diff File an issue against this benchmark runner |
Contributor
|
i get some pretty good improvements locally which seem to align with your table of when the optimization should fire: encode/fixed/8192x4 time: [6.3223 µs 6.3640 µs 6.4222 µs]
thrpt: [38.071 GiB/s 38.419 GiB/s 38.672 GiB/s]
change:
time: [+0.2755% +0.7585% +1.4886%] (p = 0.01 < 0.05)
thrpt: [−1.4668% −0.7528% −0.2747%]
Change within noise threshold.
Found 8 outliers among 100 measurements (8.00%)
1 (1.00%) high mild
7 (7.00%) high severe
encode/fixed/8192x8 time: [11.304 µs 11.314 µs 11.325 µs]
thrpt: [43.178 GiB/s 43.221 GiB/s 43.258 GiB/s]
change:
time: [+0.5348% +0.6824% +0.8462%] (p = 0.00 < 0.05)
thrpt: [−0.8391% −0.6778% −0.5319%]
Change within noise threshold.
Found 5 outliers among 100 measurements (5.00%)
3 (3.00%) high mild
2 (2.00%) high severe
encode/fixed/65536x4 time: [41.689 µs 41.794 µs 41.910 µs]
thrpt: [46.611 GiB/s 46.741 GiB/s 46.859 GiB/s]
change:
time: [+0.3794% +0.6562% +0.9363%] (p = 0.00 < 0.05)
thrpt: [−0.9277% −0.6520% −0.3779%]
Change within noise threshold.
Found 5 outliers among 100 measurements (5.00%)
5 (5.00%) high mild
encode/fixed/65536x8 time: [71.963 µs 72.078 µs 72.204 µs]
thrpt: [54.110 GiB/s 54.205 GiB/s 54.291 GiB/s]
change:
+ time: [−15.766% −15.492% −15.202%] (p = 0.00 < 0.05)
+ thrpt: [+17.927% +18.332% +18.717%]
+ Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
10 (10.00%) high mild
3 (3.00%) high severe
encode/nested/8192x4 time: [13.922 µs 13.934 µs 13.947 µs]
thrpt: [43.819 GiB/s 43.859 GiB/s 43.897 GiB/s]
change:
time: [+0.0302% +0.2961% +0.5299%] (p = 0.02 < 0.05)
thrpt: [−0.5271% −0.2952% −0.0302%]
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe
encode/nested/8192x8 time: [26.193 µs 26.229 µs 26.281 µs]
thrpt: [46.509 GiB/s 46.600 GiB/s 46.664 GiB/s]
change:
time: [+0.6524% +0.8748% +1.0919%] (p = 0.00 < 0.05)
thrpt: [−1.0801% −0.8672% −0.6481%]
Change within noise threshold.
Found 12 outliers among 100 measurements (12.00%)
6 (6.00%) high mild
6 (6.00%) high severe
encode/nested/65536x4 time: [115.74 µs 116.08 µs 116.39 µs]
thrpt: [41.958 GiB/s 42.070 GiB/s 42.195 GiB/s]
change:
+ time: [−28.716% −28.414% −28.110%] (p = 0.00 < 0.05)
+ thrpt: [+39.102% +39.692% +40.284%]
+ Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild
encode/nested/65536x8 time: [234.71 µs 234.93 µs 235.16 µs]
thrpt: [41.535 GiB/s 41.576 GiB/s 41.615 GiB/s]
change:
+ time: [−37.571% −36.777% −36.012%] (p = 0.00 < 0.05)
+ thrpt: [+56.279% +58.169% +60.183%]
+ Performance has improved.
Found 21 outliers among 100 measurements (21.00%)
15 (15.00%) high mild
6 (6.00%) high severe
encode/variable/8192x4 time: [16.089 µs 16.187 µs 16.314 µs]
thrpt: [67.386 GiB/s 67.912 GiB/s 68.327 GiB/s]
change:
- time: [+4.3447% +5.2324% +5.9907%] (p = 0.00 < 0.05)
- thrpt: [−5.6521% −4.9723% −4.1638%]
- Performance has regressed.
encode/variable/8192x8 time: [37.548 µs 37.609 µs 37.707 µs]
thrpt: [58.308 GiB/s 58.460 GiB/s 58.555 GiB/s]
change:
+ time: [−17.988% −15.154% −12.140%] (p = 0.00 < 0.05)
+ thrpt: [+13.818% +17.860% +21.933%]
+ Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
3 (3.00%) high mild
4 (4.00%) high severe
encode/variable/65536x4 time: [164.37 µs 165.07 µs 165.61 µs]
thrpt: [53.075 GiB/s 53.250 GiB/s 53.475 GiB/s]
change:
+ time: [−41.190% −40.372% −39.560%] (p = 0.00 < 0.05)
+ thrpt: [+65.454% +67.706% +70.038%]
+ Performance has improved.
encode/variable/65536x8 time: [323.54 µs 325.42 µs 327.03 µs]
thrpt: [53.755 GiB/s 54.021 GiB/s 54.334 GiB/s]
change:
+ time: [−38.285% −37.638% −37.081%] (p = 0.00 < 0.05)
+ thrpt: [+58.934% +60.354% +62.034%]
+ Performance has improved.
only bit of noise is can you produce these benchmark results locally as well? |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
Author
|
Yes im seeing similar results when I benchmark this locally (compared to main). Im on a Apple M4 Max with 64 GB of ram besides noise for smaller cases, theres large improvements for large record batches. |
Contributor
Author
|
I wonder whats causing @adriangbot to show non-significant results. is there anything I should change about the PR? |
Contributor
|
given how strong the results are locally im inclined to merge this PR as is; maybe @alamb has some insights on why the bot benchmarks arent really showing anything |
Contributor
maybe cacheing effects or something. Let's merge it in! |
alamb
reviewed
Jul 7, 2026
Rich-T-kid
commented
Jul 8, 2026
| self.scratch.reserve(additional); | ||
| } | ||
| } | ||
| pub(crate) fn scratch(&mut self) -> Vec<u8> { |
Contributor
Author
There was a problem hiding this comment.
we can actually make the field private now.
Contributor
Author
There was a problem hiding this comment.
as well as reserver_scratch
Contributor
Author
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
4 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.
Which issue does this PR close?
Rationale for this change
TLDR: Its useful to pre-allocate vectors when you know the amount of data it will require
When
IpcDataGeneratoruses theIpcBodySink::Writevariant, record batch buffer bytes are written directly into aVec. If thatVecis undersized, it repeatedly reallocates and copies bytes into a larger buffer, growing exponentially (1, 4, 16, 32 ... KB ... MB) and paying two costs on each reallocation:For large batches this cascade is expensive, and paying it fresh on every record batch chunk compounds the problem further. Since
FlightDataEncoder::split_batch_for_grpc_responsesplits record batches into roughly equal-sized chunks, we exploit this by using the previous buffer's final capacity as an estimate for the next call, keeping a correctly-sizedVecalive across iterations and avoiding repeated reallocation on the hot path.why not pre-allocate the buffers using an estimate with the length
split_batch_for_grpc_responseuses?Using the final capacity rather than the uncompressed dictionary size is intentional, since IPC encoding and compression both affect the actual bytes written, the final capacity naturally adapts to whatever encoding and compression settings are in effect rather than consistently overprovisioning.
What changes are included in this PR?
Are these changes tested?
n/a
Are there any user-facing changes?
no