Refactor / clean up / optimize FullFilterBitsReader

[pdillinger]

Summary: FullFilterBitsReader, after creating in BloomFilterPolicy, was
responsible for decoding metadata bits. This meant that
FullFilterBitsReader::MayMatch had some metadata checks in order to
implement "always true" or "always false" functionality in the case
of inconsistent or trivial metadata. This made for ugly
mixing-of-concerns code and probably had some runtime cost. It also
didn't really support plugging in alternative filter implementations
with extensions to the existing metadata schema.

BloomFilterPolicy::GetFilterBitsReader is now (exclusively) responsible
for decoding filter metadata bits and constructing appropriate instances
deriving from FilterBitsReader. "Always false" and "always true" derived
classes allow FullFilterBitsReader not to be concerned with handling of
trivial or inconsistent metadata. This also makes for easy expansion
to alternative filter implementations in new, alternative derived
classes. This change makes calls to FilterBitsReader::MayMatch
necessarily virtual because there's now more than one built-in
implementation. Compared with the previous implementation's extra
'if' checks in MayMatch, there's no consistent performance difference,
measured by (an older revision of) filter_bench (differences here seem
to be within noise):

Inside queries...
-  Dry run (407) ns/op: 35.9996
+  Dry run (407) ns/op: 35.2034
-  Single filter ns/op: 47.5483
+  Single filter ns/op: 47.4034
-  Batched, prepared ns/op: 43.1559
+  Batched, prepared ns/op: 42.2923
...
-  Random filter ns/op: 150.697
+  Random filter ns/op: 149.403
----------------------------
Outside queries...
-  Dry run (980) ns/op: 34.6114
+  Dry run (980) ns/op: 34.0405
-  Single filter ns/op: 56.8326
+  Single filter ns/op: 55.8414
-  Batched, prepared ns/op: 48.2346
+  Batched, prepared ns/op: 47.5667
-  Random filter ns/op: 155.377
+  Random filter ns/op: 153.942
     Average FP rate %: 1.1386

Also, the FullFilterBitsReader ctor was responsible for a surprising
amount of CPU in production, due in part to inefficient determination of
the CACHE_LINE_SIZE used to construct the filter being read. The
overwhelming common case (same as my CACHE_LINE_SIZE) is now
substantially optimized, as shown with filter_bench with
-new_reader_every=1 (old option - see below) (repeatable result):

Inside queries...
-  Dry run (453) ns/op: 118.799
+  Dry run (453) ns/op: 105.869
-  Single filter ns/op: 82.5831
+  Single filter ns/op: 74.2509
...
-  Random filter ns/op: 224.936
+  Random filter ns/op: 194.833
----------------------------
Outside queries...
-  Dry run (aa1) ns/op: 118.503
+  Dry run (aa1) ns/op: 104.925
-  Single filter ns/op: 90.3023
+  Single filter ns/op: 83.425
...
-  Random filter ns/op: 220.455
+  Random filter ns/op: 175.7
     Average FP rate %: 1.13886

However PR#5936 has/will reclaim most of this cost. After that PR, the optimization of this code path is likely negligible, but nonetheless it's clear we aren't making performance any worse.

Also fixed inadequate check of consistency between filter data size and
num_lines. (Unit test updated.)

Test Plan: previously added unit tests FullBloomTest.CorruptFilters and
FullBloomTest.RawSchema

[siying]

Good change!

[facebook-github-bot]

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

[facebook-github-bot]

@pdillinger merged this pull request in 5f8f2fd.