Repository navigation
GH-55399: [C++][Parquet] Minor optimize level counting with CounfIf16 #55408
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include <algorithm> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include <cstdint> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include <limits> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include "parquet/platform.h" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -35,4 +36,78 @@ struct MinMax { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| MinMax PARQUET_EXPORT FindMinMax(const int16_t* levels, int64_t num_levels); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Counts the levels that satisfy `pred`, in chunks small enough that a | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// `uint16_t` accumulator cannot overflow. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// This is the fallback for inputs that do not fit in a `uint16_t` accumulator. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// It is kept out of line so that the common path (a single Parquet page chunk, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// always short enough for one accumulator) is a plain loop without any chunk | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// bookkeeping in front of it. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+43
to
+45
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was it measured to make a difference?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Generally speaking yes. The asm and benchmark shows this could benefit the performance.
Yes, but all changes are verified and reviewed myself |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| template <typename Predicate> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ARROW_NOINLINE int64_t CountIf16Chunked(const int16_t* levels, int64_t num_levels, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Predicate pred) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // The loop below accumulates one match per lane in a `uint16_t`, so a chunk | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // must not hold more levels than that can count. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| constexpr int64_t kMaxCount = std::numeric_limits<uint16_t>::max(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int64_t total = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| while (num_levels > 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const int64_t chunk_length = std::min(num_levels, kMaxCount); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint16_t chunk_count = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (int64_t i = 0; i < chunk_length; ++i) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| chunk_count += pred(levels[i]) ? 1 : 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| total += chunk_count; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| levels += chunk_length; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| num_levels -= chunk_length; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return total; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Count the levels that satisfy the given predicate. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// The matches are accumulated in a `uint16_t`, which cannot overflow as long | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// as at most `std::numeric_limits<uint16_t>::max()` levels are counted at a | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// time, so longer inputs are delegated to `CountIf16Chunked`. This is both | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// safe and much faster than `std::count`, as the loop can be vectorized with | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// 16-bit accumulators (one accumulated match per lane) instead of widening | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// every match to a 64-bit integer. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// \param[in] levels The level values to inspect. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// \param[in] num_levels The number of levels to inspect, must be non-negative. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// \param[in] pred The predicate to apply, must be callable as | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// `bool(int16_t)`. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// \return The number of levels for which `pred` returns true. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| template <typename Predicate> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| inline int64_t CountIf16(const int16_t* levels, int64_t num_levels, Predicate pred) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Short inputs are counted first, before the wide loop and before the chunk | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // clamp, because a handful of levels cannot amortize either. Running the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // small case first is also how folly::simd::detail::simdForEach orders its | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // single steps before the unrolled ones, so that short inputs never pay for | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // the unrolled set up. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 32 is the widest per-iteration body this reduction gets: a packed int16_t | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // register holds 8 levels with SSE2/NEON and 16 with AVX2, but clang runs up | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // to 32 levels per iteration on NEON (four registers), so shorter inputs | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // never reach a full iteration on the narrow targets. The bound also has to | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // be a compile-time power of two, which is what turns the loop into an | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // unrolled ladder of 8/16/24-wide compares instead of scalar code. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+82
to
+93
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This lengthy comment is AI-generated, right?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I'll rewrite. it myself |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| constexpr int64_t kSmallLevels = 32; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| constexpr int64_t kMaxCount = std::numeric_limits<uint16_t>::max(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (num_levels < kSmallLevels) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to keep this
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know, it would making the very short loops (9-31 elements?) a bit slower ( like 1.5ns -> 3ns ) on my Mac M4 Pro. If this doesn't matter just counting by chunk is ok.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the point of this
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found this nice for small benchmark, actuall when Without Detail see: https://godbolt.org/z/nj1vKvqPY Performance on my MacOS:
I can remove this if we think it's a benchmark hack
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Micro-benchmarks like this do not tell us about performance when called from Parquet C++ reader code. Inlining may be different, code placement may be different, etc.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the main difference is that it helps unroll n<32's vectorization, and tells the compiler the loop is for element >32 && <15536. But I can remove this, any way |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint16_t count = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (int64_t i = 0; i < num_levels; ++i) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| count += pred(levels[i]) ? 1 : 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return count; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (num_levels > kMaxCount) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return CountIf16Chunked(levels, num_levels, pred); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should use perfect forwarding?
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint16_t count = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (int64_t i = 0; i < num_levels; ++i) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| count += pred(levels[i]) ? 1 : 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return count; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } // namespace parquet::internal | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move these functions to a private/internal header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Emmm it's already parquet::internal in a internal header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the header is not internal, but that can be a separate issue.