Conversation
Code Review SummaryStatus: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (3 files)
|
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| * @brief Mutable view as bytes. | ||
| * @note Uses a byte pointer to the underlying storage. | ||
| */ | ||
| std::span<std::uint16_t> As16BitInts() { |
There was a problem hiding this comment.
WARNING: Missing <cstdint> include for std::uint16_t
As16BitInts() and AsConst16BitInts() use std::uint16_t but the header doesn't include <cstdint>, which can break builds depending on transitive includes.
|
|
||
| std::mt19937_64 rng(42); | ||
| for (auto _ : state) { | ||
| uint64_t pos = rng() % n; |
There was a problem hiding this comment.
SUGGESTION: Avoid modulo when n is a power of two in this hot loop
For power-of-two sizes, using pos = rng() & (n - 1) can be measurably faster than % n. You already apply this optimization in BM_RankNonInterleaved12p5PercentFill, and reusing it here would keep benchmark overhead consistent with the other paths.
| pixie::BitVector bv(bits, n); | ||
| AlignedStorage bits(n); | ||
| PrepareRandomBits50pFill(bits.As64BitInts()); | ||
| pixie::BitVector bv(bits.As64BitInts(), n); |
There was a problem hiding this comment.
SUGGESTION: Cache the 64-bit span once for this benchmark
You call As64BitInts() twice for the same AlignedStorage. Storing it in a local auto bits_as_words = bits.As64BitInts(); avoids redundant span construction and size computation in hot benchmark setup code.
1 participant
No description provided.