Improve performance of set_bits by avoiding to set individual bits #6288
Conversation
This file contains 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 @andygrove @viirya |
kazuyukitanimura
force-pushed
the
fix-set-bits
branch
from
82cbff6 to
1e9de38
Compare
crepererum
reviewed
Aug 26, 2024
This reverts commit a15db14.
5 tasks
|
Here are the benchmark results I got from this branch: 🚀 |
crepererum
requested changes
Sep 10, 2024
Dandandan
reviewed
Sep 10, 2024
| } else { | ||
| let len = std::cmp::min(len, 64 - std::cmp::max(read_shift, write_shift)); | ||
| let bytes = ceil(len + read_shift, 8); | ||
| let chunk = unsafe { read_bytes_to_u64(data, read_byte, bytes) }; |
There was a problem hiding this comment.
Could you add some // SAFETY: explanations to unsafe usages?
crepererum
approved these changes
Sep 11, 2024
4 tasks
|
Thank you @crepererum @alamb @andygrove @viirya @Dandandan Any other comments? |
alamb
reviewed
Sep 13, 2024
alamb
reviewed
Sep 13, 2024
There was a problem hiding this comment.
Thank you @kazuyukitanimura -- I will review this PR later today
alamb
reviewed
Sep 13, 2024
There was a problem hiding this comment.
Thank you @kazuyukitanimura -- I believe this code is correct, but I think it needs some more testing for me to be super confident it in, especially given its profuse use of unsafe as mentioned by @crepererum
I have an idea for a fuzz testing -- I am going to try and code something up and report back here
| fn test_set_upto_64bits() { | ||
| // len >= 64 | ||
| let write_data: &mut [u8] = &mut [0; 9]; | ||
| let data: &[u8] = &[ |
There was a problem hiding this comment.
Could you please also add a test that is greater than 64 bits (not just = 64 bits)?
There was a problem hiding this comment.
Sure, I will add it. BTW there is an existing test https://github.com/apache/arrow-rs/blob/master/arrow-buffer/src/util/bit_mask.rs#L170
There was a problem hiding this comment.
I am working on some more tests too. Stay tuned...
Merged
Thanks @alamb FYI it looks there is an existing fuzz test although this is limited to |
alamb
approved these changes
Sep 14, 2024
There was a problem hiding this comment.
I wrote a fuzz tester for set_bits here: #6394
I ran the fuzz tester against this branch and it passed
I also ran it under MIRI (which took quite a while) and it found no errors
andrewlamb@Andrews-MacBook-Pro-2:~/Software/arrow-rs$ cargo +nightly miri test -p arrow-buffer --profile test --lib util::bit_mask::tests
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.04s
Running unittests src/lib.rs (target/miri/aarch64-apple-darwin/debug/deps/arrow_buffer-82bdebfeced20ddb)
running 6 tests
test util::bit_mask::tests::set_bits_fuz ... ok
test util::bit_mask::tests::test_set_bits_aligned ... ok
test util::bit_mask::tests::test_set_bits_unaligned ... ok
test util::bit_mask::tests::test_set_bits_unaligned_destination_end ... ok
test util::bit_mask::tests::test_set_bits_unaligned_destination_start ... ok
test util::bit_mask::tests::test_set_upto_64bits ... ok
test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 112 filtered out; finished in 38.62s
Therefore, I think this is very well done @kazuyukitanimura 👏 🏆 🚀
|
Let's get it merged |
|
Thanks again everyone -- this is quite cool |
|
Thank you @alamb @crepererum @andygrove @viirya @Dandandan |
alamb
added
performance
enhancement
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
Rationale for this change
This PR improves the performance of set_bits that is often the bottleneck for filter execs in DataFusion. In particular,
len < 64is the majority for TPCDS and this PR helps for that scenario.For the existing bench
What changes are included in this PR?
Consolidating
BitChunks::newand individual bit manipulations into one method.Are there any user-facing changes?
No