From da40d45267338ac0e91e5d065acab8df2c69cef3 Mon Sep 17 00:00:00 2001 From: Fenggang Wu Date: Thu, 23 Aug 2018 10:12:15 -0700 Subject: [PATCH] DataBlockHashIndex: avoiding expensive iiter->Next when handling hash kNoEntry (#4296) Summary: When returning `kNoEntry` from HashIndex lookup, previously we invalidate the `biter` by set `current_=restarts_`, so that the search can continue to the next block in case the search result may reside in the next block. There is one problem: when we are searching for a missing key, if the search finds a `kNoEntry` and continue the search to the next block, there is also a non-trivial possibility that the HashIndex return `kNoEntry` too, and the expensive index iterator `Next()` will happen several times for nothing. The solution is that if the hash table returns `kNoEntry`, `SeekForGetImpl()` just search the last restart interval for the key. It will stop at the first key that is large than the seek_key, or to the end of the block, and each case will be handled correctly. Microbenchmark script: ``` TEST_TMPDIR=/dev/shm ./db_bench --benchmarks=fillseq,readtocache,readmissing \ --cache_size=20000000000 --use_data_block_hash_index={true|false} ``` `readmissing` performance (lower is better): ``` binary: 3.6098 micros/op hash (before applying diff): 4.1048 micros/op hash (after applying diff): 3.3502 micros/op ``` Pull Request resolved: https://github.com/facebook/rocksdb/pull/4296 Differential Revision: D9419159 Pulled By: fgwu fbshipit-source-id: 21e3eedcccbc47a249aa8eb4bf405c9def0b8a05 --- table/block.cc | 53 ++++++++++++++++++++++++++++---------------------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/table/block.cc b/table/block.cc index 2b3bc0050..3d27e04d3 100644 --- a/table/block.cc +++ b/table/block.cc @@ -235,7 +235,7 @@ void DataBlockIter::Seek(const Slice& target) { // // If the return value is FALSE, iter location is undefined, and it means: // 1) there is no key in this block falling into the range: -// ["seek_user_key @ type | seqno", "seek_user_key @ type | 0"], +// ["seek_user_key @ type | seqno", "seek_user_key @ kTypeDeletion | 0"], // inclusive; AND // 2) the last key of this block has a greater user_key from seek_user_key // @@ -243,13 +243,21 @@ void DataBlockIter::Seek(const Slice& target) { // 1) If iter is valid, it is set to a location as if set by BinarySeek. In // this case, it points to the first key_ with a larger user_key or a // matching user_key with a seqno no greater than the seeking seqno. -// 2) If the iter is invalid, it means either the block has no such user_key, -// or the block ends with a matching user_key but with a larger seqno. +// 2) If the iter is invalid, it means that either all the user_key is less +// than the seek_user_key, or the block ends with a matching user_key but +// with a smaller [ type | seqno ] (i.e. a larger seqno, or the same seqno +// but larger type). bool DataBlockIter::SeekForGetImpl(const Slice& target) { Slice user_key = ExtractUserKey(target); uint32_t map_offset = restarts_ + num_restarts_ * sizeof(uint32_t); uint8_t entry = data_block_hash_index_->Lookup(data_, map_offset, user_key); + if (entry == kCollision) { + // HashSeek not effective, falling back + Seek(target); + return true; + } + if (entry == kNoEntry) { // Even if we cannot find the user_key in this block, the result may // exist in the next block. Consider this exmpale: @@ -260,16 +268,13 @@ bool DataBlockIter::SeekForGetImpl(const Slice& target) { // // If seek_key = axy@60, the search will starts from Block N. // Even if the user_key is not found in the hash map, the caller still - // have to conntinue searching the next block. So we invalidate the - // iterator to tell the caller to go on. - current_ = restarts_; // Invalidate the iter - return true; - } - - if (entry == kCollision) { - // HashSeek not effective, falling back - Seek(target); - return true; + // have to conntinue searching the next block. + // + // In this case, we pretend the key is the the last restart interval. + // The while-loop below will search the last restart interval for the + // key. It will stop at the first key that is larger than the seek_key, + // or to the end of the block if no one is larger. + entry = static_cast(num_restarts_ - 1); } uint32_t restart_index = entry; @@ -299,24 +304,26 @@ bool DataBlockIter::SeekForGetImpl(const Slice& target) { } if (current_ == restarts_) { - // Search reaches to the end of the block. There are two possibilites; + // Search reaches to the end of the block. There are three possibilites: // 1) there is only one user_key match in the block (otherwise collsion). - // the matching user_key resides in the last restart interval. - // it is the last key of the restart interval and of the block too. - // ParseNextDataKey() skiped it as its seqno is newer. + // the matching user_key resides in the last restart interval, and it + // is the last key of the restart interval and of the block as well. + // ParseNextDataKey() skiped it as its [ type | seqno ] is smaller. // - // 2) The seek_key is a false positive and got hashed to the last restart - // interval. - // All existing keys in the restart interval are less than seek_key. + // 2) The seek_key is not found in the HashIndex Lookup(), i.e. kNoEntry, + // AND all existing user_keys in the restart interval are smaller than + // seek_user_key. // - // The result may exist in the next block in either case, so may_exist is - // returned as true. + // 3) The seek_key is a false positive and happens to be hashed to the + // last restart interval, AND all existing user_keys in the restart + // interval are smaller than seek_user_key. + // + // The result may exist in the next block each case, so we return true. return true; } if (user_comparator_->Compare(key_.GetUserKey(), user_key) != 0) { // the key is not in this block and cannot be at the next block either. - // return false to tell the caller to break from the top-level for-loop return false; }