Skip to content

[Parquet] Reduce one copy in SerializedPageReader #8745

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.

Sign up for GitHub

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

Merged
alamb merged 5 commits into from
Nov 1, 2025
Merged

[Parquet] Reduce one copy in SerializedPageReader #8745

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6f1d112
mimalloc
XiangpengHao Oct 30, 2025
be939ef
reduce copy
XiangpengHao Oct 30, 2025
55e9d82
Merge branch 'main' into reduce-copy
XiangpengHao Oct 30, 2025
1cb0cdc
remove mimalloc
XiangpengHao Oct 31, 2025
afa2d1c
Merge branch 'reduce-copy' of https://github.com/XiangpengHao/arrow-r…
XiangpengHao Oct 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions parquet/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ tokio = { version = "1.0", default-features = false, features = ["macros", "rt-m
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 = "*" }

@alamb alamb Oct 31, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@XiangpengHao XiangpengHao Oct 31, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only dev dependency, used to make benchmark more accurate

@alamb alamb Oct 31, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@XiangpengHao XiangpengHao Oct 31, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense, I've removed mimalloc


[package.metadata.docs.rs]
all-features = true
Expand Down
3 changes: 3 additions & 0 deletions parquet/benches/arrow_reader_clickbench.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ use std::fmt::{Display, Formatter};
use std::path::{Path, PathBuf};
use std::sync::{Arc, OnceLock};

#[global_allocator]
static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc;

fn async_reader(c: &mut Criterion) {
let rt = tokio::runtime::Builder::new_current_thread()
.enable_all()
Expand Down
18 changes: 5 additions & 13 deletions parquet/src/file/serialized_reader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,9 +397,9 @@ pub(crate) fn decode_page(
}
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]);

@alamb alamb Oct 31, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alamb alamb Oct 31, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow on:

if decompressed_size > 0 {
let compressed = &buffer.as_ref()[offset..];
let compressed = &buffer[offset..];
decompressor.decompress(compressed, &mut decompressed, Some(decompressed_size))?;
}

Expand Down Expand Up @@ -897,31 +897,23 @@ impl<R: ChunkReader> PageReader for SerializedPageReader<R> {
*remaining,
)?;
let data_len = header.compressed_page_size as usize;
let data_start = *offset;
*offset += data_len as u64;
*remaining -= data_len as u64;

if header.r#type == PageType::INDEX_PAGE {
continue;
}

let mut buffer = Vec::with_capacity(data_len);
let read = read.take(data_len as u64).read_to_end(&mut buffer)?;

if read != data_len {
return Err(eof_err!(
"Expected to read {} bytes of page, read only {}",
data_len,
read
));
}
let buffer = self.reader.get_bytes(data_start, data_len)?;

@alamb alamb Oct 31, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm on review this can potentially avoid a copy if the underlying reader is already Bytes


let buffer =
self.context
.decrypt_page_data(buffer, *page_index, *require_dictionary)?;

let page = decode_page(
header,
Bytes::from(buffer),
buffer,
self.physical_type,
self.decompressor.as_mut(),
)?;
Expand Down
Loading