From d69241586e029117f4d208a5654f586c7a7f795a Mon Sep 17 00:00:00 2001 From: Yi Wu Date: Wed, 27 Mar 2019 10:24:16 -0700 Subject: [PATCH] Fix perf_context.user_key_comparison_count for range scan (#5098) Summary: Currently `perf_context.user_key_comparison_count` is bump only in `InternalKeyComparator`. For places user comparator is used directly the counter is not bump. Fixing the majority of it. Index iterator and filter code also use user comparator directly and don't bump the counter. It is not fixed in this patch. Pull Request resolved: https://github.com/facebook/rocksdb/pull/5098 Differential Revision: D14603753 Pulled By: siying fbshipit-source-id: 1cd41035644ca9e49b97a51030a5d1e15f5f3cae --- db/db_iter.cc | 38 +++++++++--------- db/dbformat.cc | 11 +++--- db/dbformat.h | 23 +++++------ db/version_set.cc | 8 ++-- table/block_based_table_reader.cc | 10 ++--- table/block_based_table_reader.h | 3 ++ util/user_comparator_wrapper.h | 65 +++++++++++++++++++++++++++++++ 7 files changed, 113 insertions(+), 45 deletions(-) create mode 100644 util/user_comparator_wrapper.h diff --git a/db/db_iter.cc b/db/db_iter.cc index 7d91ef027..0f29354ae 100644 --- a/db/db_iter.cc +++ b/db/db_iter.cc @@ -28,6 +28,7 @@ #include "util/mutexlock.h" #include "util/string_util.h" #include "util/trace_replay.h" +#include "util/user_comparator_wrapper.h" namespace rocksdb { @@ -306,7 +307,7 @@ class DBIter final: public Iterator { const SliceTransform* prefix_extractor_; Env* const env_; Logger* logger_; - const Comparator* const user_comparator_; + UserComparatorWrapper user_comparator_; const MergeOperator* const merge_operator_; InternalIterator* iter_; ReadCallback* read_callback_; @@ -455,7 +456,7 @@ bool DBIter::FindNextUserEntryInternal(bool skipping, bool prefix_check) { } if (iterate_upper_bound_ != nullptr && - user_comparator_->Compare(ikey_.user_key, *iterate_upper_bound_) >= 0) { + user_comparator_.Compare(ikey_.user_key, *iterate_upper_bound_) >= 0) { break; } @@ -470,8 +471,8 @@ bool DBIter::FindNextUserEntryInternal(bool skipping, bool prefix_check) { } if (IsVisible(ikey_.sequence)) { - if (skipping && user_comparator_->Compare(ikey_.user_key, - saved_key_.GetUserKey()) <= 0) { + if (skipping && user_comparator_.Compare(ikey_.user_key, + saved_key_.GetUserKey()) <= 0) { num_skipped++; // skip this entry PERF_COUNTER_ADD(internal_key_skipped_count, 1); } else { @@ -578,7 +579,7 @@ bool DBIter::FindNextUserEntryInternal(bool skipping, bool prefix_check) { // If this happens too many times in a row for the same user key, we want // to seek to the target sequence number. int cmp = - user_comparator_->Compare(ikey_.user_key, saved_key_.GetUserKey()); + user_comparator_.Compare(ikey_.user_key, saved_key_.GetUserKey()); if (cmp == 0 || (skipping && cmp <= 0)) { num_skipped++; } else { @@ -654,7 +655,7 @@ bool DBIter::MergeValuesNewToOld() { return false; } - if (!user_comparator_->Equal(ikey.user_key, saved_key_.GetUserKey())) { + if (!user_comparator_.Equal(ikey.user_key, saved_key_.GetUserKey())) { // hit the next user key, stop right here break; } else if (kTypeDeletion == ikey.type || kTypeSingleDeletion == ikey.type || @@ -774,8 +775,7 @@ bool DBIter::ReverseToForward() { if (!ParseKey(&ikey)) { return false; } - if (user_comparator_->Compare(ikey.user_key, saved_key_.GetUserKey()) >= - 0) { + if (user_comparator_.Compare(ikey.user_key, saved_key_.GetUserKey()) >= 0) { return true; } iter_->Next(); @@ -838,8 +838,8 @@ void DBIter::PrevInternal() { } if (iterate_lower_bound_ != nullptr && - user_comparator_->Compare(saved_key_.GetUserKey(), - *iterate_lower_bound_) < 0) { + user_comparator_.Compare(saved_key_.GetUserKey(), + *iterate_lower_bound_) < 0) { // We've iterated earlier than the user-specified lower bound. valid_ = false; return; @@ -900,7 +900,7 @@ bool DBIter::FindValueForCurrentKey() { } if (!IsVisible(ikey.sequence) || - !user_comparator_->Equal(ikey.user_key, saved_key_.GetUserKey())) { + !user_comparator_.Equal(ikey.user_key, saved_key_.GetUserKey())) { break; } if (TooManyInternalKeysSkipped()) { @@ -1053,7 +1053,7 @@ bool DBIter::FindValueForCurrentKeyUsingSeek() { if (!ParseKey(&ikey)) { return false; } - if (!user_comparator_->Equal(ikey.user_key, saved_key_.GetUserKey())) { + if (!user_comparator_.Equal(ikey.user_key, saved_key_.GetUserKey())) { // No visible values for this key, even though FindValueForCurrentKey() // has seen some. This is possible if we're using a tailing iterator, and // the entries were discarded in a compaction. @@ -1109,7 +1109,7 @@ bool DBIter::FindValueForCurrentKeyUsingSeek() { if (!ParseKey(&ikey)) { return false; } - if (!user_comparator_->Equal(ikey.user_key, saved_key_.GetUserKey())) { + if (!user_comparator_.Equal(ikey.user_key, saved_key_.GetUserKey())) { break; } @@ -1191,7 +1191,7 @@ bool DBIter::FindUserKeyBeforeSavedKey() { return false; } - if (user_comparator_->Compare(ikey.user_key, saved_key_.GetUserKey()) < 0) { + if (user_comparator_.Compare(ikey.user_key, saved_key_.GetUserKey()) < 0) { return true; } @@ -1281,8 +1281,8 @@ void DBIter::Seek(const Slice& target) { #endif // ROCKSDB_LITE if (iterate_lower_bound_ != nullptr && - user_comparator_->Compare(saved_key_.GetUserKey(), - *iterate_lower_bound_) < 0) { + user_comparator_.Compare(saved_key_.GetUserKey(), *iterate_lower_bound_) < + 0) { saved_key_.Clear(); saved_key_.SetInternalKey(*iterate_lower_bound_, seq); } @@ -1333,8 +1333,8 @@ void DBIter::SeekForPrev(const Slice& target) { kValueTypeForSeekForPrev); if (iterate_upper_bound_ != nullptr && - user_comparator_->Compare(saved_key_.GetUserKey(), - *iterate_upper_bound_) >= 0) { + user_comparator_.Compare(saved_key_.GetUserKey(), + *iterate_upper_bound_) >= 0) { saved_key_.Clear(); saved_key_.SetInternalKey(*iterate_upper_bound_, kMaxSequenceNumber); } @@ -1428,7 +1428,7 @@ void DBIter::SeekToLast() { if (iterate_upper_bound_ != nullptr) { // Seek to last key strictly less than ReadOptions.iterate_upper_bound. SeekForPrev(*iterate_upper_bound_); - if (Valid() && user_comparator_->Equal(*iterate_upper_bound_, key())) { + if (Valid() && user_comparator_.Equal(*iterate_upper_bound_, key())) { ReleaseTempPinnedData(); PrevInternal(); } diff --git a/db/dbformat.cc b/db/dbformat.cc index b34b04ca4..0ef2df4f7 100644 --- a/db/dbformat.cc +++ b/db/dbformat.cc @@ -116,8 +116,7 @@ int InternalKeyComparator::Compare(const ParsedInternalKey& a, // increasing user key (according to user-supplied comparator) // decreasing sequence number // decreasing type (though sequence# should be enough to disambiguate) - int r = user_comparator_->Compare(a.user_key, b.user_key); - PERF_COUNTER_ADD(user_key_comparison_count, 1); + int r = user_comparator_.Compare(a.user_key, b.user_key); if (r == 0) { if (a.sequence > b.sequence) { r = -1; @@ -139,9 +138,9 @@ void InternalKeyComparator::FindShortestSeparator( Slice user_start = ExtractUserKey(*start); Slice user_limit = ExtractUserKey(limit); std::string tmp(user_start.data(), user_start.size()); - user_comparator_->FindShortestSeparator(&tmp, user_limit); + user_comparator_.FindShortestSeparator(&tmp, user_limit); if (tmp.size() <= user_start.size() && - user_comparator_->Compare(user_start, tmp) < 0) { + user_comparator_.Compare(user_start, tmp) < 0) { // User key has become shorter physically, but larger logically. // Tack on the earliest possible number to the shortened user key. PutFixed64(&tmp, PackSequenceAndType(kMaxSequenceNumber,kValueTypeForSeek)); @@ -154,9 +153,9 @@ void InternalKeyComparator::FindShortestSeparator( void InternalKeyComparator::FindShortSuccessor(std::string* key) const { Slice user_key = ExtractUserKey(*key); std::string tmp(user_key.data(), user_key.size()); - user_comparator_->FindShortSuccessor(&tmp); + user_comparator_.FindShortSuccessor(&tmp); if (tmp.size() <= user_key.size() && - user_comparator_->Compare(user_key, tmp) < 0) { + user_comparator_.Compare(user_key, tmp) < 0) { // User key has become shorter physically, but larger logically. // Tack on the earliest possible number to the shortened user key. PutFixed64(&tmp, PackSequenceAndType(kMaxSequenceNumber,kValueTypeForSeek)); diff --git a/db/dbformat.h b/db/dbformat.h index 686b82966..854dfddb8 100644 --- a/db/dbformat.h +++ b/db/dbformat.h @@ -21,6 +21,7 @@ #include "rocksdb/types.h" #include "util/coding.h" #include "util/logging.h" +#include "util/user_comparator_wrapper.h" namespace rocksdb { @@ -160,13 +161,13 @@ class InternalKeyComparator #endif : public Comparator { private: - const Comparator* user_comparator_; + UserComparatorWrapper user_comparator_; std::string name_; public: - explicit InternalKeyComparator(const Comparator* c) : user_comparator_(c), - name_("rocksdb.InternalKeyComparator:" + - std::string(user_comparator_->Name())) { - } + explicit InternalKeyComparator(const Comparator* c) + : user_comparator_(c), + name_("rocksdb.InternalKeyComparator:" + + std::string(user_comparator_.Name())) {} virtual ~InternalKeyComparator() {} virtual const char* Name() const override; @@ -177,12 +178,14 @@ class InternalKeyComparator const Slice& limit) const override; virtual void FindShortSuccessor(std::string* key) const override; - const Comparator* user_comparator() const { return user_comparator_; } + const Comparator* user_comparator() const { + return user_comparator_.user_comparator(); + } int Compare(const InternalKey& a, const InternalKey& b) const; int Compare(const ParsedInternalKey& a, const ParsedInternalKey& b) const; virtual const Comparator* GetRootComparator() const override { - return user_comparator_->GetRootComparator(); + return user_comparator_.GetRootComparator(); } }; @@ -639,8 +642,7 @@ int InternalKeyComparator::Compare(const Slice& akey, const Slice& bkey) const { // increasing user key (according to user-supplied comparator) // decreasing sequence number // decreasing type (though sequence# should be enough to disambiguate) - int r = user_comparator_->Compare(ExtractUserKey(akey), ExtractUserKey(bkey)); - PERF_COUNTER_ADD(user_key_comparison_count, 1); + int r = user_comparator_.Compare(ExtractUserKey(akey), ExtractUserKey(bkey)); if (r == 0) { const uint64_t anum = DecodeFixed64(akey.data() + akey.size() - 8); const uint64_t bnum = DecodeFixed64(bkey.data() + bkey.size() - 8); @@ -659,8 +661,7 @@ int InternalKeyComparator::CompareKeySeq(const Slice& akey, // Order by: // increasing user key (according to user-supplied comparator) // decreasing sequence number - int r = user_comparator_->Compare(ExtractUserKey(akey), ExtractUserKey(bkey)); - PERF_COUNTER_ADD(user_key_comparison_count, 1); + int r = user_comparator_.Compare(ExtractUserKey(akey), ExtractUserKey(bkey)); if (r == 0) { // Shift the number to exclude the last byte which contains the value type const uint64_t anum = DecodeFixed64(akey.data() + akey.size() - 8) >> 8; diff --git a/db/version_set.cc b/db/version_set.cc index 5241608df..6bf105c6a 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -51,6 +51,7 @@ #include "util/stop_watch.h" #include "util/string_util.h" #include "util/sync_point.h" +#include "util/user_comparator_wrapper.h" namespace rocksdb { @@ -477,6 +478,7 @@ class LevelIterator final : public InternalIterator { read_options_(read_options), env_options_(env_options), icomparator_(icomparator), + user_comparator_(icomparator.user_comparator()), flevel_(flevel), prefix_extractor_(prefix_extractor), file_read_hist_(file_read_hist), @@ -541,9 +543,8 @@ class LevelIterator final : public InternalIterator { bool KeyReachedUpperBound(const Slice& internal_key) { return read_options_.iterate_upper_bound != nullptr && - icomparator_.user_comparator()->Compare( - ExtractUserKey(internal_key), - *read_options_.iterate_upper_bound) >= 0; + user_comparator_.Compare(ExtractUserKey(internal_key), + *read_options_.iterate_upper_bound) >= 0; } InternalIterator* NewFileIterator() { @@ -571,6 +572,7 @@ class LevelIterator final : public InternalIterator { const ReadOptions read_options_; const EnvOptions& env_options_; const InternalKeyComparator& icomparator_; + const UserComparatorWrapper user_comparator_; const LevelFilesBrief* flevel_; mutable FileDescriptor current_value_; const SliceTransform* prefix_extractor_; diff --git a/table/block_based_table_reader.cc b/table/block_based_table_reader.cc index ac57eec21..3952f89ab 100644 --- a/table/block_based_table_reader.cc +++ b/table/block_based_table_reader.cc @@ -2353,9 +2353,8 @@ void BlockBasedTableIterator::Seek(const Slice& target) { assert( !block_iter_.Valid() || (key_includes_seq_ && icomp_.Compare(target, block_iter_.key()) <= 0) || - (!key_includes_seq_ && - icomp_.user_comparator()->Compare(ExtractUserKey(target), - block_iter_.key()) <= 0)); + (!key_includes_seq_ && user_comparator_.Compare(ExtractUserKey(target), + block_iter_.key()) <= 0)); } template @@ -2522,9 +2521,8 @@ void BlockBasedTableIterator::FindKeyForward() { bool reached_upper_bound = (read_options_.iterate_upper_bound != nullptr && block_iter_points_to_real_block_ && block_iter_.Valid() && - icomp_.user_comparator()->Compare(ExtractUserKey(block_iter_.key()), - *read_options_.iterate_upper_bound) >= - 0); + user_comparator_.Compare(ExtractUserKey(block_iter_.key()), + *read_options_.iterate_upper_bound) >= 0); TEST_SYNC_POINT_CALLBACK( "BlockBasedTable::BlockEntryIteratorState::KeyReachedUpperBound", &reached_upper_bound); diff --git a/table/block_based_table_reader.h b/table/block_based_table_reader.h index b0dbb4e40..4ef7aed8a 100644 --- a/table/block_based_table_reader.h +++ b/table/block_based_table_reader.h @@ -33,6 +33,7 @@ #include "table/two_level_iterator.h" #include "util/coding.h" #include "util/file_reader_writer.h" +#include "util/user_comparator_wrapper.h" namespace rocksdb { @@ -580,6 +581,7 @@ class BlockBasedTableIterator : public InternalIteratorBase { : table_(table), read_options_(read_options), icomp_(icomp), + user_comparator_(icomp.user_comparator()), index_iter_(index_iter), pinned_iters_mgr_(nullptr), block_iter_points_to_real_block_(false), @@ -676,6 +678,7 @@ class BlockBasedTableIterator : public InternalIteratorBase { BlockBasedTable* table_; const ReadOptions read_options_; const InternalKeyComparator& icomp_; + UserComparatorWrapper user_comparator_; InternalIteratorBase* index_iter_; PinnedIteratorsManager* pinned_iters_mgr_; TBlockIter block_iter_; diff --git a/util/user_comparator_wrapper.h b/util/user_comparator_wrapper.h new file mode 100644 index 000000000..43797709c --- /dev/null +++ b/util/user_comparator_wrapper.h @@ -0,0 +1,65 @@ +// Copyright (c) 2011-present, Facebook, Inc. All rights reserved. +// This source code is licensed under both the GPLv2 (found in the +// COPYING file in the root directory) and Apache 2.0 License +// (found in the LICENSE.Apache file in the root directory). +// Copyright (c) 2011 The LevelDB Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. See the AUTHORS file for names of contributors. + +#pragma once + +#include "monitoring/perf_context_imp.h" +#include "rocksdb/comparator.h" + +namespace rocksdb { + +// Wrapper of user comparator, with auto increment to +// perf_context.user_key_comparison_count. +class UserComparatorWrapper final : public Comparator { + public: + explicit UserComparatorWrapper(const Comparator* const user_cmp) + : user_comparator_(user_cmp) {} + + ~UserComparatorWrapper() = default; + + const Comparator* user_comparator() const { return user_comparator_; } + + int Compare(const Slice& a, const Slice& b) const override { + PERF_COUNTER_ADD(user_key_comparison_count, 1); + return user_comparator_->Compare(a, b); + } + + bool Equal(const Slice& a, const Slice& b) const override { + PERF_COUNTER_ADD(user_key_comparison_count, 1); + return user_comparator_->Equal(a, b); + } + + const char* Name() const override { return user_comparator_->Name(); } + + void FindShortestSeparator(std::string* start, + const Slice& limit) const override { + return user_comparator_->FindShortestSeparator(start, limit); + } + + void FindShortSuccessor(std::string* key) const override { + return user_comparator_->FindShortSuccessor(key); + } + + const Comparator* GetRootComparator() const override { + return user_comparator_->GetRootComparator(); + } + + bool IsSameLengthImmediateSuccessor(const Slice& s, + const Slice& t) const override { + return user_comparator_->IsSameLengthImmediateSuccessor(s, t); + } + + bool CanKeysWithDifferentByteContentsBeEqual() const override { + return user_comparator_->CanKeysWithDifferentByteContentsBeEqual(); + } + + private: + const Comparator* user_comparator_; +}; + +} // namespace rocksdb