New Bloom filter implementation for full and partitioned filters #6007

pdillinger · 2019-11-04T01:07:10Z

Summary: Adds an improved, replacement Bloom filter implementation (FastLocalBloom) for full and partitioned filters in the block-based table. This replacement is faster and more accurate, especially for high bits per key or millions of keys in a single filter.

Speed

The improved speed, at least on recent x86_64, comes from

Using fastrange instead of modulo (%)
Using our new hash function (XXH3 preview, added in a previous commit), which is much faster for large keys and only slightly slower on keys around 12 bytes if hashing the same size many thousands of times in a row.
Optimizing the Bloom filter queries with AVX2 SIMD operations. (Added AVX2 to the USE_SSE=1 build.) Careful design was required to support (a) SIMD-optimized queries, (b) compatible non-SIMD code that's simple and efficient, (c) flexible choice of number of probes, and (d) essentially maximized accuracy for a cache-local Bloom filter. Probes are made eight at a time, so any number of probes up to 8 is the same speed, then up to 16, etc.
Prefetching cache lines when building the filter. Although this optimization could be applied to the old structure as well, it seems to balance out the small added cost of accumulating 64 bit hashes for adding to the filter rather than 32 bit hashes.

Here's nominal speed data from filter_bench (200MB in filters, about 10k keys each, 10 bits filter data / key, 6 probes, avg key size 24 bytes, includes hashing time) on Skylake DE (relatively low clock speed):

$ ./filter_bench -quick -impl=2 -net_includes_hashing # New Bloom filter
Build avg ns/key: 47.7135
Mixed inside/outside queries...
Single filter net ns/op: 26.2825
Random filter net ns/op: 150.459
Average FP rate %: 0.954651
$ ./filter_bench -quick -impl=0 -net_includes_hashing # Old Bloom filter
Build avg ns/key: 47.2245
Mixed inside/outside queries...
Single filter net ns/op: 63.2978
Random filter net ns/op: 188.038
Average FP rate %: 1.13823

Similar build time but dramatically faster query times on hot data (63 ns to 26 ns), and somewhat faster on stale data (188 ns to 150 ns). Performance differences on batched and skewed query loads are between these extremes as expected.

The only other interesting thing about speed is "inside" (query key was added to filter) vs. "outside" (query key was not added to filter) query times. The non-SIMD implementations are substantially slower when most queries are "outside" vs. "inside". This goes against what one might expect or would have observed years ago, as "outside" queries only need about two probes on average, due to short-circuiting, while "inside" always have num_probes (say 6). The problem is probably the nastily unpredictable branch. The SIMD implementation has few branches (very predictable) and has pretty consistent running time regardless of query outcome.

Accuracy

The generally improved accuracy (re: Issue #5857) comes from a better design for probing indices
within a cache line (re: Issue #4120) and improved accuracy for millions of keys in a single filter from using a 64-bit hash function (XXH3p). Design details in code comments.

Accuracy data (generalizes, except old impl gets worse with millions of keys):
Memory bits per key: FP rate percent old impl -> FP rate percent new impl
6: 5.70953 -> 5.69888
8: 2.45766 -> 2.29709
10: 1.13977 -> 0.959254
12: 0.662498 -> 0.411593
16: 0.353023 -> 0.0873754
24: 0.261552 -> 0.0060971
50: 0.225453 -> ~0.00003 (less than 1 in a million queries are FP)

Fixes #5857
Fixes #4120

Unlike the old implementation, this implementation has a fixed cache line size (64 bytes). At 10 bits per key, the accuracy of this new implementation is very close to the old implementation with 128-byte cache line size. If there's sufficient demand, this implementation could be generalized.

Compatibility

Although old releases would see the new structure as corrupt filter data and read the table as if there's no filter, we've decided only to enable the new Bloom filter with new format_version=5. This provides a smooth path for automatic adoption over time, with an option for early opt-in.

Test plan: filter_bench has been used thoroughly to validate speed, accuracy, and correctness. Unit tests have been carefully updated to exercise new and old implementations, as well as the logic to select an implementation based on context (format_version).

facebook-github-bot

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Add third-party/folly to includes (for buck build) so that folly headers can include each other with #include <folly/Whatever.h>

facebook-github-bot · 2019-11-04T19:07:37Z

@pdillinger has updated the pull request. Re-import the pull request

facebook-github-bot

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Because for example folly::Optional uses exceptions

facebook-github-bot · 2019-11-04T22:11:39Z

@pdillinger has updated the pull request. Re-import the pull request

facebook-github-bot

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-11-05T16:52:22Z

@pdillinger has updated the pull request. Re-import the pull request

facebook-github-bot

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-11-05T19:26:03Z

@pdillinger has updated the pull request. Re-import the pull request

facebook-github-bot

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying

It's awesome. I left some comments. The most critical one is to add regression tests to assert the stability of the format. Also, as always, it's better to think about how to add comments to make it more understandable.

siying · 2019-11-11T20:41:27Z

table/block_based/filter_policy.cc

@@ -186,6 +189,163 @@ inline void FullFilterBitsBuilder::AddHash(uint32_t h, char* data,
                                folly::constexpr_log2(CACHE_LINE_SIZE));
 }

+// See description in FastLocalBloomImpl
+class FastLocalBloomBitsBuilder : public BuiltinFilterBitsBuilder {


Is it Is there a way to move it to the beginning of the file, or even better, a separate file? I think having legacy code in the beginning is less intuitive for maintenance.

siying · 2019-11-11T20:49:01Z

table/block_based/filter_policy.cc

+    }
+  }
+  // otherwise
+  // Reserved / future safe


Can you draw a graph to illustrate it?

siying · 2019-11-11T20:51:28Z

util/bloom_impl.h

@@ -135,4 +140,192 @@ class LegacyLocalityBloomImpl {
  }
 };

+// A fast, flexible, and accurate cache-local Bloom implementation with


Same comment here, can we move it to a separate file? The second choice is to move it ahead to before legacy data structure.

siying · 2019-11-11T21:08:54Z

util/bloom_impl.h

+    for (int i = 0; i < num_probes; ++i, h *= uint32_t{0x9e3779b9}) {
+      // 9-bit address within 512 bit cache line
+      int bitpos = h >> (32 - 9);
+      data_at_cache_line[bitpos >> 3] |= (uint8_t(1) << (bitpos & 7));


nit: uint8_1{1}

siying · 2019-11-11T21:12:26Z

table/block_based/filter_policy.cc

+      FastLocalBloomImpl::PrepareHash(Lower32of64(h), len_bytes_, data_,
+                                      /*out*/ &byte_offsets[i]);
+      hashes[i] = Upper32of64(h);
+    }


Just a question for future: is there a chance that this for-loop is optimized with SIMD? Of course we don't have to do it. Just curious.

Unless you could compute several hashes in parallel with SIMD (unlikely with a complex hash function and/or variable size keys; questionable speed increase), you wouldn't want to. As soon as you have a key's hash, you want to prefetch memory for it. This way we can pipeline hash computation on top of memory fetching and possibly hide all the memory latency. If you had 16 keys and did all the memory fetches with _mm512_prefetch_i32gather_ps, the latency for the first memory access would come after the last hash computation.

util/bloom_impl.h

siying · 2019-11-11T22:23:38Z

util/bloom_impl.h

+        return match;
+      } else if (!match) {
+        return false;
+      }


nit: do you know what compilers would compile the code too? If there is still branching after that, we may be able to remove the branching here.

siying · 2019-11-11T22:49:31Z

util/bloom_impl.h

+      // Strip off the 4 bit word address (shift left)
+      __m256i bit_addresses = _mm256_slli_epi32(hash_vector, 4);
+      // And keep only 5-bit (32 - 27) bit-within-32-bit-word addresses.
+      bit_addresses = _mm256_srli_epi32(bit_addresses, 27);


It may be helpful to illustrate the parts of bits like this:

+----------------------------------------------------+ | | | | | a | b | ... | | | | | +----------------------------------------------------+ | | | +<-+ 4 bits +->+ <-+ 5 bits +--->+ + + +

And reference which part we are getting in each part of the function.

I think that's overkill, if not misleading, for transient data (vs. serialized data). I'll add a couple more clarifying comments, though.

siying · 2019-11-11T23:10:34Z

util/bloom_test.cc

+  EXPECT_EQ(
+      BloomHash(FilterData()),
+      SelectByImpl(SelectByCacheLineSize(2885052954U, 769447944, 4175124908U),
+                   23699164));


I think it makes sense to have some test cases where the actual bits are validated. This is helpful for other developers to validate it in another environment, and for future developers to make sure the changed algorithms are stable.

You can ignore the comment if it is already done.

facebook-github-bot · 2019-11-13T00:26:09Z

@pdillinger has updated the pull request. Re-import the pull request

Minor clean up.

facebook-github-bot · 2019-11-13T21:45:46Z

@pdillinger has updated the pull request. Re-import the pull request

facebook-github-bot

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-11-14T02:09:18Z

@pdillinger merged this pull request in f059c7d.

Summary: Had complications with LITE build and valgrind test. Reverts/fixes small parts of PR facebook#6007 Test Plan: make LITE=1 all check and ROCKSDB_VALGRIND_RUN=1 DISABLE_JEMALLOC=1 make -j24 db_bloom_filter_test && ROCKSDB_VALGRIND_RUN=1 DISABLE_JEMALLOC=1 ./db_bloom_filter_test

Summary: Had complications with LITE build and valgrind test. Reverts/fixes small parts of PR #6007 Pull Request resolved: #6036 Test Plan: make LITE=1 all check and ROCKSDB_VALGRIND_RUN=1 DISABLE_JEMALLOC=1 make -j24 db_bloom_filter_test && ROCKSDB_VALGRIND_RUN=1 DISABLE_JEMALLOC=1 ./db_bloom_filter_test Differential Revision: D18512238 Pulled By: pdillinger fbshipit-source-id: 37213cf0d309edf11c483fb4b2fb6c02c2cf2b28

…ebook#6007) Summary: Adds an improved, replacement Bloom filter implementation (FastLocalBloom) for full and partitioned filters in the block-based table. This replacement is faster and more accurate, especially for high bits per key or millions of keys in a single filter. Speed The improved speed, at least on recent x86_64, comes from * Using fastrange instead of modulo (%) * Using our new hash function (XXH3 preview, added in a previous commit), which is much faster for large keys and only *slightly* slower on keys around 12 bytes if hashing the same size many thousands of times in a row. * Optimizing the Bloom filter queries with AVX2 SIMD operations. (Added AVX2 to the USE_SSE=1 build.) Careful design was required to support (a) SIMD-optimized queries, (b) compatible non-SIMD code that's simple and efficient, (c) flexible choice of number of probes, and (d) essentially maximized accuracy for a cache-local Bloom filter. Probes are made eight at a time, so any number of probes up to 8 is the same speed, then up to 16, etc. * Prefetching cache lines when building the filter. Although this optimization could be applied to the old structure as well, it seems to balance out the small added cost of accumulating 64 bit hashes for adding to the filter rather than 32 bit hashes. Here's nominal speed data from filter_bench (200MB in filters, about 10k keys each, 10 bits filter data / key, 6 probes, avg key size 24 bytes, includes hashing time) on Skylake DE (relatively low clock speed): $ ./filter_bench -quick -impl=2 -net_includes_hashing # New Bloom filter Build avg ns/key: 47.7135 Mixed inside/outside queries... Single filter net ns/op: 26.2825 Random filter net ns/op: 150.459 Average FP rate %: 0.954651 $ ./filter_bench -quick -impl=0 -net_includes_hashing # Old Bloom filter Build avg ns/key: 47.2245 Mixed inside/outside queries... Single filter net ns/op: 63.2978 Random filter net ns/op: 188.038 Average FP rate %: 1.13823 Similar build time but dramatically faster query times on hot data (63 ns to 26 ns), and somewhat faster on stale data (188 ns to 150 ns). Performance differences on batched and skewed query loads are between these extremes as expected. The only other interesting thing about speed is "inside" (query key was added to filter) vs. "outside" (query key was not added to filter) query times. The non-SIMD implementations are substantially slower when most queries are "outside" vs. "inside". This goes against what one might expect or would have observed years ago, as "outside" queries only need about two probes on average, due to short-circuiting, while "inside" always have num_probes (say 6). The problem is probably the nastily unpredictable branch. The SIMD implementation has few branches (very predictable) and has pretty consistent running time regardless of query outcome. Accuracy The generally improved accuracy (re: Issue facebook#5857) comes from a better design for probing indices within a cache line (re: Issue facebook#4120) and improved accuracy for millions of keys in a single filter from using a 64-bit hash function (XXH3p). Design details in code comments. Accuracy data (generalizes, except old impl gets worse with millions of keys): Memory bits per key: FP rate percent old impl -> FP rate percent new impl 6: 5.70953 -> 5.69888 8: 2.45766 -> 2.29709 10: 1.13977 -> 0.959254 12: 0.662498 -> 0.411593 16: 0.353023 -> 0.0873754 24: 0.261552 -> 0.0060971 50: 0.225453 -> ~0.00003 (less than 1 in a million queries are FP) Fixes facebook#5857 Fixes facebook#4120 Unlike the old implementation, this implementation has a fixed cache line size (64 bytes). At 10 bits per key, the accuracy of this new implementation is very close to the old implementation with 128-byte cache line size. If there's sufficient demand, this implementation could be generalized. Compatibility Although old releases would see the new structure as corrupt filter data and read the table as if there's no filter, we've decided only to enable the new Bloom filter with new format_version=5. This provides a smooth path for automatic adoption over time, with an option for early opt-in. Pull Request resolved: facebook#6007 Test Plan: filter_bench has been used thoroughly to validate speed, accuracy, and correctness. Unit tests have been carefully updated to exercise new and old implementations, as well as the logic to select an implementation based on context (format_version). Differential Revision: D18294749 Pulled By: pdillinger fbshipit-source-id: d44c9db3696e4d0a17caaec47075b7755c262c5f

Summary: Had complications with LITE build and valgrind test. Reverts/fixes small parts of PR facebook#6007 Pull Request resolved: facebook#6036 Test Plan: make LITE=1 all check and ROCKSDB_VALGRIND_RUN=1 DISABLE_JEMALLOC=1 make -j24 db_bloom_filter_test && ROCKSDB_VALGRIND_RUN=1 DISABLE_JEMALLOC=1 ./db_bloom_filter_test Differential Revision: D18512238 Pulled By: pdillinger fbshipit-source-id: 37213cf0d309edf11c483fb4b2fb6c02c2cf2b28

FastLocalBloom

1aedfb4

facebook-github-bot added the CLA Signed label Nov 4, 2019

Oops, don't update default format_version

af51860

facebook-github-bot reviewed Nov 4, 2019

View reviewed changes

Include third-party/folly

bead81a

Summary: Add third-party/folly to includes (for buck build) so that folly headers can include each other with #include <folly/Whatever.h>

facebook-github-bot reviewed Nov 4, 2019

View reviewed changes

Don't add folly include dir for LITE build

010a3ce

Because for example folly::Optional uses exceptions

facebook-github-bot reviewed Nov 4, 2019

View reviewed changes

pdillinger added 2 commits November 5, 2019 08:04

Fix lints

3fc600a

Batch new Bloom filter adds, for speed

bade739

facebook-github-bot reviewed Nov 5, 2019

View reviewed changes

Fix handling of num_entries

28893e1

facebook-github-bot reviewed Nov 5, 2019

View reviewed changes

Format code

0a527db

pdillinger requested a review from siying November 5, 2019 20:07

siying approved these changes Nov 11, 2019

View reviewed changes

Merge in 'origin/master' for PR facebook#6024

48148ec

pdillinger added 2 commits November 13, 2019 11:40

More comments and moving code around.

69ccafb

Minor clean up.

Expand testing

31c7980

facebook-github-bot reviewed Nov 13, 2019

View reviewed changes

facebook-github-bot closed this in f059c7d Nov 14, 2019

facebook-github-bot added the Merged label Nov 14, 2019

pdillinger mentioned this pull request Nov 14, 2019

Abandon use of folly::Optional #6036

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Bloom filter implementation for full and partitioned filters #6007

New Bloom filter implementation for full and partitioned filters #6007

pdillinger commented Nov 4, 2019 •

edited

Loading

facebook-github-bot left a comment

facebook-github-bot commented Nov 4, 2019

facebook-github-bot left a comment

facebook-github-bot commented Nov 4, 2019

facebook-github-bot left a comment

facebook-github-bot commented Nov 5, 2019

facebook-github-bot left a comment

facebook-github-bot commented Nov 5, 2019

facebook-github-bot left a comment

siying left a comment

siying Nov 11, 2019

siying Nov 11, 2019

siying Nov 11, 2019

siying Nov 11, 2019

siying Nov 11, 2019

pdillinger Nov 12, 2019

siying Nov 11, 2019

siying Nov 11, 2019

pdillinger Nov 13, 2019

siying Nov 11, 2019

facebook-github-bot commented Nov 13, 2019

facebook-github-bot commented Nov 13, 2019

facebook-github-bot left a comment

facebook-github-bot commented Nov 14, 2019

New Bloom filter implementation for full and partitioned filters #6007

New Bloom filter implementation for full and partitioned filters #6007

Conversation

pdillinger commented Nov 4, 2019 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 4, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 4, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 5, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 5, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 13, 2019

facebook-github-bot commented Nov 13, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 14, 2019

pdillinger commented Nov 4, 2019 •

edited

Loading