perf(arrow-ipc): Avoid copies and write dictionary batches directly to writers when possible - #10128
Open
JakeDern wants to merge 7 commits into
Open
perf(arrow-ipc): Avoid copies and write dictionary batches directly to writers when possible#10128JakeDern wants to merge 7 commits into
JakeDern wants to merge 7 commits into
Conversation
Contributor
Author
|
Pretty good improvement - ~42% for the dictionary case and ~20% for delta dictionary cases. Not 100% sure why less improvement on the delta side yet, but I think this is worth it to take on its own and can investigate further later. Perf results from #10122: perf results from this branch: |
Contributor
Author
|
CC: @alamb and @Rich-T-kid - I think we got pretty good results here! I also tried to clean up a few things here and there where I could like removing some unnecessary parameter drilling. This has the benchmarks from #10122 as well, will rebase once that goes in. |
Contributor
|
I can take a look at this early next week. |
Contributor
|
started taking a look, results look good! 🚀 |
Rich-T-kid
reviewed
Jun 17, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
I think this PR looks mostly fine. it would be nice to include a round trip test similar to ##10097 that validate that nothing is being broken.
left a couple non-blocking comments. Ill try and take a second pass through tommorow if I can get a chance/ if this is still open
JakeDern
force-pushed
the
ipc-writer-collect-dicts
branch
2 times, most recently
from
July 10, 2026 04:07
2a21b60 to
f9a0328
Compare
Contributor
Author
|
Took a little longer than I'd have liked, but I got back to rebasing this and fixing it up. @Rich-T-kid or @alamb would definitely appreciate another look.
We do have some round trip tests for the writer, but if there's anything specific missing here, I'm happy to take a stab at adding it! |
JakeDern
marked this pull request as ready for review
July 10, 2026 04:58
Contributor
|
Ill try and take another look next week |
Contributor
|
cc @Phoenix500526 in case your interested @JakeDern could you rebase this branch? I think there are outdated changed in your diff |
Contributor
|
This may also be related #9775 |
Contributor
|
run benchmark arrow_writer |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing ipc-writer-collect-dicts (fd9d7c2) to fed7862 (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 |
JakeDern
force-pushed
the
ipc-writer-collect-dicts
branch
from
July 21, 2026 05:15
fd9d7c2 to
2bda004
Compare
Contributor
Author
I think we're all fixed up now - Nice to see so much IPC activity! |
Contributor
@alamb I think you meant to run |
Contributor
|
run benchmark ipc_writer |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing ipc-writer-collect-dicts (2bda004) to 98998a8 (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
|
|
Rich-T-kid
suggested changes
Jul 24, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
The benchmarks look very good and the PR looks nice. I left a couple of suggestions.
I'd like to take a second look as well! could you update the description to explain the flow between calls a bit more? The arrow-ipc reader/writer code takes a lot of context to keep in mind. 😮💨
Rich-T-kid
approved these changes
Jul 28, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
Thanks @JakeDern this looks good to me. I left a couple comments, mostly nits.
🚀
Contributor
|
also with @Phoenix500526's #10277 being merged yesterday we may have to rebase this again |
JakeDern
force-pushed
the
ipc-writer-collect-dicts
branch
from
July 29, 2026 03:52
dcfe3f9 to
e946870
Compare
Contributor
Author
|
Alright @Rich-T-kid, one more rebase for the books! |
Contributor
Author
I'm a bit stumped actually, I can't reproduce the perf regression on my machine. Maybe it only happens on ARM64? Will play around with it a bit more... |
Contributor
Author
|
It's also only occurring after the most recent rebase which is interesting |
JakeDern
force-pushed
the
ipc-writer-collect-dicts
branch
from
August 19, 2026 02:26
8c91263 to
5162fd3
Compare
Contributor
|
run benchmark ipc_writer |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing ipc-writer-collect-dicts (5162fd3) to 505afcb (merge-base) diff Run configurationrun benchmark ipc_writerBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench ipc_writer File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing ipc-writer-collect-dicts (5162fd3) to 505afcb (merge-base) diff Run configurationrun benchmark ipc_writerCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
|
run benchmark ipc_writer |
1 similar comment
Contributor
|
run benchmark ipc_writer |
Contributor
|
seems to have gone down |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing ipc-writer-collect-dicts (5162fd3) to 505afcb (merge-base) diff Run configurationrun benchmark ipc_writerBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench ipc_writer File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing ipc-writer-collect-dicts (5162fd3) to 505afcb (merge-base) diff Run configurationrun benchmark ipc_writerBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench ipc_writer File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing ipc-writer-collect-dicts (5162fd3) to 505afcb (merge-base) diff Run configurationrun benchmark ipc_writerCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing ipc-writer-collect-dicts (5162fd3) to 505afcb (merge-base) diff Run configurationrun benchmark ipc_writerCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
|
the regression is still there. I'll take a look at the PR to try and help pinpoint it |
Contributor
Author
Thanks, I was also planning to try and spin up an arm64 machine on gcp this week and see if I could reproduce it there. Will report back on what I find |
Contributor
|
run benchmark ipc_writer |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing ipc-writer-collect-dicts (5162fd3) to 505afcb (merge-base) diff Run configurationrun benchmark ipc_writerBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench ipc_writer File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing ipc-writer-collect-dicts (5162fd3) to 505afcb (merge-base) diff Run configurationrun benchmark ipc_writerCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Rich-T-kid
suggested changes
Aug 23, 2026
Contributor
There was a problem hiding this comment.
PR still mostly looks good to me, left a couple suggestions. it may also be useful to look at the profiles of both benchmarks between main and your branch to see if there any new hot spots.
feel free to ping me to run the benchmarks after you update the PR 🚀
Contributor
|
run benchmark ipc_writer |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing ipc-writer-collect-dicts (1091642) to cd7c6b8 (merge-base) diff Run configurationrun benchmark ipc_writerBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench ipc_writer File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing ipc-writer-collect-dicts (1091642) to cd7c6b8 (merge-base) diff Run configurationrun benchmark ipc_writerCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
slightly better? |
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
6 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
This is a follow on to #10044, applying basically the same optimization for dictionary batches.
This needs to wait for #10122 before merge.
What changes are included in this PR?
Are these changes tested?
Yes, existing unit tests should cover the change.
Are there any user-facing changes?
No.