[Parquet] Reduce one copy in SerializedPageReader
#8745
Merged
+5
−13
Conversation
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
alamb
reviewed
Oct 31, 2025
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @XiangpengHao and @MikeWalrus ❤️
I don't think the parquet crate should be setting a global allocator, but otherwise this PR looks great to me
I think this change is justified on code simplification grounds alone even if we can't measure a performance difference
I am also going to explore the potential decompressor optimziation in a follow on PR
parquet/Cargo.toml
Outdated
| rand = { version = "0.9", default-features = false, features = ["std", "std_rng", "thread_rng"] } | ||
| object_store = { version = "0.12.0", default-features = false, features = ["azure", "fs"] } | ||
| sysinfo = { version = "0.37.1", default-features = false, features = ["system"] } | ||
| mimalloc = { version = "*" } |
Contributor
There was a problem hiding this comment.
I don't think we should add this in the parquet crate as it will conflict with downstream crates that want to use a different allocator
Contributor
Author
There was a problem hiding this comment.
This is only dev dependency, used to make benchmark more accurate
Contributor
There was a problem hiding this comment.
Let's get some benchmark results with the different allocator
However, I still don't think it is a good idea to use a non-system allocator for dev/benchmarks as it will make the benchmarks potentially farther from what the (average) user actually experiences
Contributor
Author
There was a problem hiding this comment.
make sense, I've removed mimalloc
| let decompressed_size = uncompressed_page_size - offset; | ||
| let mut decompressed = Vec::with_capacity(uncompressed_page_size); | ||
| decompressed.extend_from_slice(&buffer.as_ref()[..offset]); | ||
| decompressed.extend_from_slice(&buffer[..offset]); |
Contributor
There was a problem hiding this comment.
This seems an unrelated (but nice) cleanup
While looking at this code, it seems like it always copies the compressed bytes, even when it then decompresses it immediately. I'll make a small PR to see if I can remove that unecessary copy
Contributor
There was a problem hiding this comment.
| read | ||
| )); | ||
| } | ||
| let buffer = self.reader.get_bytes(data_start, data_len)?; |
Contributor
There was a problem hiding this comment.
I confirm on review this can potentially avoid a copy if the underlying reader is already Bytes
alamb
changed the title
SerializedPageReaderSerializedPageReader
Contributor
|
🤖 |
Contributor
|
🤖: Benchmark completed Details
|
Contributor
|
🤖 |
Contributor
|
🤖: Benchmark completed Details
|
Contributor
|
🤖 |
Contributor
|
🤖: Benchmark completed Details
|
Contributor
Author
|
Looks like we have some meaningful perf difference here: But I'm not sure if this is because a reduced copy or mimalloc, should we run |
Contributor
|
will rerun |
alamb
approved these changes
Oct 31, 2025
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you again @XiangpengHao and @MikeWalrus
Contributor
|
🤖 |
Contributor
|
🤖: Benchmark completed Details
|
Contributor
|
🤖 |
Contributor
|
🤖: Benchmark completed Details
|
The regression is indeed related to memory allocation (brk, page faults, etc.), but I'm not sure if it's due to the read buffers being held. Those buffers are small and freed quickly. I traced that query and made a visualization. Interactive version. Script and raw traces. @XiangpengHao @alamb Any thoughts? |
Contributor
Author
|
I'm pretty sure this has to do with things not under our control, and I wouldn't call it a regression, but system benchmark artifact. 
⬆️ Both binary are the same, as evidenced by the same hash. But their performance is very very different. and it seems to has something to do with the filename length... =========== One more thing about page-faults: slow one: fast one: ===== I have disabled ASLR, so it must come from some weird things in the glibc allocator... |
Fyi, that difference goes away if there's no So without that weird artifact, there's still something going on that makes this branch slower than main. Also, the memory allocation trace I got was without |
Contributor
So in my mind the most recent benchmark results show a performance improvement for this branch In an ideal world, benchmark results would be 100% reproducible and free of noise. However, in the real world, especially on the "machine" I am using to benchmark (a VM) there are many sources of noise in the measurements:
So while I applaud our efforts here to be scientific, I also think it has passed the level of scrutiny needed From my perspective, the code after this PR is clearly doing less work, and shows improvements in the benchmarks (even if there is some noise), thus it is a net improvement over what is on main |
Contributor
The memory usage over time is quite neat |
Contributor
I agree 100% with this |
Contributor
|
Let's merge this one in and keep making the code better going forward |
|
@alamb I agree. Thanks! |
Contributor
Thank you for the (great!) find |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
This was originally found by @MikeWalrus
Basically the ChunkReader for the async reader is
ColumnChunkData:arrow-rs/parquet/src/arrow/in_memory_row_group.rs
Lines 282 to 292 in 2eabb59
Which by itself is
Bytes. The original implementation will copy the data from it and later only to make it a newBytes.This PR removes it.
Normally this should mean performance improvements across the board, but here're the nuances:
tldr: with mimalloc, it will always improve performance, or at least as fast as the original implementation, tested locally with
arrow_reader_clickbenchcc @tustvold and @alamb who might know this better