Refactor some confusing logic in PlainTableReader

Summary: Pull Request resolved: facebook#5780

Test Plan: existing plain table unit test

Differential Revision: D17368629

Pulled By: pdillinger

fbshipit-source-id: f25409cdc2f39ebe8d5cbb599cf820270e6b5d26

Author: pdillinger

Makefile

@@ -332,6 +332,11 @@ ifeq ($(USE_FOLLY_DISTRIBUTED_MUTEX),1)
 	endif
 endif
 
+ifdef TEST_CACHE_LINE_SIZE
+  PLATFORM_CCFLAGS += -DTEST_CACHE_LINE_SIZE=$(TEST_CACHE_LINE_SIZE)
+  PLATFORM_CXXFLAGS += -DTEST_CACHE_LINE_SIZE=$(TEST_CACHE_LINE_SIZE)
+endif
+
 # This (the first rule) must depend on "all".
 default: all
 

db/db_properties_test.cc

@@ -212,7 +212,8 @@ void VerifySimilar(uint64_t a, uint64_t b, double bias) {
 
 void VerifyTableProperties(const TableProperties& base_tp,
                            const TableProperties& new_tp,
-                           double filter_size_bias = 0.1,
+                           double filter_size_bias =
+                               CACHE_LINE_SIZE >= 256 ? 0.15 : 0.1,
                            double index_size_bias = 0.1,
                            double data_size_bias = 0.1,
                            double num_data_blocks_bias = 0.05) {
@@ -266,7 +267,8 @@ void GetExpectedTableProperties(
        // discount 1 byte as value size is not encoded in value delta encoding
        (value_delta_encoding ? 1 : 0));
   expected_tp->filter_size =
-      kTableCount * (kKeysPerTable * kBloomBitsPerKey / 8);
+      kTableCount * ((kKeysPerTable * kBloomBitsPerKey + 7) / 8 +
+                     /*average-ish overhead*/ CACHE_LINE_SIZE / 2);
 }
 }  // anonymous namespace
 

port/port_posix.h

@@ -178,22 +178,31 @@ typedef pthread_once_t OnceType;
 extern void InitOnce(OnceType* once, void (*initializer)());
 
 #ifndef CACHE_LINE_SIZE
-  #if defined(__s390__)
-    #define CACHE_LINE_SIZE 256U
-  #elif defined(__powerpc__) || defined(__aarch64__)
-    #define CACHE_LINE_SIZE 128U
+  // To test behavior with non-native cache line size, e.g. for
+  // Bloom filters, set TEST_CACHE_LINE_SIZE to the desired test size.
+  // This disables ALIGN_AS to keep it from failing compilation.
+  #ifdef TEST_CACHE_LINE_SIZE
+    #define CACHE_LINE_SIZE TEST_CACHE_LINE_SIZE
+    #define ALIGN_AS(n) /*empty*/
   #else
-    #define CACHE_LINE_SIZE 64U
+    #if defined(__s390__)
+      #define CACHE_LINE_SIZE 256U
+    #elif defined(__powerpc__) || defined(__aarch64__)
+      #define CACHE_LINE_SIZE 128U
+    #else
+      #define CACHE_LINE_SIZE 64U
+    #endif
+    #define ALIGN_AS(n) alignas(n)
   #endif
 #endif
 
+static_assert((CACHE_LINE_SIZE & (CACHE_LINE_SIZE - 1)) == 0,
+              "Cache line size must be a power of 2 number of bytes");
 
 extern void *cacheline_aligned_alloc(size_t size);
 
 extern void cacheline_aligned_free(void *memblock);
 
-#define ALIGN_AS(n) alignas(n)
-
 #define PREFETCH(addr, rw, locality) __builtin_prefetch(addr, rw, locality)
 
 extern void Crash(const std::string& srcfile, int srcline);

table/full_filter_bits_builder.h

@@ -20,8 +20,8 @@ class Slice;
 
 class FullFilterBitsBuilder : public FilterBitsBuilder {
  public:
-  explicit FullFilterBitsBuilder(const size_t bits_per_key,
-                                 const size_t num_probes);
+  explicit FullFilterBitsBuilder(const int bits_per_key,
+                                 const int num_probes);
 
   // No Copy allowed
   FullFilterBitsBuilder(const FullFilterBitsBuilder&) = delete;
@@ -56,8 +56,8 @@ class FullFilterBitsBuilder : public FilterBitsBuilder {
 
  private:
   friend class FullFilterBlockTest_DuplicateEntries_Test;
-  size_t bits_per_key_;
-  size_t num_probes_;
+  int bits_per_key_;
+  int num_probes_;
   std::vector<uint32_t> hash_entries_;
 
   // Get totalbits that optimized for cpu cache line

table/plain/plain_table_bloom.cc

@@ -33,9 +33,9 @@ uint32_t GetTotalBitsForLocality(uint32_t total_bits) {
 PlainTableBloomV1::PlainTableBloomV1(uint32_t num_probes)
     : kTotalBits(0), kNumBlocks(0), kNumProbes(num_probes), data_(nullptr) {}
 
-void PlainTableBloomV1::SetRawData(unsigned char* raw_data, uint32_t total_bits,
+void PlainTableBloomV1::SetRawData(char* raw_data, uint32_t total_bits,
                               uint32_t num_blocks) {
-  data_ = reinterpret_cast<uint8_t*>(raw_data);
+  data_ = raw_data;
   kTotalBits = total_bits;
   kNumBlocks = num_blocks;
 }
@@ -63,7 +63,7 @@ void PlainTableBloomV1::SetTotalBits(Allocator* allocator,
   if (kNumBlocks > 0 && cache_line_offset > 0) {
     raw += CACHE_LINE_SIZE - cache_line_offset;
   }
-  data_ = reinterpret_cast<uint8_t*>(raw);
+  data_ = raw;
 }
 
 void BloomBlockBuilder::AddKeysHashes(const std::vector<uint32_t>& keys_hashes) {

table/plain/plain_table_bloom.h

@@ -10,8 +10,11 @@
 #include "rocksdb/slice.h"
 
 #include "port/port.h"
+#include "util/bloom_impl.h"
 #include "util/hash.h"
 
+#include "third-party/folly/folly/ConstexprMath.h"
+
 #include <memory>
 
 namespace rocksdb {
@@ -51,10 +54,10 @@ class PlainTableBloomV1 {
   uint32_t GetNumBlocks() const { return kNumBlocks; }
 
   Slice GetRawData() const {
-    return Slice(reinterpret_cast<char*>(data_), GetTotalBits() / 8);
+    return Slice(data_, GetTotalBits() / 8);
   }
 
-  void SetRawData(unsigned char* raw_data, uint32_t total_bits,
+  void SetRawData(char* raw_data, uint32_t total_bits,
                   uint32_t num_blocks = 0);
 
   uint32_t GetTotalBits() const { return kTotalBits; }
@@ -66,7 +69,10 @@ class PlainTableBloomV1 {
   uint32_t kNumBlocks;
   const uint32_t kNumProbes;
 
-  uint8_t* data_;
+  char* data_;
+
+  static constexpr int LOG2_CACHE_LINE_SIZE =
+      folly::constexpr_log2(CACHE_LINE_SIZE);
 };
 
 #if defined(_MSC_VER)
@@ -76,8 +82,9 @@ class PlainTableBloomV1 {
 #endif
 inline void PlainTableBloomV1::Prefetch(uint32_t h) {
   if (kNumBlocks != 0) {
-    uint32_t b = ((h >> 11 | (h << 21)) % kNumBlocks) * (CACHE_LINE_SIZE * 8);
-    PREFETCH(&(data_[b / 8]), 0, 3);
+    uint32_t ignored;
+    LegacyLocalityBloomImpl</*ExtraRotates*/true>::PrepareHashMayMatch(
+      h, kNumBlocks, data_, &ignored, LOG2_CACHE_LINE_SIZE);
   }
 }
 #if defined(_MSC_VER)
@@ -86,54 +93,22 @@ inline void PlainTableBloomV1::Prefetch(uint32_t h) {
 
 inline bool PlainTableBloomV1::MayContainHash(uint32_t h) const {
   assert(IsInitialized());
-  const uint32_t delta = (h >> 17) | (h << 15);  // Rotate right 17 bits
   if (kNumBlocks != 0) {
-    uint32_t b = ((h >> 11 | (h << 21)) % kNumBlocks) * (CACHE_LINE_SIZE * 8);
-    for (uint32_t i = 0; i < kNumProbes; ++i) {
-      // Since CACHE_LINE_SIZE is defined as 2^n, this line will be optimized
-      //  to a simple and operation by compiler.
-      const uint32_t bitpos = b + (h % (CACHE_LINE_SIZE * 8));
-      if ((data_[bitpos / 8] & (1 << (bitpos % 8))) == 0) {
-        return false;
-      }
-      // Rotate h so that we don't reuse the same bytes.
-      h = h / (CACHE_LINE_SIZE * 8) +
-          (h % (CACHE_LINE_SIZE * 8)) * (0x20000000U / CACHE_LINE_SIZE);
-      h += delta;
-    }
+    return LegacyLocalityBloomImpl<true>::HashMayMatch(
+               h, kNumBlocks, kNumProbes, data_, LOG2_CACHE_LINE_SIZE);
   } else {
-    for (uint32_t i = 0; i < kNumProbes; ++i) {
-      const uint32_t bitpos = h % kTotalBits;
-      if ((data_[bitpos / 8] & (1 << (bitpos % 8))) == 0) {
-        return false;
-      }
-      h += delta;
-    }
+    return LegacyNoLocalityBloomImpl::HashMayMatch(
+               h, kTotalBits, kNumProbes, data_);
   }
-  return true;
 }
 
 inline void PlainTableBloomV1::AddHash(uint32_t h) {
   assert(IsInitialized());
-  const uint32_t delta = (h >> 17) | (h << 15);  // Rotate right 17 bits
   if (kNumBlocks != 0) {
-    uint32_t b = ((h >> 11 | (h << 21)) % kNumBlocks) * (CACHE_LINE_SIZE * 8);
-    for (uint32_t i = 0; i < kNumProbes; ++i) {
-      // Since CACHE_LINE_SIZE is defined as 2^n, this line will be optimized
-      // to a simple and operation by compiler.
-      const uint32_t bitpos = b + (h % (CACHE_LINE_SIZE * 8));
-      data_[bitpos / 8] |= (1 << (bitpos % 8));
-      // Rotate h so that we don't reuse the same bytes.
-      h = h / (CACHE_LINE_SIZE * 8) +
-          (h % (CACHE_LINE_SIZE * 8)) * (0x20000000U / CACHE_LINE_SIZE);
-      h += delta;
-    }
+    LegacyLocalityBloomImpl<true>::AddHash(
+        h, kNumBlocks, kNumProbes, data_, LOG2_CACHE_LINE_SIZE);
   } else {
-    for (uint32_t i = 0; i < kNumProbes; ++i) {
-      const uint32_t bitpos = h % kTotalBits;
-      data_[bitpos / 8] |= (1 << (bitpos % 8));
-      h += delta;
-    }
+    LegacyNoLocalityBloomImpl::AddHash(h, kTotalBits, kNumProbes, data_);
   }
 }
 

table/plain/plain_table_reader.cc

@@ -260,23 +260,19 @@ Status PlainTableReader::PopulateIndexRecordList(
   return s;
 }
 
-void PlainTableReader::AllocateAndFillBloom(
-    int bloom_bits_per_key, int num_prefixes, size_t huge_page_tlb_size,
-    std::vector<uint32_t>* prefix_hashes) {
-  if (!IsTotalOrderMode()) {
-    uint32_t bloom_total_bits = num_prefixes * bloom_bits_per_key;
-    if (bloom_total_bits > 0) {
-      enable_bloom_ = true;
-      bloom_.SetTotalBits(&arena_, bloom_total_bits, ioptions_.bloom_locality,
-                          huge_page_tlb_size, ioptions_.info_log);
-      FillBloom(prefix_hashes);
-    }
+void PlainTableReader::AllocateBloom(int bloom_bits_per_key, int num_keys,
+                                     size_t huge_page_tlb_size) {
+  uint32_t bloom_total_bits = num_keys * bloom_bits_per_key;
+  if (bloom_total_bits > 0) {
+    enable_bloom_ = true;
+    bloom_.SetTotalBits(&arena_, bloom_total_bits, ioptions_.bloom_locality,
+                        huge_page_tlb_size, ioptions_.info_log);
   }
 }
 
-void PlainTableReader::FillBloom(std::vector<uint32_t>* prefix_hashes) {
+void PlainTableReader::FillBloom(const std::vector<uint32_t>& prefix_hashes) {
   assert(bloom_.IsInitialized());
-  for (auto prefix_hash : *prefix_hashes) {
+  for (const auto prefix_hash : prefix_hashes) {
     bloom_.AddHash(prefix_hash);
   }
 }
@@ -354,14 +350,9 @@ Status PlainTableReader::PopulateIndex(TableProperties* props,
   if (!index_in_file) {
     // Allocate bloom filter here for total order mode.
     if (IsTotalOrderMode()) {
-      uint32_t num_bloom_bits =
-          static_cast<uint32_t>(table_properties_->num_entries) *
-          bloom_bits_per_key;
-      if (num_bloom_bits > 0) {
-        enable_bloom_ = true;
-        bloom_.SetTotalBits(&arena_, num_bloom_bits, ioptions_.bloom_locality,
-                            huge_page_tlb_size, ioptions_.info_log);
-      }
+      AllocateBloom(bloom_bits_per_key,
+                    static_cast<uint32_t>(table_properties_->num_entries),
+                    huge_page_tlb_size);
     }
   } else if (bloom_in_file) {
     enable_bloom_ = true;
@@ -377,8 +368,7 @@ Status PlainTableReader::PopulateIndex(TableProperties* props,
     }
     // cast away const qualifier, because bloom_ won't be changed
     bloom_.SetRawData(
-        const_cast<unsigned char*>(
-            reinterpret_cast<const unsigned char*>(bloom_block->data())),
+        const_cast<char*>(bloom_block->data()),
         static_cast<uint32_t>(bloom_block->size()) * 8, num_blocks);
   } else {
     // Index in file but no bloom in file. Disable bloom filter in this case.
@@ -392,6 +382,7 @@ Status PlainTableReader::PopulateIndex(TableProperties* props,
 
   std::vector<uint32_t> prefix_hashes;
   if (!index_in_file) {
+    // Populates _bloom if enabled (total order mode)
     s = PopulateIndexRecordList(&index_builder, &prefix_hashes);
     if (!s.ok()) {
       return s;
@@ -404,10 +395,15 @@ Status PlainTableReader::PopulateIndex(TableProperties* props,
   }
 
   if (!index_in_file) {
-    // Calculated bloom filter size and allocate memory for
-    // bloom filter based on the number of prefixes, then fill it.
-    AllocateAndFillBloom(bloom_bits_per_key, index_.GetNumPrefixes(),
-                         huge_page_tlb_size, &prefix_hashes);
+    if (!IsTotalOrderMode()) {
+      // Calculated bloom filter size and allocate memory for
+      // bloom filter based on the number of prefixes, then fill it.
+      AllocateBloom(bloom_bits_per_key, index_.GetNumPrefixes(),
+                    huge_page_tlb_size);
+      if (enable_bloom_) {
+        FillBloom(prefix_hashes);
+      }
+    }
   }
 
   // Fill two table properties.

table/plain/plain_table_reader.h

@@ -209,12 +209,11 @@ class PlainTableReader: public TableReader {
   Status PopulateIndexRecordList(PlainTableIndexBuilder* index_builder,
                                  std::vector<uint32_t>* prefix_hashes);
 
-  // Internal helper function to allocate memory for bloom filter and fill it
-  void AllocateAndFillBloom(int bloom_bits_per_key, int num_prefixes,
-                            size_t huge_page_tlb_size,
-                            std::vector<uint32_t>* prefix_hashes);
+  // Internal helper function to allocate memory for bloom filter
+  void AllocateBloom(int bloom_bits_per_key, int num_prefixes,
+                     size_t huge_page_tlb_size);
 
-  void FillBloom(std::vector<uint32_t>* prefix_hashes);
+  void FillBloom(const std::vector<uint32_t>& prefix_hashes);
 
   // Read the key and value at `offset` to parameters for keys, the and
   // `seekable`.

third-party/folly/folly/ConstexprMath.h

@@ -14,4 +14,32 @@ template <typename T, typename... Ts>
 constexpr T constexpr_max(T a, T b, Ts... ts) {
   return b < a ? constexpr_max(a, ts...) : constexpr_max(b, ts...);
 }
+
+namespace detail {
+template <typename T>
+constexpr T constexpr_log2_(T a, T e) {
+  return e == T(1) ? a : constexpr_log2_(a + T(1), e / T(2));
+}
+
+template <typename T>
+constexpr T constexpr_log2_ceil_(T l2, T t) {
+  return l2 + T(T(1) << l2 < t ? 1 : 0);
+}
+
+template <typename T>
+constexpr T constexpr_square_(T t) {
+  return t * t;
+}
+} // namespace detail
+
+template <typename T>
+constexpr T constexpr_log2(T t) {
+  return detail::constexpr_log2_(T(0), t);
+}
+
+template <typename T>
+constexpr T constexpr_log2_ceil(T t) {
+  return detail::constexpr_log2_ceil_(constexpr_log2(t), t);
+}
+
 } // namespace folly