Fix performance regression in Get() for block-based tables #3953
Conversation
| @@ -269,6 +269,9 @@ class BlockIter final : public InternalIterator { | |||
| current_ = restarts_; | |||
| status_ = s; | |||
|
|
|||
| // Call cleanup callbacks. | |||
| Cleanable::Reset(); | |||
There was a problem hiding this comment.
This is the fix. In my runs of the regression tests this consistently makes the regression go away.
What was probably going on is that BlockBasedTableIterator didn't unpin blocks in block cache as it transitioned from block to block. Instead it unpinned all at once when iterator is destroyed. This caused compaction iterators to take up a lot of space in block cache, leaving less for the Get()s.
|
@al13n321 has updated the pull request. |
Actually, nevermind that one, it's probably correct. Reverted it. Now this PR is just the performance fix. |
There was a problem hiding this comment.
@al13n321 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
I verified that this fixes the perf regression in readwhilewriting benchmark due to 8bf555f. Thanks!! |
|
Travis failed (https://travis-ci.org/facebook/rocksdb/jobs/388056399), but a few other recent PRs have the same failure (in DBTest.GroupCommitTest), so I'm assuming it's not related to this PR. |
|
it's a flaky test, you can ignore it |
There was a problem hiding this comment.
@miasantreble is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
can't wait to land this PR, thanks @al13n321 for investigating and fixing this! |
Summary: This fixes a regression in one of myrocks regression tests (readwhilewriting), introduced in 8bf555f This PR changes two lines of code: one of them actually fixes the observed regression, the other is a mostly unrelated small fix that I'm piggy-backing here. EDIT: Nevermind, it fixes one line. More details in inline comments. Closes #3953 Differential Revision: D8270664 Pulled By: al13n321 fbshipit-source-id: a7d91e196807d1e816551591257c700f70e4ccac
This fixes a regression in one of myrocks regression tests (readwhilewriting), introduced in 8bf555f
This PR changes two lines of code: one of them actually fixes the observed regression, the other is a mostly unrelated small fix that I'm piggy-backing here. EDIT: Nevermind, it fixes one line. More details in inline comments.