Partitioned filters #1891

maysamyabandeh · 2017-02-21T16:56:39Z

Partition filter blocks and an index on the partitions. This should reduce the cost of reading filters when they do not fit into the memory.

The feature is currently depend on partitioned indexes and need that being enabled too. Partition filters can be enabled by using full filters and setting partition_filters in table options to true. Currently a filter partition is cut the same time that a index partition is cut, which is controlled by index_per_partition.

A new test is added table/partitioned_filter_block_test.cc and also db/db_bloom_filter_test.cc is updated. The code is also refactored to allow testing the partitioned filter module separately.

yiwu-arbug · 2017-02-22T23:42:46Z

Sorry for delay. Will find time to review today or tomorrow.

facebook-github-bot · 2017-02-24T16:57:45Z

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

facebook-github-bot · 2017-02-25T00:58:49Z

@maysamyabandeh updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-02-25T01:00:50Z

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

facebook-github-bot · 2017-02-27T18:32:51Z

@maysamyabandeh updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-02-27T18:33:46Z

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

yiwu-arbug

Any chance the PR can be split? It is huge and I don't know where to start reading.

yiwu-arbug · 2017-02-27T21:13:23Z

db/db_test_util.cc

@@ -496,6 +496,13 @@ Status DBTestBase::ReadOnlyReopen(const Options& options) {

 Status DBTestBase::TryReopen(const Options& options) {
  Close();
+  last_options_.table_factory.reset();


How about (in a separate PR) make the last_options_ a unique_ptr, and destruct the content before assignment?

Sure. I can do a separate patch. But I already tried the destruction approach and it causes other issues. I did not have time to look into it but apparently it would also destruct some objects that will be reused later.

yiwu-arbug · 2017-02-28T07:14:54Z

table/index_builder.h

+#include "util/crc32c.h"
+#include "util/stop_watch.h"
+#include "util/string_util.h"
+#include "util/xxhash.h"


many of these headers can probably only include in .cc file?

Sure. I will remove them.

yiwu-arbug · 2017-02-28T07:22:21Z

util/options_helper.h

@@ -625,6 +625,9 @@ static std::unordered_map<std::string, OptionTypeInfo>
        {"index_per_partition",
         {offsetof(struct BlockBasedTableOptions, index_per_partition),
          OptionType::kUInt64T, OptionVerificationType::kNormal, false, 0}},
+        {"partition_filters",


Does partitioned filters always comes with partitioned index? In that case can we have one less options, and just enable partition filter if index_type==kTwolevelIndexSearch and filter_policy != null?

First, in general they should not have two come together and the implementation should be improved by future patches (as we discussed offline) to separate them.

In the particular case that it is suggested here (index_type==kTwolevelIndexSearch and filter_policy != null) does not still mean that the filter has to be partitioned. The other way around is true though: if filter is partitioned index has to be partitioned too. But since this restriction is to be lifted in future, I think it is better to keep their configuration separately.

alright. Let's comment in table.h that partition_filters works only with kTwolevelIndexSearch right now.

yiwu-arbug · 2017-02-28T07:54:22Z

table/partitioned_filter_block_test.cc

+// A Mock of BlockBasedTable
+// The only three engaged methods are GetFilter, ReadFilter, and Open. The rest
+// are noop.
+BlockBasedTable::CachableEntry<FilterBlockReader> BlockBasedTable::GetFilter(


How's the performance different is with Mocking this way vs. with inheritance? I would prefer inheritance if the difference is negligible, since it is simpler.

It would require rigorous benchmarking which adds a lot to the cost of preparing this patch. It is much simpler and faster to go with an option that we are confident that is safe and inexpensive.

I do not see much complexity added by the current approach; it simply excludes one object file from a test binary.

I don't think there's much overhead if the mock class subclassing BlockBasedTable and make some of the methods virtual calls. I was trying very hard with the key comparator and only get 5% gain (#1928). Let's make the mocking more explicit.

I did a simple test and observed 2% reduction in throughput. I am hence inclined to avoid virtual.

TEST_TMPDIR=/dev/shm/virtualtest/ ./db_bench --benchmarks=fillrandom --num=1000000 -value_size=100 -compression_type=none -bloom_bits=10
TEST_TMPDIR=/dev/shm/virtualtest/ ./db_bench --benchmarks="readseq,readrandom[X3]" --use_existing_db --num=1000000 --reads=10000000 --cache_size=10000000000 -threads=32 -compression_type=none -duration 60 -bloom_bits=10 -disable_auto_compactions

virtual: readrandom [AVG 3 runs] : 7995501 ops/sec; 559.9 MB/sec
non : readrandom [AVG 3 runs] : 8174033 ops/sec; 572.4 MB/sec

Another way around is making PartitionedFilterBlockBuilder a template, parameterized on BlockBasedTable. This should get rid of virtual overhead, but template is something need to avoid as well.

cc @siying @IslamAbdelRahman @ajkr @lightmark adding more eyes on this. Here this patch is trying to mock BlockBasedTable to unit test PartitionedFilterBlockBuilder. Instead of inherit from BlockBasedTable, the test simply exclude table/block_based_table_reader.cc on compile (see the change to Makefile) and implement a fake BlockBasedTable that's only visible in this test. We are debating whether it worth to do this hack to avoid virtual calls to BlockBasedTable::GetFilter, which is a hot code path.

Talked to yi offline. With -O3 I still fluctuation in results and cannot confirm that avoiding virtual gave a benefit. Going to turn GetFilter virtual.

facebook-github-bot · 2017-03-02T18:47:08Z

@maysamyabandeh updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-03-02T18:52:42Z

@maysamyabandeh updated the pull request - view changes - changes since last import

yiwu-arbug · 2017-03-02T19:01:54Z

table/persistent_cache_helper.cc

 #include "table/persistent_cache_helper.h"
+#include "table/block_based_table_reader.h"


I'm not sure but would forward declaration resolve the cyclic dependency?

I tried many things until I reached this organization that works. It also seems cleaner to me compared to what we had before: persistent_cache_helper should not depend on block_based_table_reader.

yiwu-arbug

Haven't go into the detail of filter block building, but at high level, the multi-inheritance around PartitionIndexBuilder looks messy to me. Can we separate partition index builder and partition filter builder, and let them share a index builder member which builds the meta index?

yiwu-arbug · 2017-03-02T19:15:25Z

table/filter_block.h

-  virtual bool KeyMayMatch(const Slice& key,
-                           uint64_t block_offset = kNotValid) = 0;
+  virtual bool KeyMayMatch(const Slice& key, uint64_t block_offset = kNotValid,
+                           const bool no_io = false,


Add comments about what's no_io and const_ikey_ptr?

yiwu-arbug · 2017-03-02T20:13:40Z

table/partitioned_filter_block.h

+ * containing a secondary index on the partitions, built using
+ * ShortenedIndexBuilder.
+ */
+class PartitionIndexBuilder : public IndexBuilder,


If we are to keep the multi-inheritance for now, let's at least rename it to PartitionIndexAndFilterBuilder?

yiwu-arbug · 2017-03-02T20:19:51Z

table/block_based_table_builder.cc

@@ -618,7 +247,8 @@ struct BlockBasedTableBuilder::Rep {
  TableProperties props;

  bool closed = false;  // Either Finish() or Abandon() has been called.
-  std::unique_ptr<FilterBlockBuilder> filter_block;
+  std::unique_ptr<FilterBlockBuilder> filter_block_gc;
+  FilterBlockBuilder* filter_block;


rename it filter_builder?

That was not the name chosen by me. Would like to do this refactoring as part of this patch?

yes, let's update this variable name.

yiwu-arbug · 2017-03-02T20:33:51Z

table/index_builder.cc

+#include "util/xxhash.h"
+
+#include "table/index_builder.h"
+#include "table/partitioned_filter_block.h"


many of the these headers are not used. clean it up?

yiwu-arbug · 2017-03-02T20:46:10Z

table/filter_block.h

@@ -51,7 +51,12 @@ class FilterBlockBuilder {
  virtual bool IsBlockBased() = 0;                    // If is blockbased filter
  virtual void StartBlock(uint64_t block_offset) = 0;  // Start new block filter
  virtual void Add(const Slice& key) = 0;      // Add a key to current filter
-  virtual Slice Finish() = 0;                     // Generate Filter
+  inline Slice Finish() {                      // Generate Filter


nit: remove inline keyword, leave compiler to decide whether to inline.

yiwu-arbug · 2017-03-02T20:51:22Z

table/block_based_table_builder.cc

+  if (ok() && r->filter_block != nullptr) {
+    Status s;
+    Slice filter_content = r->filter_block->Finish(filter_block_handle, &s);
+    r->props.filter_size += filter_content.size();


does it initialized to 0?

yiwu-arbug · 2017-03-02T20:57:06Z

table/block_based_table_builder.cc

+    Slice filter_content = r->filter_block->Finish(filter_block_handle, &s);
+    r->props.filter_size += filter_content.size();
+    WriteRawBlock(filter_content, kNoCompression, &filter_block_handle);
+    while (s.IsIncomplete()) {


I would prefer moving line 642-644 into the while loop. Also shall we fail on non-ok non-incomplete status?

Status s = Status::Incomplete(); while (s.IsIncomplete()) { ... } if (!s.ok()) { return s; }

yiwu-arbug · 2017-03-02T21:13:50Z

table/block_based_table_reader.h

@@ -288,6 +341,115 @@ class BlockBasedTable::BlockEntryIteratorState : public TwoLevelIteratorState {
  BlockBasedTable* table_;
  const ReadOptions read_options_;
  bool skip_filters_;
+  bool is_user_data_;


Add comment on what does this mean.

Probably we shall name it is_index_?

yeah makes sense.

yiwu-arbug · 2017-03-02T21:16:32Z

table/block_based_table_reader.cc

@@ -208,10 +178,15 @@ class PartitionIndexReader : public IndexReader {
  // return a two-level iterator: first level is on the partition index
  virtual InternalIterator* NewIterator(BlockIter* iter = nullptr,
                                        bool dont_care = true) override {
+    // Filters are already checked before seeking the index
+    const bool skip_filters = true;
+    const bool is_user_data = true;


nit: the more common coding style for passing booleans is like:

new BlockBasedTable::BlockEntryIteratorState( table_, ReadOptions(), true /*skip_filters*/, false /*is_user_data*/),

yeah I noticed that but I believe mine is more natural. I can still change it back if you feel strongly about it.

sure, I'm okay with either.

yiwu-arbug · 2017-03-02T21:17:40Z

table/block_based_table_reader.cc

+            table_, ReadOptions(), skip_filters, !is_user_data),
+        index_block_->NewIterator(comparator_, nullptr, true));
+    // TODO: Update TwoLevelIterator to be able to make use of on-stack
+    // BlockIter while the state is on heap


The on-stack/on heap iterator thing still look weird to me... Does it really make any perf difference?

It would require rigorous benchmarking to be able to answer that. Even if we want to revert this approach that was introduced long time ago, I would rather do it separately from this patch.

Sure, we can leave it for future since it is not introduce in this patch. It is said that "premature optimization is the root of all evil" and I want to avoid that.

This is the first split of facebook#1891 and will be needed for the upcoming partitioned filter patch.

Summary: This is the first split of #1891 and will be needed for the upcoming partitioned filter patch. Closes #1949 Differential Revision: D4652152 Pulled By: maysamyabandeh fbshipit-source-id: 9801778

This is the second split of this pull request: facebook#1891 which includes only the builder part. The testing will be included in the third split, where the reader is also included.

Summary: This is the second split of this pull request: #1891 which includes only the builder part. The testing will be included in the third split, where the reader is also included. Closes #1952 Differential Revision: D4660272 Pulled By: maysamyabandeh fbshipit-source-id: 36b3cf0

This is the last split of this pull request: facebook#1891 which includes the reader part as well as the tests.

maysamyabandeh · 2017-03-22T16:12:32Z

The PR is split into three PRs, which are linked from here.

Summary: This is the last split of this pull request: #1891 which includes the reader part as well as the tests. Closes #1961 Differential Revision: D4672216 Pulled By: maysamyabandeh fbshipit-source-id: 6a2b829

Maysam Yabandeh added 8 commits February 21, 2017 08:36

Partitioned Filters

4868a66

Reorg IndexBuilder to separate files

3a2f97a

Reorg IndexReader to separate files

9efaf0f

adding unit test for partitioned filters

30f2b62

fix BloomFilter test

4ff72e2

fix db_bloom_filter_test

ac202f3

fix partitioned_filter_block_test

9a445f5

make format

c1f79af

maysamyabandeh requested a review from yiwu-arbug February 21, 2017 16:56

facebook-github-bot added the CLA Signed label Feb 21, 2017

fix error raised by 4.8.1 compiler

ab20185

fix valgrind complaints

6daf751

yiwu-arbug reviewed Feb 27, 2017

View reviewed changes

yiwu-arbug reviewed Feb 28, 2017

View reviewed changes

Fix report of index miss rate

1f9610c

maysamyabandeh force-pushed the partition-filter-new branch from 5ccf619 to 1f9610c Compare March 2, 2017 18:51

yiwu-arbug reviewed Mar 2, 2017

View reviewed changes

maysamyabandeh mentioned this pull request Mar 3, 2017

Refactoring #1949

Closed

maysamyabandeh pushed a commit to maysamyabandeh/rocksdb that referenced this pull request Mar 3, 2017

Refactoring

3aa6991

This is the first split of facebook#1891 and will be needed for the upcoming partitioned filter patch.

maysamyabandeh mentioned this pull request Mar 7, 2017

Builders for partition filter #1952

Closed

maysamyabandeh mentioned this pull request Mar 8, 2017

Readers for partition filter #1961

Closed

maysamyabandeh pushed a commit to maysamyabandeh/rocksdb that referenced this pull request Mar 20, 2017

Partitioned Filters: the readers

2c39ba5

This is the last split of this pull request: facebook#1891 which includes the reader part as well as the tests.

maysamyabandeh closed this Mar 22, 2017

		#include "table/persistent_cache_helper.h"
		#include "table/block_based_table_reader.h"

Partitioned filters #1891

Partitioned filters #1891

Conversation

maysamyabandeh commented Feb 21, 2017

yiwu-arbug commented Feb 22, 2017

facebook-github-bot commented Feb 24, 2017

facebook-github-bot commented Feb 25, 2017

facebook-github-bot commented Feb 25, 2017

facebook-github-bot commented Feb 27, 2017

facebook-github-bot commented Feb 27, 2017

yiwu-arbug 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

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 Mar 2, 2017

facebook-github-bot commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiwu-arbug 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysamyabandeh commented Mar 22, 2017