Skip to content
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

Jump to bottom

Speed up writing bloom filter #3325

Closed
wants to merge 2 commits into from
Closed

Speed up writing bloom filter #3325

wants to merge 2 commits into from

Conversation

viirya
Copy link
Member

@viirya viirya commented Dec 10, 2022

Which issue does this PR close?

Part of #3320.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the parquet Changes to the parquet crate label Dec 10, 2022
@viirya
Copy link
Member Author

viirya commented Dec 10, 2022 

write_batch primitive/4096 values primitive non-null with bloom filter
                        time:   [4.9215 ms 5.0923 ms 5.2726 ms]
                        thrpt:  [32.831 MiB/s 33.993 MiB/s 35.173 MiB/s]
                 change:
                        time:   [-12.924% -7.9765% -2.7587%] (p = 0.00 < 0.05)
                        thrpt:  [+2.8370% +8.6680% +14.842%]
                        Performance has improved.

write_batch primitive/4096 values string with bloom filter
                        time:   [996.35 µs 998.81 µs 1.0016 ms]
                        thrpt:  [79.512 MiB/s 79.730 MiB/s 79.927 MiB/s]
                 change:                                                                                           
                        time:   [-22.548% -21.255% -19.970%] (p = 0.00 < 0.05)
                        thrpt:  [+24.953% +26.991% +29.112%]                  
                        Performance has improved.                                                                  

write_batch primitive/4096 values string dictionary with bloom filter
                        time:   [485.50 µs 487.92 µs 491.49 µs]               
                        thrpt:  [97.950 MiB/s 98.668 MiB/s 99.159 MiB/s]
                 change:                         
                        time:   [-19.659% -18.522% -17.359%] (p = 0.00 < 0.05)
                        thrpt:  [+21.006% +22.733% +24.470%]
                        Performance has improved.

write_batch primitive/4096 values string non-null with bloom filter
                        time:   [1.0293 ms 1.0347 ms 1.0454 ms]
                        thrpt:  [75.240 MiB/s 76.018 MiB/s 76.419 MiB/s]
                 change:
                        time:   [-28.353% -27.839% -27.174%] (p = 0.00 < 0.05)
                        thrpt:  [+37.313% +38.580% +39.573%]
                        Performance has improved.

tustvold
tustvold reviewed Dec 12, 2022
View reviewed changes
parquet/src/bloom_filter/mod.rs
))
})?;
let pointer: *const u8 = (word as *const u32) as *const _;
let bytes: &[u8] =
Copy link
Contributor

Choose a reason for hiding this comment

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

NGL it is kind of disappointing that LLVM isn't able to automatically make this optimization, I will have a play and see if I can get it to do the correct thing

parquet/src/bloom_filter/mod.rs
let bytes: &[u8] =
unsafe { slice::from_raw_parts(pointer, mem::size_of::<u32>()) };
if cfg!(target_endian = "big") {
Self::write_all_for_big_endian(&mut writer, bytes)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just use word.to_le_bytes() here, I imagine it will be faster than the code below and is certainly less code

Copy link
Member Author

Choose a reason for hiding this comment

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

okay

@tustvold
Copy link
Contributor

This nerd-sniped me to the point where I created #3333, which at least in my testing performs better than this PR, PTAL 😄

@viirya viirya closed this Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tustvold tustvold tustvold left review comments

Assignees
No one assigned
Labels
parquet Changes to the parquet crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@viirya @tustvold