Readers for partition filter #1961

maysamyabandeh · 2017-03-08T02:51:13Z

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

facebook-github-bot · 2017-03-08T02:54:51Z

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

facebook-github-bot · 2017-03-08T19:33:22Z

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

facebook-github-bot · 2017-03-08T19:41:18Z

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

facebook-github-bot · 2017-03-15T20:45:22Z

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

yiwu-arbug

Sorry for delay! Haven't finish reading, but here's some comments and questions.

yiwu-arbug · 2017-03-17T20:34:43Z

table/filter_block.h

  virtual size_t ApproximateMemoryUsage() const = 0;
  virtual size_t size() const { return size_; }
  virtual Statistics* statistics() const { return statistics_; }

  bool whole_key_filtering() const { return whole_key_filtering_; }

+  inline int GetLevel() const { return level_; };


nit: remove inline keyword?

yiwu-arbug · 2017-03-17T20:40:46Z

table/partitioned_filter_block.h

+  Slice GetFilterPartitionHandle(const Slice& entry);
+  BlockBasedTable::CachableEntry<FilterBlockReader> GetFilterPartition(
+      Slice* handle, const bool no_io, bool* cached);
+  std::map<uint64_t, FilterBlockReader*> filter_cache;


nit: private member names should end with underscore.

use std::unordered_map instead?

yiwu-arbug · 2017-03-17T20:45:03Z

table/partitioned_filter_block.cc

+                                   kDisableGlobalSequenceNumber,
+                                   0 /* read_amp_bytes_per_bit */, stats));
+  BlockIter iter;
+  idx_on_fltr_blk_->NewIterator(&comparator_, &iter, true);


What's this doing?

must be a mistake. will remote it.

yiwu-arbug · 2017-03-17T20:59:15Z

table/partitioned_filter_block.h

+  std::unique_ptr<Block> idx_on_fltr_blk_;
+  const Comparator& comparator_;
+  const BlockBasedTable* table_;
+  Slice GetFilterPartitionHandle(const Slice& entry);


nit: move methods to before member variables?

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

table/partitioned_filter_block.cc

+  if (cached) {
+    return res;
+  }
+  if (LIKELY(filter_partition.IsSet())) {


why release the cached block?

a block cache entries should always be released unless we want to pin it. here we have no intention to pin all the filter partitions in the block cache.

I see. I don't know why I confuse between release and erase.

yiwu-arbug · 2017-03-17T21:08:49Z

table/partitioned_filter_block.cc

+    const Slice* const const_ikey_ptr) {
+  assert(const_ikey_ptr != nullptr);
+  assert(block_offset == kNotValid);
+  if (!prefix_extractor_) {


where is the prefix extractor being use? Do we only need a boolean flag?

this is based on the existing impl in the full filter block: https://github.com/facebook/rocksdb/blob/master/table/full_filter_block.cc#L79

But in full filter block the prefix extractor is actually being use. I'm not familiar with prefix match, so my question is, do you intent to let partitioned filter support prefix match, if so how is it being support right now?

Yes. Partitioned Filter must be able to all that full filter does. FullFilter does it by implementing PrefixMayMatch, so does partitioned filter. The impl of full filter assumes that prefix has been added at the build time just like a normal key, so does Partitioned Filter.

yiwu-arbug · 2017-03-17T21:47:19Z

table/partitioned_filter_block.h

+  BlockBasedTable::CachableEntry<FilterBlockReader> GetFilterPartition(
+      Slice* handle, const bool no_io, bool* cached);
+  std::map<uint64_t, FilterBlockReader*> filter_cache;
+  std::vector<Cache::Handle*> handle_list;


you can also consider using rocksdb::autovector

yiwu-arbug · 2017-03-17T21:53:05Z

table/partitioned_filter_block.cc

+    auto filter =
+        table_->GetFilter(fltr_blk_handle, is_a_filter_partition, no_io);
+    if (pin_cached_filters && filter.IsSet()) {
+      WriteLock wl(&mu);


Seems to me it is possible some other thread may cache the filter before we hold write lock. Should we double check if it is cached after holding write lock?

it would not cause a correctness issue. the filter_cache would simply overwrite the value. due to low probability of this happening it is not a performance issue either. but it would be clear if we do the extra check.

yiwu-arbug

Look good to me overall. Will accept after comments are addressed.

yiwu-arbug · 2017-03-18T07:12:25Z

table/block_based_table_reader.cc

@@ -1000,8 +1083,7 @@ BlockBasedTable::CachableEntry<FilterBlockReader> BlockBasedTable::GetFilter(
    return {nullptr /* filter */, nullptr /* cache handle */};
  }

-  // we have a pinned filter block
-  if (rep_->filter_entry.IsSet()) {
+  if (!is_a_filter_partition && rep_->filter_entry.IsSet()) {


Should this go before line 1080? This branch don't depend on block cache.

filter_entry refers to an entry in the block cache:

CachableEntry<FilterBlockReader> filter_entry;

yiwu-arbug · 2017-03-18T07:20:59Z

table/partitioned_filter_block.cc

+  if (cached) {
+    return res;
+  }
+  if (LIKELY(filter_partition.IsSet())) {


I see. I don't know why I confuse between release and erase.

yiwu-arbug · 2017-03-18T07:24:32Z

table/partitioned_filter_block.cc

+    const Slice* const const_ikey_ptr) {
+  assert(const_ikey_ptr != nullptr);
+  assert(block_offset == kNotValid);
+  if (!prefix_extractor_) {


But in full filter block the prefix extractor is actually being use. I'm not familiar with prefix match, so my question is, do you intent to let partitioned filter support prefix match, if so how is it being support right now?

yiwu-arbug · 2017-03-18T07:32:01Z

table/partitioned_filter_block_test.cc

+    cache_ = NewLRUCache(1, 1, false);
+    table_options_.block_cache = cache_;
+    table_options_.filter_policy.reset(NewBloomFilterPolicy(10, false));
+    table_options_.no_block_cache = true;  // Otherwise BlockBasedTable::Close


why are you setting block cache (line 40-41) if you don't want it.

good catch. will remove it.

yiwu-arbug · 2017-03-18T07:37:03Z

table/partitioned_filter_block_test.cc

+    builder->Add(keys[i]);
+    CutABlock(pib.get(), keys[i], keys[i + 1]);
+    i++;
+    builder->Add(keys[i]);


what's the purpose of adding same key twice?

i do not have a particular bug in mind but if the impl of FullFilterBitsBuilder does check for two consecutive duplicate keys so partitioned filter should also be resilient against this case.

facebook-github-bot · 2017-03-20T17:44:24Z

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

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

facebook-github-bot · 2017-03-20T17:49:03Z

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

facebook-github-bot · 2017-03-20T18:13:20Z

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

facebook-github-bot · 2017-03-21T17:49:51Z

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

facebook-github-bot · 2017-03-21T18:41:37Z

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

facebook-github-bot · 2017-03-22T00:41:24Z

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

facebook-github-bot · 2017-03-22T00:43:03Z

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

ajkr · 2017-03-23T18:45:58Z

table/block_based_table_reader.cc

@@ -697,6 +732,7 @@ Status BlockBasedTable::Open(const ImmutableCFOptions& ioptions,
        if (rep->table_options.pin_l0_filter_and_index_blocks_in_cache &&
            level == 0) {
          rep->filter_entry = filter_entry;
+          rep->filter_entry.value->SetLevel(level);


this causes a segfault when filter_policy is nullptr and both pin_l0_filter_and_index_blocks_in_cache/cache_index_and_filter_blocks are set.

repro:
TEST_TMPDIR=/dev/shm/ ./db_bench -benchmarks=fillrandom -pin_l0_filter_and_index_blocks_in_cache=true -cache_index_and_filter_blocks=true

Summary: Fixes facebook#1961 which causes a segfault when filter_policy is nullptr and both pin_l0_filter_and_index_blocks_in_cache/cache_index_and_filter_blocks are set. Test Plan: TEST_TMPDIR=/data/rate_limit_bench/ ./db_bench -benchmarks=fillrandom -pin_l0_filter_and_index_blocks_in_cache=true -cache_index_and_filter_blocks=true Reviewers: Subscribers: Tasks: Tags: Blame Revision:

Summary: Fixes #1961 which causes a segfault when filter_policy is nullptr and both pin_l0_filter_and_index_blocks_in_cache/cache_index_and_filter_blocks are set. Closes #2029 Differential Revision: D4764862 Pulled By: maysamyabandeh fbshipit-source-id: 05bd695

facebook-github-bot added the CLA Signed label Mar 8, 2017

maysamyabandeh requested a review from yiwu-arbug March 8, 2017 02:52

yiwu-arbug reviewed Mar 17, 2017

View reviewed changes

yiwu-arbug reviewed Mar 18, 2017

View reviewed changes

Maysam Yabandeh added 7 commits March 20, 2017 10:47

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.

Use inheritance for mocking BlockBasedTable

77b822e

apply comments

46ad12f

extend pin_l0 config to partitioned index/filters

b3e46b9

fix concurrency issue with pin_l0

9d676df

Store subindexes with high pri

6051f70

apply comments

c2b25b5

maysamyabandeh force-pushed the readers-for-partition-filter branch from 40f7b1f to c2b25b5 Compare March 20, 2017 17:48

maysamyabandeh force-pushed the readers-for-partition-filter branch from 08991eb to 7c6c318 Compare March 21, 2017 18:41

Fixing the flaky unit test

9e1ef29

maysamyabandeh force-pushed the readers-for-partition-filter branch from 7c6c318 to 9e1ef29 Compare March 22, 2017 00:40

yiwu-arbug approved these changes Mar 22, 2017

View reviewed changes

facebook-github-bot closed this in 8b0097b Mar 22, 2017

ajkr reviewed Mar 23, 2017

View reviewed changes

maysamyabandeh mentioned this pull request Mar 23, 2017

Fix segmentation fault caused by #1961 #2029

Closed

Readers for partition filter #1961

Readers for partition filter #1961

Conversation

maysamyabandeh commented Mar 8, 2017

facebook-github-bot commented Mar 8, 2017

facebook-github-bot commented Mar 8, 2017

facebook-github-bot commented Mar 8, 2017

facebook-github-bot commented Mar 15, 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

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

facebook-github-bot commented Mar 20, 2017

facebook-github-bot commented Mar 20, 2017

facebook-github-bot commented Mar 20, 2017

facebook-github-bot commented Mar 21, 2017

facebook-github-bot commented Mar 21, 2017

facebook-github-bot commented Mar 22, 2017

facebook-github-bot commented Mar 22, 2017

ajkr Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

ajkr Mar 23, 2017 •

edited

Loading