You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by ba...@apache.org on 2020/04/24 22:12:01 UTC
[kudu] branch master updated: [util] Don't use static function ptrs
for member functions in BlockBloom filter
This is an automated email from the ASF dual-hosted git repository.
bankim pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new 389d4f1 [util] Don't use static function ptrs for member functions in BlockBloom filter
389d4f1 is described below
commit 389d4f1e1c78bf3ddffade99eddd30f07ed982bd
Author: Bankim Bhavsar <ba...@cloudera.com>
AuthorDate: Fri Apr 17 15:27:20 2020 -0700
[util] Don't use static function ptrs for member functions in BlockBloom filter
Use of static function pointers to non-static member functions causes
crash in Kudu client in Impala test cases.
Backtrace from an instrumented build that checks for null function pointer:
https://gist.github.com/bbhavsar/1580e5e897dcd7271bad623f7da631f0
I don't understand the problem completely but it seems that static POD like
std::once_flag doesn't get destructed but static function pointers that point
to non-static member functions and are initialized just once
point to nullptr across test runs which leads to NPE.
I couldn't reproduce the problem in Kudu using similar combination of Bloom
filter predicate and range predicate values.
OrEqual functions are static whereas Insert/Find are non-static.
So this change:
- Uses non-static member function pointers for Insert/Find that are initialized
in constructor(this is same as the change before introduction of OrEqual
functions) avoiding the branch in Insert/Find.
- Doesn't use a static function pointer for OrEqual functions and checks for
AVX2 capability at run-time on every invocation.
Tests:
- No crashes with the same test run in Impala with the fix.
Change-Id: I39703fd1a7e256ff60ef86d0b370590fbb526380
Reviewed-on: http://gerrit.cloudera.org:8080/15767
Tested-by: Bankim Bhavsar <ba...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
src/kudu/util/block_bloom_filter-test.cc | 1 -
src/kudu/util/block_bloom_filter.cc | 59 ++++++++++++++++----------------
src/kudu/util/block_bloom_filter.h | 22 +++++-------
3 files changed, 37 insertions(+), 45 deletions(-)
diff --git a/src/kudu/util/block_bloom_filter-test.cc b/src/kudu/util/block_bloom_filter-test.cc
index 72974a3..3a50060 100644
--- a/src/kudu/util/block_bloom_filter-test.cc
+++ b/src/kudu/util/block_bloom_filter-test.cc
@@ -63,7 +63,6 @@ class BlockBloomFilterTest : public KuduTest {
BlockBloomFilter* CreateBloomFilter(size_t log_space_bytes) {
FLAGS_disable_blockbloomfilter_avx2 = (MakeRand() & 0x1) == 0;
- BlockBloomFilter::InitializeFunctionPtrs();
unique_ptr<BlockBloomFilter> bf(new BlockBloomFilter(allocator_));
CHECK_OK(bf->Init(log_space_bytes, FAST_HASH, 0));
diff --git a/src/kudu/util/block_bloom_filter.cc b/src/kudu/util/block_bloom_filter.cc
index 7c468da..2c80f79 100644
--- a/src/kudu/util/block_bloom_filter.cc
+++ b/src/kudu/util/block_bloom_filter.cc
@@ -24,12 +24,12 @@
#include <cmath>
#include <cstdlib>
#include <cstring>
-#include <mutex>
#include <string>
#include <gflags/gflags.h>
#include "kudu/gutil/cpu.h"
+#include "kudu/gutil/integral_types.h"
#include "kudu/gutil/strings/substitute.h"
#include "kudu/util/block_bloom_filter.pb.h"
#include "kudu/util/flag_tags.h"
@@ -45,9 +45,6 @@ DEFINE_bool(disable_blockbloomfilter_avx2, false,
"that doesn't support AVX2.");
TAG_FLAG(disable_blockbloomfilter_avx2, hidden);
-// Flag used to initialize the static function pointers for the BlockBloomFilter class.
-static std::once_flag g_init_func_ptrs_flag;
-
namespace kudu {
// Initialize the static member variables from BlockBloomFilter class.
@@ -57,42 +54,30 @@ const base::CPU BlockBloomFilter::kCpu = base::CPU();
// Hence no duplicate initialization in the definition here.
constexpr BlockBloomFilter* const BlockBloomFilter::kAlwaysTrueFilter;
-decltype(&BlockBloomFilter::BucketInsert) BlockBloomFilter::bucket_insert_func_ptr_ = nullptr;
-decltype(&BlockBloomFilter::BucketFind) BlockBloomFilter::bucket_find_func_ptr_ = nullptr;
-decltype(&BlockBloomFilter::OrEqualArrayNoAVX2) BlockBloomFilter::or_equal_array_func_ptr_ =
- nullptr;
+BlockBloomFilter::BlockBloomFilter(BlockBloomFilterBufferAllocatorIf* buffer_allocator) :
+ always_false_(true),
+ buffer_allocator_(buffer_allocator),
+ log_num_buckets_(0),
+ directory_mask_(0),
+ directory_(nullptr),
+ hash_algorithm_(UNKNOWN_HASH),
+ hash_seed_(0) {
-void BlockBloomFilter::InitializeFunctionPtrs() {
#ifdef USE_AVX2
if (has_avx2()) {
bucket_insert_func_ptr_ = &BlockBloomFilter::BucketInsertAVX2;
bucket_find_func_ptr_ = &BlockBloomFilter::BucketFindAVX2;
- or_equal_array_func_ptr_ = &BlockBloomFilter::OrEqualArrayAVX2;
} else {
bucket_insert_func_ptr_ = &BlockBloomFilter::BucketInsert;
bucket_find_func_ptr_ = &BlockBloomFilter::BucketFind;
- or_equal_array_func_ptr_ = &BlockBloomFilter::OrEqualArrayNoAVX2;
}
#else
bucket_insert_func_ptr_ = &BlockBloomFilter::BucketInsert;
bucket_find_func_ptr_ = &BlockBloomFilter::BucketFind;
- or_equal_array_func_ptr_ = &BlockBloomFilter::OrEqualArrayNoAVX2;
#endif
-}
-
-BlockBloomFilter::BlockBloomFilter(BlockBloomFilterBufferAllocatorIf* buffer_allocator) :
- always_false_(true),
- buffer_allocator_(buffer_allocator),
- log_num_buckets_(0),
- directory_mask_(0),
- directory_(nullptr),
- hash_algorithm_(UNKNOWN_HASH),
- hash_seed_(0) {
- std::call_once(g_init_func_ptrs_flag, InitializeFunctionPtrs);
DCHECK(bucket_insert_func_ptr_);
DCHECK(bucket_find_func_ptr_);
- DCHECK(or_equal_array_func_ptr_);
}
BlockBloomFilter::~BlockBloomFilter() {
@@ -260,6 +245,7 @@ void BlockBloomFilter::InsertNoAvx2(const uint32_t hash) noexcept {
void BlockBloomFilter::Insert(const uint32_t hash) noexcept {
always_false_ = false;
const uint32_t bucket_idx = Rehash32to32(hash) & directory_mask_;
+ DCHECK(bucket_insert_func_ptr_);
(this->*bucket_insert_func_ptr_)(bucket_idx, hash);
}
@@ -268,6 +254,7 @@ bool BlockBloomFilter::Find(const uint32_t hash) const noexcept {
return false;
}
const uint32_t bucket_idx = Rehash32to32(hash) & directory_mask_;
+ DCHECK(bucket_find_func_ptr_);
return (this->*bucket_find_func_ptr_)(bucket_idx, hash);
}
@@ -292,15 +279,27 @@ bool BlockBloomFilter::operator!=(const BlockBloomFilter& rhs) const {
return !(rhs == *this);
}
+void BlockBloomFilter::OrEqualArrayInternal(size_t n, const uint8_t* __restrict__ in,
+ uint8_t* __restrict__ out) {
+#ifdef USE_AVX2
+ if (has_avx2()) {
+ BlockBloomFilter::OrEqualArrayAVX2(n, in, out);
+ } else {
+ BlockBloomFilter::OrEqualArrayNoAVX2(n, in, out);
+ }
+#else
+ BlockBloomFilter::OrEqualArrayNoAVX2(n, in, out);
+#endif
+}
+
Status BlockBloomFilter::OrEqualArray(size_t n, const uint8_t* __restrict__ in,
uint8_t* __restrict__ out) {
if ((n % kBucketByteSize) != 0) {
return Status::InvalidArgument(Substitute("Input size $0 not a multiple of 32-bytes", n));
}
- std::call_once(g_init_func_ptrs_flag, InitializeFunctionPtrs);
- DCHECK(or_equal_array_func_ptr_);
- (*or_equal_array_func_ptr_)(n, in, out);
+ OrEqualArrayInternal(n, in, out);
+
return Status::OK();
}
@@ -347,9 +346,9 @@ Status BlockBloomFilter::Or(const BlockBloomFilter& other) {
return Status::OK();
}
- (*or_equal_array_func_ptr_)(directory_size(),
- reinterpret_cast<uint8_t*>(other.directory_),
- reinterpret_cast<uint8_t*>(directory_));
+ OrEqualArrayInternal(directory_size(), reinterpret_cast<const uint8*>(other.directory_),
+ reinterpret_cast<uint8*>(directory_));
+
always_false_ = false;
return Status::OK();
}
diff --git a/src/kudu/util/block_bloom_filter.h b/src/kudu/util/block_bloom_filter.h
index 35b2166..c14f3de 100644
--- a/src/kudu/util/block_bloom_filter.h
+++ b/src/kudu/util/block_bloom_filter.h
@@ -241,6 +241,10 @@ class BlockBloomFilter {
// operations.
static void OrEqualArrayNoAVX2(size_t n, const uint8_t* __restrict__ in,
uint8_t* __restrict__ out);
+ // Helper function for OrEqualArray functions that encapsulates AVX2 v/s non-AVX2 logic to
+ // invoke the right function.
+ static void OrEqualArrayInternal(size_t n, const uint8_t* __restrict__ in,
+ uint8_t* __restrict__ out);
#ifdef USE_AVX2
// Same as Insert(), but skips the CPU check and assumes that AVX2 is available.
@@ -260,16 +264,10 @@ class BlockBloomFilter {
uint8_t* __restrict__ out) __attribute__((target("avx2")));
#endif
- // Function pointers initialized just once to avoid run-time cost
- // in hot-path of Find, Insert and Or operations.
- static decltype(&BlockBloomFilter::BucketInsert) bucket_insert_func_ptr_;
- static decltype(&BlockBloomFilter::BucketFind) bucket_find_func_ptr_;
- static decltype(&BlockBloomFilter::OrEqualArrayNoAVX2) or_equal_array_func_ptr_;
-
- // Helper function to initialize the function pointers above based on whether
- // compiler and/or CPU at run-time supports AVX2.
- // It also helps testing both AVX2 and non-AVX2 code paths.
- static void InitializeFunctionPtrs();
+ // Function pointers initialized in the constructor to avoid run-time cost in hot-path
+ // of Find and Insert operations.
+ decltype(&BlockBloomFilter::BucketInsert) bucket_insert_func_ptr_;
+ decltype(&BlockBloomFilter::BucketFind) bucket_find_func_ptr_;
// Size of the internal directory structure in bytes.
int64_t directory_size() const {
@@ -305,10 +303,6 @@ class BlockBloomFilter {
}
DISALLOW_COPY_AND_ASSIGN(BlockBloomFilter);
-
- // Allow BlockBloomFilterTest unit test to invoke the private InitializeFunctionPtrs()
- // function to test both AVX2 and non-AVX2 code paths.
- friend class BlockBloomFilterTest;
};
// Generic interface to allocate and de-allocate memory for the BlockBloomFilter.