Customized BlockBasedTableIterator and LevelIterator #3406

siying · 2018-01-25T00:33:58Z

Summary:
Use a customzied BlockBasedTableIterator and LevelIterator to replace current implementations leveraging two-level-iterator. Hope the customized logic will make code easier to understand. As a side effect, BlockBasedTableIterator reduces the allocation for the data block iterator object, and avoid the virtual function call to it, because we can directly reference BlockIter, a final class. Similarly, LevelIterator reduces virtual function call to the dummy iterator iterating the file metadata. It also enabled further optimization.

The upper bound check is also moved from index block to data block. This implementation fits this iterator better. After the change, forwared iterator is slightly optimized to ensure we trim those iterators.

The two-level-iterator now is only used by partitioned index, so it is simplified.

Test Plan: Run all existing tests using ASAN.

facebook-github-bot

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

facebook-github-bot · 2018-01-26T00:47:20Z

@siying has updated the pull request. View: changes, changes since last import

facebook-github-bot · 2018-01-26T01:26:29Z

@siying has updated the pull request.

facebook-github-bot · 2018-01-26T01:56:53Z

@siying has updated the pull request. View: changes, changes since last import

facebook-github-bot

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

facebook-github-bot · 2018-01-26T18:57:43Z

@siying has updated the pull request. View: changes, changes since last import

facebook-github-bot · 2018-01-26T19:06:59Z

@siying has updated the pull request.

facebook-github-bot · 2018-01-26T19:41:19Z

@siying has updated the pull request. View: changes, changes since last import

facebook-github-bot

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

yiwu-arbug

Some minor comment and general questions. And I haven't go through BlockBasedTableReader yet.

yiwu-arbug · 2018-01-30T21:27:13Z

table/two_level_iterator.h

@@ -19,16 +19,10 @@ class InternalKeyComparator;
 class Arena;

 struct TwoLevelIteratorState {
-  explicit TwoLevelIteratorState(bool _check_prefix_may_match)
-      : check_prefix_may_match(_check_prefix_may_match) {}
+  explicit TwoLevelIteratorState() {}


nit: not needing explicit keyword

yiwu-arbug · 2018-01-30T21:55:11Z

table/internal_iterator.h

@@ -76,6 +76,8 @@ class InternalIterator : public Cleanable {
  // with PinnedIteratorsManager.
  virtual void SetPinnedItersMgr(PinnedIteratorsManager* pinned_iters_mgr) {}

+  virtual bool MayPositionAfterEnd() { return true; }


Add comment to explain what it means to position after end?

yiwu-arbug · 2018-01-30T22:30:23Z

db/version_set.cc

-                                     false /* don't sample compaction */));
+        list[num++] = new LevelIterator(
+            cfd->table_cache(), read_options, env_options_compactions,
+            cfd->internal_comparator(), c->input_levels(which), false,


nit: add comment what's the last false.

yiwu-arbug · 2018-01-30T22:35:29Z

db/version_set.cc

- public:
-  // @param skip_filters Disables loading/accessing the filter block
-  LevelFileIteratorState(TableCache* table_cache,
+  explicit LevelIterator(TableCache* table_cache,


nit: explicit keyword not needed.

it's not that uncommon, I think, to always mark constructors explicit so that the keyword's not forgotten when people add/remove arguments and it one day ends up being a single-argument constructor.

@yiwu-arbug let me remove it.

@ajkr make sense too.

yiwu-arbug · 2018-02-01T06:20:34Z

db/version_set.cc

+    SetFileIterator(nullptr);
+    return;
+  } else {
+    if (file_iter_.iter() != nullptr && !file_iter_.status().IsIncomplete() &&


when file_iter_.status() can be incomplete? Do we need to check other status?

When users set read_options.read_tiers=kNoIO or something like that, when block cache miss happens, rather than reading from disk, it continues to the next block and set status to incomplete. I suspect that it was a bug in initial implementation, but I didn't change this behavior in this PR.

yiwu-arbug · 2018-02-01T06:42:31Z

db/version_set.cc

        level_(level),
-        range_del_agg_(range_del_agg) {}
+        range_del_agg_(range_del_agg),
+        may_position_after_end_(true),


what does may_position_after_end mean?

Let me remove it if it is not needed in this iterator.
I used this flag to identify that the iterator is made invalid because of upper bound check. This information is important to an upper level iterator because, unless knowing this information, the upper level will assume this is the end so that they may consider to open the next file.

yiwu-arbug · 2018-02-01T06:43:22Z

db/version_set.cc

-// 16-byte value containing the file number and file size, both
-// encoded using EncodeFixed64.
-class LevelFileNumIterator : public InternalIterator {
+class LevelIterator final : public InternalIterator {


This class still feel like TwoLevelIterator with some renaming. Any chance to simplify it further?

I tried to make TwoLevelIterator much simpler now. Maybe it can be made even simpler. Before TwoLevelIterator contains lots of special logic, like prefix may match, etc. Now the logic only goes to where it is needed.

facebook-github-bot · 2018-02-02T18:35:58Z

@siying has updated the pull request.

siying · 2018-02-02T18:36:10Z

Addressed the comments. I changed from MayPositionAfterEnd() to IsOutOfBound() and add comments about it. Hope it will be clearer.

facebook-github-bot

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

facebook-github-bot · 2018-02-02T23:55:18Z

@siying has updated the pull request. View: changes, changes since last import

facebook-github-bot

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

yiwu-arbug

Sorry for the super delay. Only some minor comments.

yiwu-arbug · 2018-02-12T05:46:06Z

table/block_based_table_reader.cc

-      skip_filters_(skip_filters),
-      is_index_(is_index),
-      block_map_(block_map) {}
+    : table_(table), block_map_(block_map) {}


Most of the params of this constructor is unused. Can we remove them?

yiwu-arbug · 2018-02-12T06:14:32Z

table/block_based_table_reader.cc

+
+  SavePrevIndexValue();
+
+  index_iter_->Seek(target);


Add comment on why here we call Seek not SeekForPrev? And would it be correct and more efficient to call SeekForPrev()?

yiwu-arbug · 2018-02-12T06:20:21Z

table/block_based_table_reader.h

+  bool is_out_of_bound_ = false;
+  bool check_filter_;
+  // TODO use block offset instead
+  std::string prev_index_value;


nit: private member should end with underscore.

yiwu-arbug · 2018-02-12T06:27:18Z

table/block_based_table_reader.cc

  }

-  // indicates if we are on the last page that need to be pre-fetched
-  bool prefetching_boundary_page = false;
+  void BlockBasedTableIterator::FindKeyBackward() {


formatting: there's an extra indent starting from here.

facebook-github-bot · 2018-02-12T21:51:59Z

@siying has updated the pull request.

Customized BlockBasedTableIterator Summary: Use a customzied BlockBasedTableIterator to replace current implementation leveraging two level iterator. Hope the customized logic will make code easier to understand. As a side effect, we reduce the allocation for the data block iterator object, and avoid the virtual function call to it, because we can directly reference BlockIter, a final class. It also enabled further optimization. The upper bound check is also moved from index block to data block. This implementation fits this iterator better. After the change, forwared iterator is slightly optimized to ensure we trim those iterators. Test Plan: Run all existing tests using ASAN. Simplify two-level-iterator fix clang Fix issues because of rebase Address comments. Fix a memory issue

facebook-github-bot

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

facebook-github-bot · 2018-02-12T22:10:43Z

@siying has updated the pull request.

facebook-github-bot · 2018-02-12T22:14:26Z

@siying has updated the pull request. View: changes, changes since last import

facebook-github-bot

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

siying · 2018-02-12T22:16:10Z

Addressed comments.

siying · 2018-02-12T23:06:30Z

appveyor stucks now. But it has passed in a previous revision, and we don't have many changes since then. Land it.

facebook-github-bot

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

siying · 2018-02-13T00:12:27Z

The Travis times out in test group 2. The master branch experienced the same issue this morning, so I think it's not the problem.

miasantreble · 2018-04-10T18:52:10Z

table/two_level_iterator.cc

-      state_->~TwoLevelIteratorState();
-    }
-  }
+  virtual ~TwoLevelIterator() { delete state_; }


Under the new design, who's responsible for deleting first_level_iter_ and second_level_iter_?
RIght now there are memory leaks that trace back to https://github.com/facebook/rocksdb/blob/master/table/two_level_iterator.cc#L205 and https://github.com/facebook/rocksdb/blob/master/table/two_level_iterator.cc#L196

Call stack see https://gist.github.com/miasantreble/0de9bd7ee11451cb1bebbd108f7b3e80

this is happening after #3692 fixed the test to vary the level

@miasantreble this seems like a bug to me.

Summary: this PR fixes a few failed contbuild: 1. ASAN memory leak in Block::NewIterator (table/block.cc:429). the proper destruction of first_level_iter_ and second_level_iter_ of two_level_iterator.cc is missing from the code after the refactoring in #3406 2. various unused param errors introduced by #3662 3. updated comment for `ForceReleaseCachedEntry` to emphasize the use of `force_erase` flag. Closes #3718 Reviewed By: maysamyabandeh Differential Revision: D7621192 Pulled By: miasantreble fbshipit-source-id: 476c94264083a0730ded957c29de7807e4f5b146

siying requested review from maysamyabandeh and ajkr January 25, 2018 00:33

facebook-github-bot added the CLA Signed label Jan 25, 2018

facebook-github-bot reviewed Jan 25, 2018

View reviewed changes

siying changed the title ~~Customized BlockBasedTableIterator~~ Customized BlockBasedTableIterator and LevelIterator Jan 26, 2018

siying force-pushed the refactor_tli branch from 9b3236d to fd6e26b Compare January 26, 2018 01:26

facebook-github-bot reviewed Jan 26, 2018

View reviewed changes

siying force-pushed the refactor_tli branch from 3b6ec03 to 54dc5c8 Compare January 26, 2018 19:04

facebook-github-bot reviewed Jan 26, 2018

View reviewed changes

yiwu-arbug reviewed Feb 1, 2018

View reviewed changes

siying force-pushed the refactor_tli branch from 2e1c2f7 to 2f731f2 Compare February 2, 2018 18:35

facebook-github-bot reviewed Feb 2, 2018

View reviewed changes

yiwu-arbug approved these changes Feb 12, 2018

View reviewed changes

siying force-pushed the refactor_tli branch from 081e989 to 493aefb Compare February 12, 2018 21:51

siying force-pushed the refactor_tli branch from 493aefb to 5798e13 Compare February 12, 2018 22:10

facebook-github-bot reviewed Feb 12, 2018

View reviewed changes

fix

ace2100

facebook-github-bot reviewed Feb 12, 2018

View reviewed changes

facebook-github-bot reviewed Feb 13, 2018

View reviewed changes

facebook-github-bot closed this in b555ed3 Feb 13, 2018

miasantreble reviewed Apr 10, 2018

View reviewed changes

miasantreble mentioned this pull request Apr 13, 2018

fix memory leak in two_level_iterator #3718

Closed

siying mentioned this pull request Mar 26, 2019

Avoid per-key upper bound check in BlockBasedTableIterator #5101

Closed

Customized BlockBasedTableIterator and LevelIterator #3406

Customized BlockBasedTableIterator and LevelIterator #3406

Conversation

siying commented Jan 25, 2018 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 26, 2018

facebook-github-bot commented Jan 26, 2018

facebook-github-bot commented Jan 26, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 26, 2018

facebook-github-bot commented Jan 26, 2018

facebook-github-bot commented Jan 26, 2018

facebook-github-bot left a 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

siying Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siying Feb 2, 2018 • edited Loading

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 Feb 2, 2018

siying commented Feb 2, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 2, 2018

facebook-github-bot left a 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

facebook-github-bot commented Feb 12, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 12, 2018

facebook-github-bot commented Feb 12, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying commented Feb 12, 2018

siying commented Feb 12, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying commented Feb 13, 2018

miasantreble Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siying commented Jan 25, 2018 •

edited

Loading

siying Feb 2, 2018 •

edited

Loading

siying Feb 2, 2018 •

edited

Loading

miasantreble Apr 10, 2018 •

edited

Loading