Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Less linear search in DBIter::Seek() when keys are overwritten a lot #1413

Closed
wants to merge 1 commit into from
Closed

Less linear search in DBIter::Seek() when keys are overwritten a lot #1413

wants to merge 1 commit into from

Conversation

al13n321
Copy link
Contributor

In one deployment we saw high latencies (presumably from slow iterator operations) and a lot of CPU time reported by perf with this stack:

  rocksdb::MergingIterator::Next
  rocksdb::DBIter::FindNextUserEntryInternal
  rocksdb::DBIter::Seek

I think what's happening is:

  1. we create a snapshot iterator,
  2. we do lots of Put()s for the same key x; this creates lots of entries in memtable,
  3. we seek the iterator to a key slightly smaller than x,
  4. the seek walks over lots of entries in memtable for key x, skipping them because of high sequence numbers.

CC @IslamAbdelRahman

@facebook-github-bot
Copy link
Contributor

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

@yiwu-arbug
Copy link
Contributor

@IslamAbdelRahman @lightmark do you mind review the PR? Thanks!

IslamAbdelRahman
IslamAbdelRahman reviewed Oct 31, 2016
View reviewed changes
Copy link
Contributor

@IslamAbdelRahman IslamAbdelRahman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but let's fix ROCKSDB_LITE before landing

db/db_iterator_test.cc

// Check that memtable wasn't flushed.
std::string val;
ASSERT_TRUE(db_->GetProperty("rocksdb.num-files-at-level0", &val));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call is not supported under ROCKSDB_LITE
we need to

#ifndef ROCKSDB_LITE
#endif

This test

@IslamAbdelRahman
Copy link
Contributor

looks like now we need to rebase as well

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@al13n321
Copy link
Contributor Author

Rebased and fixed ROCKSDB_LITE build.

@al13n321
Copy link
Contributor Author

Are the two test failures in travis legit? One of them also appears in #1553 , and other recent PRs have test failures. make -j check passes locally.

siying added a commit to siying/rocksdb that referenced this pull request Mar 7, 2017
@siying
A bug of max_iter
5023188 
Summary: The new unit test reproduces a regression bug introduced in facebook#1413
@siying
Copy link
Contributor

siying commented Mar 7, 2017

FYI, there appears to be a bug in it, which can be reproduced with siying@5023188

@lightmark lightmark mentioned this pull request Mar 8, 2017
Closed
facebook-github-bot pushed a commit that referenced this pull request Mar 9, 2017
@lightmark @facebook-github-bot
Reset DBIter::saved_key_ with proper user key anywhere before pass to…
12ba00e 
… DBIter::FindNextUserEntry

Summary:
fix db_iter bug introduced by [#1413](#1413)
Closes #1962

Differential Revision: D4672369

Pulled By: lightmark

fbshipit-source-id: 6a22953
IslamAbdelRahman pushed a commit that referenced this pull request Mar 9, 2017
@lightmark @IslamAbdelRahman
Reset DBIter::saved_key_ with proper user key anywhere before pass to…
acb2e98 
… DBIter::FindNextUserEntry

Summary:
fix db_iter bug introduced by [#1413](#1413)
Closes #1962

Differential Revision: D4672369

Pulled By: lightmark

fbshipit-source-id: 6a22953
lightmark added a commit that referenced this pull request Mar 14, 2017
@lightmark
Reset DBIter::saved_key_ with proper user key anywhere before pass to…
25ea289 
… DBIter::FindNextUserEntry

Summary:
fix db_iter bug introduced by [#1413](#1413)
Closes #1962

Differential Revision: D4672369

Pulled By: lightmark

fbshipit-source-id: 6a22953
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IslamAbdelRahman IslamAbdelRahman IslamAbdelRahman left review comments

Assignees

@IslamAbdelRahman IslamAbdelRahman

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@al13n321 @facebook-github-bot @yiwu-arbug @IslamAbdelRahman @siying