GH-47514: [C++][Parquet] Add unpack tests and benchmarks #47515
Conversation
|
|
AntoinePrv
force-pushed
the
unpack-test-benchmark
branch
from
d8d1970 to
c77759e
Compare
There was a problem hiding this comment.
Thanks a lot for doing this @AntoinePrv , this will be a nice improvement.
| num_bits); | ||
| int unpack32_avx2(const uint8_t* in, uint32_t* out, int batch_size, int num_bits) { | ||
| return unpack32_specialized<UnpackBits256<DispatchLevel::AVX2>>( | ||
| reinterpret_cast<const uint32_t*>(in), out, batch_size, num_bits); |
There was a problem hiding this comment.
I think we can also change the signature of the generated functions, what do you think? The current signature assumes the SIMD functions will load the input in 32-bit chunks, but they might make different choices.
There was a problem hiding this comment.
I intended to keep that for a future PR, and keep this one focused on the tests and benchmark of the "public" functions. There are a few things we can change internally.
cpp/src/arrow/util/bpacking_test.cc
Outdated
| auto [num_values, bit_width] = GetParam(); | ||
| constexpr int32_t kExtraValues = sizeof(Int) * 8; | ||
|
|
||
| auto const packed = generateRandomPackedValues(num_values + kExtraValues, bit_width); |
There was a problem hiding this comment.
The random data doesn't need to be generated again in each call to TestRoundtripAlignement. You can just generate it in the fixture's SetUp method.
There was a problem hiding this comment.
We need to know the largest test parameter to know how big of a buffer to generate.
cpp/src/arrow/util/bpacking_test.cc
Outdated
| }; | ||
|
|
||
| INSTANTIATE_TEST_SUITE_P( | ||
| MutpliesOf64Values, UnpackingRandomRoundTrip, |
There was a problem hiding this comment.
Because these functions do not handle all possible input sizes.
| return out; | ||
| } | ||
|
|
||
| /// Use BitWriter to pack values into a vector. |
There was a problem hiding this comment.
So BitWriter is used as a reference bit-packing implementation?
There was a problem hiding this comment.
Arf, yes. It is not so simple to write a dumb implementation with alignments, bit shifts...
I thought on top of that we could add some unpacking tests against known values (manually packed) but the data may not be very readable.
There was a problem hiding this comment.
I have added tests for some known values for which we can easily extrapolate the unpacking almost independently of the bit_width:
- All zeros
- All ones
- Alternating ones and zeros
github-actions
bot
added
awaiting committer review
AntoinePrv
force-pushed
the
unpack-test-benchmark
branch
2 times, most recently
from
dfb2cd0 to
3b39c0f
Compare
|
#47501 was just merged, you'll need to rebase/merge at some point and then fix lint failures. |
AntoinePrv
force-pushed
the
unpack-test-benchmark
branch
from
3b39c0f to
90a4495
Compare
|
@ursabot please benchmark lang=C++ |
|
Benchmark runs are scheduled for commit caeec86. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete. |
|
The |
|
Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit caeec86. There were 3 benchmark results indicating a performance regression:
The full Conbench report has more details. |
|
The benchmarks above are interesting @AntoinePrv . On all 4 machines, the scalar version is faster than the SIMD version for bit_width=20. Perhaps more insights could be gained by looking at the generated SIMD code. From a quick look at the AVX2 unpack32 functions, I see a lot of scalar shifts ( |
|
That said, I expect the most important specializations to be those with a small bit width, so this might not be an important concern for now. |
|
Yes, I had noticed similar behavior. Gonna investigate in #47573. |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 931acd8. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
Tests and benchmarks make it easier to iterate and compare improvements.
What changes are included in this PR?
unpackDD_XXXfunctions for genericity in tests/benchmarks.Also results in safer API suggesting that the data may not be aligned.
Are these changes tested?
Yes very much.
Are there any user-facing changes?
No.