fix: shrink DataPageV2 buffer to fit before converting to Bytes - #10520
Open
vedjaw wants to merge 1 commit into
Open
fix: shrink DataPageV2 buffer to fit before converting to Bytes#10520vedjaw wants to merge 1 commit into
vedjaw wants to merge 1 commit into
Conversation
…he#10448) The DataPageV2 code path in add_data_page builds its buffer by incrementally extending a Vec - first repetition levels, then definition levels, then compressed or uncompressed values. This incremental growth leaves the Vec with excess capacity that is never reclaimed before the buffer is converted to Bytes and handed to the page writer. The PARQUET_1_0 (DataPage v1) path already calls shrink_to_fit on its compressed buffer, but the PARQUET_2_0 path did not. As a result, DataPageV2 buffers - especially those held for the lifetime of a dictionary-encoded column or buffered by a deferred page writer - retained their over-allocation indefinitely, inflating peak memory usage for no benefit. This adds a buffer.shrink_to_fit() call immediately before the Page::DataPageV2 construction, matching the v1 path's behaviour and ensuring pages carry no slack capacity into the page writer.
Contributor
|
run benchmark arrow_writer env:
BENCH_FILTER: parquet_2 |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/v2-shrink-to-fit-memory-optimization (53a38ff) to ed92960 (merge-base) diff Run configurationrun benchmark arrow_writer
env:
BENCH_FILTER: "parquet_2"BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_writer File an issue against this benchmark runner |
Contributor
|
Thanks @vedjaw, this seems reasonable. Looking over the existing code, I wonder if we could first Edit: it seems discussion is continuing in the linked issue, so perhaps this is premature. |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing fix/v2-shrink-to-fit-memory-optimization (53a38ff) to ed92960 (merge-base) diff Run configurationrun benchmark arrow_writer
env:
BENCH_FILTER: "parquet_2"CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
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
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.
Which issue does this PR close?
Closes #10448.
Rationale for this change
The
add_data_pagefunction builds the DataPageV2 buffer by incrementally extending a Vec: first repetition levels, then definition levels, then values (compressed or uncompressed). This incremental growth viaextend_from_slicecan leave significant excess capacity.The PARQUET_1_0 (DataPage v1) path already calls
shrink_to_fiton its compressed buffer, but the PARQUET_2_0 path never reclaimed the excess capacity before converting the buffer toBytesand handing it to the page writer.For dictionary-encoded columns and deferred/buffered page writers, these pages can be retained in memory for extended periods. Each page carrying slack capacity contributes to inflated peak RSS.
What changes are included in this PR?
Adds a
buffer.shrink_to_fit()call immediately before thePage::DataPageV2construction inadd_data_page, matching the v1 path's existing behavior.Are there any user-facing changes?
No API changes. Pages produced by the writer now carry no slack capacity, which may reduce peak memory usage for workloads that use DataPage v2 (the default for
parquet::file::properties::WriterPropertieswhendata_page_versionis set toV2).Are these changes tested?
The existing DataPageV2 test suite (100 tests in
column::writer::tests) all pass, including roundtrip and compression tests. The change is a memory-management detail and does not alter the serialized page format.