-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[PARQUET] Improve memory efficency for compressed writer parquet 1.0 #8527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PARQUET] Improve memory efficency for compressed writer parquet 1.0 #8527
Conversation
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lilianm -- this seems like a reasonable plan to me. My only concern is that it may slow down the encoder for other usecases as shrink_to_fit may copy the bytes.
I'll start some benchmarks to see if we can see any difference.
If we do, perhaps we can apply some heuristic like only call shrink_to_fit if the buffer is less than half used or something
| if let Some(ref mut cmpr) = self.compressor { | ||
| let mut compressed_buf = Vec::with_capacity(uncompressed_size); | ||
| cmpr.compress(&buffer[..], &mut compressed_buf)?; | ||
| compressed_buf.shrink_to_fit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should apply the same optimization to V2 path below 🤔
Also, @mapleFU recently updated the compression check for V2 pages to use the uncompressed values if the compression didn't actually reduce the space. Maybe we should apply that to V1 pages too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cost of copy is pretty insignifiant because memcpy speed it's around 10000MB/s and compression speed it's around 600MB/s. Underlayer vector use shink method https://doc.rust-lang.org/alloc/alloc/trait.Allocator.html#method.shrink. In standard malloc threadhold for switch to mmap allocation it's 128k and for shrink the system only unmap page and no need memory copy.
In V2 page buffer is not reserved
For no compress page when compression it's bad i can be a good idea to apply for V1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not possible to disable compression on the fly with page v1
https://github.com/apache/parquet-format/blob/9fd57b59e0ce1a82a69237dcf8977d3e72a2965d/src/main/thrift/parquet.thrift#L671-L692
|
🤖 |
|
🤖: Benchmark completed Details
|
|
@alamb I don't find bench with compression enable in writer_batch test. Maybe i have missing some things |
You are probably right -- could you could make a new PR to add such a benchmark so that we can evaluate the performance impact of this one? |
|
I updated this PR to get the new parquet writer benchmarks and am rerunning them. Thank you @lilianm |
|
🤖 |
|
🤖: Benchmark completed Details
|
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My analysis of the benchmark results is that this PR does not change performnace significantly
Thank you @lilianm for your contribution and attention to detail 🦾
|
Thanks again @lilianm |
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Memory improvement for page v1 with data compression
Are these changes tested?
N/A already tested
Are there any user-facing changes?
No