From cf73d158cf452d2c48bd3e33406296c8b1abfc01 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 27 May 2020 16:03:33 -0500 Subject: [PATCH 1/2] Do not inline BitUtil::SetBitsTo --- cpp/src/arrow/util/bit_util.cc | 44 ++++++++++++++++++++++++ cpp/src/arrow/util/bit_util.h | 42 ++-------------------- cpp/src/arrow/util/bit_util_benchmark.cc | 11 ++++++ 3 files changed, 57 insertions(+), 40 deletions(-) diff --git a/cpp/src/arrow/util/bit_util.cc b/cpp/src/arrow/util/bit_util.cc index 395801f5e8d..3cd3f371142 100644 --- a/cpp/src/arrow/util/bit_util.cc +++ b/cpp/src/arrow/util/bit_util.cc @@ -58,6 +58,50 @@ void FillBitsFromBytes(const std::vector& bytes, uint8_t* bits) { } // namespace +void SetBitsTo(uint8_t* bits, int64_t start_offset, int64_t length, bool bits_are_set) { + if (length == 0) { + return; + } + + const int64_t i_begin = start_offset; + const int64_t i_end = start_offset + length; + const uint8_t fill_byte = static_cast(-static_cast(bits_are_set)); + + const int64_t bytes_begin = i_begin / 8; + const int64_t bytes_end = i_end / 8 + 1; + + const uint8_t first_byte_mask = kPrecedingBitmask[i_begin % 8]; + const uint8_t last_byte_mask = kTrailingBitmask[i_end % 8]; + + if (bytes_end == bytes_begin + 1) { + // set bits within a single byte + const uint8_t only_byte_mask = + i_end % 8 == 0 ? first_byte_mask + : static_cast(first_byte_mask | last_byte_mask); + bits[bytes_begin] &= only_byte_mask; + bits[bytes_begin] |= static_cast(fill_byte & ~only_byte_mask); + return; + } + + // set/clear trailing bits of first byte + bits[bytes_begin] &= first_byte_mask; + bits[bytes_begin] |= static_cast(fill_byte & ~first_byte_mask); + + if (bytes_end - bytes_begin > 2) { + // set/clear whole bytes + std::memset(bits + bytes_begin + 1, fill_byte, + static_cast(bytes_end - bytes_begin - 2)); + } + + if (i_end % 8 == 0) { + return; + } + + // set/clear leading bits of last byte + bits[bytes_end - 1] &= last_byte_mask; + bits[bytes_end - 1] |= static_cast(fill_byte & ~last_byte_mask); +} + Result> BytesToBits(const std::vector& bytes, MemoryPool* pool) { int64_t bit_length = BytesForBits(bytes.size()); diff --git a/cpp/src/arrow/util/bit_util.h b/cpp/src/arrow/util/bit_util.h index e708c3725be..67d3528e7b3 100644 --- a/cpp/src/arrow/util/bit_util.h +++ b/cpp/src/arrow/util/bit_util.h @@ -460,46 +460,8 @@ static inline void SetBitTo(uint8_t* bits, int64_t i, bool bit_is_set) { } /// \brief set or clear a range of bits quickly -static inline void SetBitsTo(uint8_t* bits, int64_t start_offset, int64_t length, - bool bits_are_set) { - if (length == 0) return; - - const auto i_begin = start_offset; - const auto i_end = start_offset + length; - const uint8_t fill_byte = static_cast(-static_cast(bits_are_set)); - - const auto bytes_begin = i_begin / 8; - const auto bytes_end = i_end / 8 + 1; - - const auto first_byte_mask = kPrecedingBitmask[i_begin % 8]; - const auto last_byte_mask = kTrailingBitmask[i_end % 8]; - - if (bytes_end == bytes_begin + 1) { - // set bits within a single byte - const auto only_byte_mask = - i_end % 8 == 0 ? first_byte_mask - : static_cast(first_byte_mask | last_byte_mask); - bits[bytes_begin] &= only_byte_mask; - bits[bytes_begin] |= static_cast(fill_byte & ~only_byte_mask); - return; - } - - // set/clear trailing bits of first byte - bits[bytes_begin] &= first_byte_mask; - bits[bytes_begin] |= static_cast(fill_byte & ~first_byte_mask); - - if (bytes_end - bytes_begin > 2) { - // set/clear whole bytes - std::memset(bits + bytes_begin + 1, fill_byte, - static_cast(bytes_end - bytes_begin - 2)); - } - - if (i_end % 8 == 0) return; - - // set/clear leading bits of last byte - bits[bytes_end - 1] &= last_byte_mask; - bits[bytes_end - 1] |= static_cast(fill_byte & ~last_byte_mask); -} +ARROW_EXPORT +void SetBitsTo(uint8_t* bits, int64_t start_offset, int64_t length, bool bits_are_set); /// \brief Convert vector of bytes to bitmap buffer ARROW_EXPORT diff --git a/cpp/src/arrow/util/bit_util_benchmark.cc b/cpp/src/arrow/util/bit_util_benchmark.cc index fd8855c2f6a..e1960879df7 100644 --- a/cpp/src/arrow/util/bit_util_benchmark.cc +++ b/cpp/src/arrow/util/bit_util_benchmark.cc @@ -322,6 +322,16 @@ static void VisitBitsUnrolled(benchmark::State& state) { BenchmarkVisitBits(state, state.range(0)); } +static void SetBitsTo(benchmark::State& state) { + int64_t nbytes = state.range(0); + std::shared_ptr buffer = CreateRandomBuffer(nbytes); + + for (auto _ : state) { + BitUtil::SetBitsTo(buffer->mutable_data(), /*offset=*/0, nbytes * 8, true); + } + state.SetBytesProcessed(state.iterations() * nbytes); +} + constexpr int64_t kBufferSize = 1024 * 8; template @@ -364,6 +374,7 @@ BENCHMARK(ReferenceNaiveBitmapReader)->Arg(kBufferSize); BENCHMARK(BitmapReader)->Arg(kBufferSize); BENCHMARK(VisitBits)->Arg(kBufferSize); BENCHMARK(VisitBitsUnrolled)->Arg(kBufferSize); +BENCHMARK(SetBitsTo)->Arg(2)->Arg(128)->Arg(1024); #ifdef ARROW_WITH_BENCHMARKS_REFERENCE static void ReferenceNaiveBitmapWriter(benchmark::State& state) { From 91b7926a608b013a65f4c8f8855a8eb1e4e2f5f4 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 27 May 2020 16:14:16 -0500 Subject: [PATCH 2/2] Expand benchmark --- cpp/src/arrow/util/bit_util_benchmark.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/util/bit_util_benchmark.cc b/cpp/src/arrow/util/bit_util_benchmark.cc index e1960879df7..9844c08214f 100644 --- a/cpp/src/arrow/util/bit_util_benchmark.cc +++ b/cpp/src/arrow/util/bit_util_benchmark.cc @@ -374,7 +374,7 @@ BENCHMARK(ReferenceNaiveBitmapReader)->Arg(kBufferSize); BENCHMARK(BitmapReader)->Arg(kBufferSize); BENCHMARK(VisitBits)->Arg(kBufferSize); BENCHMARK(VisitBitsUnrolled)->Arg(kBufferSize); -BENCHMARK(SetBitsTo)->Arg(2)->Arg(128)->Arg(1024); +BENCHMARK(SetBitsTo)->Arg(2)->Arg(1 << 4)->Arg(1 << 10)->Arg(1 << 17); #ifdef ARROW_WITH_BENCHMARKS_REFERENCE static void ReferenceNaiveBitmapWriter(benchmark::State& state) {