wgtmac commented on code in PR #55449:
URL: https://github.com/apache/arrow/pull/55449#discussion_r4526989586
##########
cpp/src/arrow/util/alp/alp_compression.cc:
##########
@@ -524,16 +525,25 @@ template <typename TargetType>
void AlpCompression<T>::PatchExceptions(
std::span<const T> exceptions,
std::span<const AlpFormatConstants::PositionType> exception_positions,
- std::span<TargetType> outputs) {
+ const bool positions_strictly_increasing, std::span<TargetType> outputs) {
ARROW_DCHECK_EQ(exceptions.size(), exception_positions.size());
- // `ivdep` assumes the positions are distinct. The encoder writes them in
- // order, and loading a page rejects positions that do not increase.
- int64_t exception_idx = 0;
- ARROW_ALP_UNROLL_AND_ASSUME_INDEPENDENT
- for (const AlpFormatConstants::PositionType exception_position :
exception_positions) {
- ARROW_DCHECK_LT(exception_position, outputs.size());
- outputs[exception_position] = static_cast<T>(exceptions[exception_idx]);
- exception_idx++;
+ const size_t num_exceptions = exception_positions.size();
+ const auto patch_exception = [&](size_t i) {
+ const auto position = exception_positions[i];
+ ARROW_DCHECK_LT(position, outputs.size());
+ outputs[position] = static_cast<T>(exceptions[i]);
+ };
+ if (positions_strictly_increasing) {
+ // The encoder emits strictly increasing positions. Use the vectorized
+ // fast path for that common case.
Review Comment:
https://github.com/apache/arrow/pull/48345#discussion_r3144916033 mentioned
testing unroll factors of 2, 4 and 8, but provided no measurements specific to
exception patching.
When I made this change, I have verified that the code path has been
vectorized by gcc on godbolt. But I didn't verify its performance gain because
I used Clang locally which failed to do this :/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
