fix memory leak in two_level_iterator #3718

miasantreble · 2018-04-13T19:09:58Z

this PR fixes a few failed contbuild:

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 Customized BlockBasedTableIterator and LevelIterator #3406
various unused param errors introduced by [codemod] comment unused parameters to turn on -Wunused-parameter flag #3662
updated comment for ForceReleaseCachedEntry to emphasize the use of force_erase flag.

test plan: run asan, clang_release, lite, release, release_481 locally

facebook-github-bot

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

miasantreble · 2018-04-13T19:58:12Z

@siying PTAL my changes in TwoLevelIterator __dtor

ajkr

I don't know about the TwoLevelIterator changes, so am accepting the unused params part only (which could be a whole PR itself)

ajkr · 2018-04-13T20:25:46Z

util/delete_scheduler_test.cc

@@ -642,6 +642,8 @@ int main(int argc, char** argv) {

 #else
 int main(int argc, char** argv) {
+  (void) argc;


can we comment out never-used variables?

maysamyabandeh · 2018-04-13T21:05:55Z

I changed the title from "fix memory leak and unused param errors" to more descriptive title of "fix memory leak in two_level_iterator"

maysamyabandeh · 2018-04-13T21:10:34Z

table/two_level_iterator.cc

@@ -24,7 +24,11 @@ class TwoLevelIterator : public InternalIterator {
  explicit TwoLevelIterator(TwoLevelIteratorState* state,
                            InternalIterator* first_level_iter);

-  virtual ~TwoLevelIterator() { delete state_; }
+  virtual ~TwoLevelIterator() {
+    first_level_iter_.DeleteIter(false);


what is the variable that is assigned to false? and why false?

it's is_arena_mode that dictates whether to call delete or ~InternalIterator(), see https://github.com/facebook/rocksdb/blob/master/table/iterator_wrapper.h#L45
I think by default it was false for both first and second level iter in the original code https://github.com/facebook/rocksdb/pull/3406/files#diff-5fed9b271c2846509d9ad683c2624f26

Can you put a comment beside the false value? something like /* is_arena_mode */

What if later someone uses TwoLevelIterator and passes a first_level_iterator is created using arena? Can we add some comments to NewTwoLevelIterator and NewSecondaryIterator to indicate the contract that the object should not have been created using arena.

maysamyabandeh · 2018-04-13T21:26:17Z

tools/db_stress.cc

@@ -1023,20 +1023,16 @@ class DbStressListener : public EventListener {
        column_families_(column_families) {}
  virtual ~DbStressListener() {}
 #ifndef ROCKSDB_LITE
-  virtual void OnFlushCompleted(DB* db, const FlushJobInfo& info) override {
-    assert(db);


instead of removing the assert, can we use unused attributed?

@ajkr suggested these assert are useless since for db_stress they will always be true

yeah it seemed strange to me that we're testing whether OnFlushCompleted is passed the right arguments in our stress test, so I suggested deleting those checks rather than maintaining them.

maysamyabandeh · 2018-04-13T21:27:57Z

why not using attribute((unused))?

miasantreble · 2018-04-13T21:50:31Z

@maysamyabandeh the (void) unused way to resolve unused param issue was use throughout the previous PR #3662, is there advantage to use attribute((unused)) over (void) unused?

facebook-github-bot · 2018-04-13T22:44:01Z

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

facebook-github-bot

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

maysamyabandeh · 2018-04-13T22:53:04Z

Can you also add a comment about arena to table/two_level_iterator.h:25

 21 struct TwoLevelIteratorState {
 22   TwoLevelIteratorState() {}
 23
 24   virtual ~TwoLevelIteratorState() {}
 25   virtual InternalIterator* NewSecondaryIterator(const Slice& handle) = 0;
 26 };

facebook-github-bot · 2018-04-13T23:28:49Z

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

facebook-github-bot

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

maysamyabandeh · 2018-04-14T01:58:32Z

Seems like you strongly oppose against adding any clarification to NewSecondaryIterator. No problem. I can add it later after you land this.

miasantreble · 2018-04-14T02:57:42Z

@maysamyabandeh No, I don't have problem with that, I missed the line number in your comment so I added in line 20, do you think that's ok or shall I move it to line 25?

maysamyabandeh · 2018-04-16T00:01:57Z

Thanks @miasantreble. Let me go ahead and land this as I am about concerned about the production services being affected by that.

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

fix memory leak and unused param errors

d7364bb

miasantreble requested review from maysamyabandeh and ajkr April 13, 2018 19:09

facebook-github-bot added the CLA Signed label Apr 13, 2018

facebook-github-bot reviewed Apr 13, 2018

View reviewed changes

maysamyabandeh requested a review from yiwu-arbug April 13, 2018 19:21

miasantreble requested a review from siying April 13, 2018 19:55

ajkr approved these changes Apr 13, 2018

View reviewed changes

maysamyabandeh changed the title ~~fix memory leak and unused param errors~~ fix memory leak in two_level_iterator Apr 13, 2018

maysamyabandeh reviewed Apr 13, 2018

View reviewed changes

comment out unused variable, update comment for two_level_iterator

0632c72

facebook-github-bot reviewed Apr 13, 2018

View reviewed changes

comment update

87aef57

facebook-github-bot reviewed Apr 13, 2018

View reviewed changes

facebook-github-bot closed this in 954b496 Apr 16, 2018

miasantreble deleted the fix-memory-leak branch April 16, 2018 03:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix memory leak in two_level_iterator #3718

fix memory leak in two_level_iterator #3718

miasantreble commented Apr 13, 2018 •

edited by maysamyabandeh

Loading

facebook-github-bot left a comment

miasantreble commented Apr 13, 2018

ajkr left a comment

ajkr Apr 13, 2018

maysamyabandeh commented Apr 13, 2018

maysamyabandeh Apr 13, 2018

miasantreble Apr 13, 2018 •

edited

Loading

maysamyabandeh Apr 13, 2018

maysamyabandeh Apr 13, 2018

miasantreble Apr 13, 2018

ajkr Apr 14, 2018

maysamyabandeh commented Apr 13, 2018

miasantreble commented Apr 13, 2018

facebook-github-bot commented Apr 13, 2018

facebook-github-bot left a comment

maysamyabandeh commented Apr 13, 2018

facebook-github-bot commented Apr 13, 2018

facebook-github-bot left a comment

maysamyabandeh commented Apr 14, 2018

miasantreble commented Apr 14, 2018

maysamyabandeh commented Apr 16, 2018

fix memory leak in two_level_iterator #3718

fix memory leak in two_level_iterator #3718

Conversation

miasantreble commented Apr 13, 2018 • edited by maysamyabandeh Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

miasantreble commented Apr 13, 2018

ajkr left a comment

Choose a reason for hiding this comment

ajkr Apr 13, 2018

Choose a reason for hiding this comment

maysamyabandeh commented Apr 13, 2018

maysamyabandeh Apr 13, 2018

Choose a reason for hiding this comment

miasantreble Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

maysamyabandeh Apr 13, 2018

Choose a reason for hiding this comment

maysamyabandeh Apr 13, 2018

Choose a reason for hiding this comment

miasantreble Apr 13, 2018

Choose a reason for hiding this comment

ajkr Apr 14, 2018

Choose a reason for hiding this comment

maysamyabandeh commented Apr 13, 2018

miasantreble commented Apr 13, 2018

facebook-github-bot commented Apr 13, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

maysamyabandeh commented Apr 13, 2018

facebook-github-bot commented Apr 13, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

maysamyabandeh commented Apr 14, 2018

miasantreble commented Apr 14, 2018

maysamyabandeh commented Apr 16, 2018

miasantreble commented Apr 13, 2018 •

edited by maysamyabandeh

Loading

miasantreble Apr 13, 2018 •

edited

Loading