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.

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

save a key comparison in block seeks #6646

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
address comments
  • Loading branch information
ajkr committed Jun 10, 2020
commit acb69e1cc200caacfab288651c47695047e4425d
26 changes: 11 additions & 15 deletions table/block_based/block.cc
Original file line number Diff line number Diff line change
Expand Up @@ -653,31 +653,27 @@ void BlockIter<TValue>::ScanAfterBinarySeek(const Slice& target, uint32_t index,
bool is_index_key_result,
const Comparator* comp) {
// Linear search (within restart block) for first key >= target
if (is_index_key_result) {
SeekToRestartPoint(index);
Next();
} else {
SeekToRestartPoint(index);
SeekToRestartPoint(index);
Next();
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment here might help. It took me a little while to figure out that this Next() actually doesn't advance to the next key, but just ends up decoding the key at the restart point we seek'd to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, done.

if (!is_index_key_result) {
uint32_t max_offset;
if (index + 1 < num_restarts_) {
// We are in a non-last restart interval. Since `BinarySeek()` guarantees
// the next restart key is strictly greater than `target`, we can
// terminate upon reaching it without any additional key comparison.
max_offset = GetRestartPoint(index + 1);
} else {
max_offset = restarts_;
// We are in the last restart interval. The while-loop will terminate by
// `Valid()` returning false upon advancing past the block's last key.
max_offset = port::kMaxUint32;
}
bool first = true;
while (true) {
Next();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use ParseNextDataKey instead of Next and Valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's making use of polymorphism here -- IndexBlockIter::Next() calls ParseNextIndexKey() while DataBlockIter::Next() calls ParseNextDataKey().

if (!Valid()) {
break;
}
if (first) {
// Skip the comparison on the first key since we already know from
// the binary search that it's less than target.
first = false;
} else if (current_ == max_offset) {
// We already know from the binary search that this key is >= target.
// Elide the key comparison.
assert(comp->Compare(applied_key_.UpdateAndGetKey(), target) >= 0);
if (current_ == max_offset) {
assert(comp->Compare(applied_key_.UpdateAndGetKey(), target) > 0);
break;
} else if (comp->Compare(applied_key_.UpdateAndGetKey(), target) >= 0) {
break;
Expand Down
2 changes: 1 addition & 1 deletion table/block_based/block.h
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ class BlockIter : public InternalIteratorBase<TValue> {

Cache::Handle* cache_handle() { return cache_handle_; }

virtual void Next() = 0;
virtual void Next() override = 0;

protected:
// Note: The type could be changed to InternalKeyComparator but we see a weird
Expand Down