From 5d17297b76336c6a246bd98b521eced0f37abed0 Mon Sep 17 00:00:00 2001 From: sdong Date: Fri, 21 Oct 2022 12:27:50 -0700 Subject: [PATCH] Make UserComparatorWrapper not Customizable (#10837) Summary: Right now UserComparatorWrapper is a Customizable object, although it is not, which introduces some intialization overhead for the object. In some benchmarks, it shows up in CPU profiling. Make it not configurable by defining most functions needed by UserComparatorWrapper to an interface and implement the interface. Pull Request resolved: https://github.com/facebook/rocksdb/pull/10837 Test Plan: Make sure existing tests pass Reviewed By: pdillinger Differential Revision: D40528511 fbshipit-source-id: 70eaac89ecd55401a26e8ed32abbc413a9617c62 --- HISTORY.md | 1 + db/db_iter.cc | 3 ++- db/version_set.cc | 2 +- util/user_comparator_wrapper.h | 44 ++++++++++------------------------ 4 files changed, 16 insertions(+), 34 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index c6f1f58ce7..d6b1075945 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -24,6 +24,7 @@ ### Performance Improvements * Try to align the compaction output file boundaries to the next level ones, which can reduce more than 10% compaction load for the default level compaction. The feature is enabled by default, to disable, set `AdvancedColumnFamilyOptions.level_compaction_dynamic_file_size` to false. As a side effect, it can create SSTs larger than the target_file_size (capped at 2x target_file_size) or smaller files. * Improve RoundRobin TTL compaction, which is going to be the same as normal RoundRobin compaction to move the compaction cursor. +* Fix a small CPU regression caused by a change that UserComparatorWrapper was made Customizable, because Customizable itself has small CPU overhead for initialization. ### New Features * Add a new option IOOptions.do_not_recurse that can be used by underlying file systems to skip recursing through sub directories and list only files in GetChildren API. diff --git a/db/db_iter.cc b/db/db_iter.cc index bce4711294..5bc9bb8d00 100644 --- a/db/db_iter.cc +++ b/db/db_iter.cc @@ -90,7 +90,8 @@ DBIter::DBIter(Env* _env, const ReadOptions& read_options, iter_.iter()->SetPinnedItersMgr(&pinned_iters_mgr_); } status_.PermitUncheckedError(); - assert(timestamp_size_ == user_comparator_.timestamp_size()); + assert(timestamp_size_ == + user_comparator_.user_comparator()->timestamp_size()); } Status DBIter::GetProperty(std::string prop_name, std::string* prop) { diff --git a/db/version_set.cc b/db/version_set.cc index af872f8cb3..284fa1ca99 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -1239,7 +1239,7 @@ void LevelIterator::Seek(const Slice& target) { prefix_extractor_ != nullptr && !read_options_.total_order_seek && !read_options_.auto_prefix_mode && file_index_ < flevel_->num_files - 1) { - size_t ts_sz = user_comparator_.timestamp_size(); + size_t ts_sz = user_comparator_.user_comparator()->timestamp_size(); Slice target_user_key_without_ts = ExtractUserKeyAndStripTimestamp(target, ts_sz); Slice next_file_first_user_key_without_ts = diff --git a/util/user_comparator_wrapper.h b/util/user_comparator_wrapper.h index c40e04a76c..59ebada12d 100644 --- a/util/user_comparator_wrapper.h +++ b/util/user_comparator_wrapper.h @@ -15,65 +15,45 @@ namespace ROCKSDB_NAMESPACE { // Wrapper of user comparator, with auto increment to // perf_context.user_key_comparison_count. -class UserComparatorWrapper final : public Comparator { +class UserComparatorWrapper { public: // `UserComparatorWrapper`s constructed with the default constructor are not // usable and will segfault on any attempt to use them for comparisons. UserComparatorWrapper() : user_comparator_(nullptr) {} explicit UserComparatorWrapper(const Comparator* const user_cmp) - : Comparator(user_cmp->timestamp_size()), user_comparator_(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 { + int Compare(const Slice& a, const Slice& b) const { PERF_COUNTER_ADD(user_key_comparison_count, 1); return user_comparator_->Compare(a, b); } - bool Equal(const Slice& a, const Slice& b) const override { + bool Equal(const Slice& a, const Slice& b) const { 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(); - } - - int CompareTimestamp(const Slice& ts1, const Slice& ts2) const override { + int CompareTimestamp(const Slice& ts1, const Slice& ts2) const { return user_comparator_->CompareTimestamp(ts1, ts2); } - using Comparator::CompareWithoutTimestamp; + int CompareWithoutTimestamp(const Slice& a, const Slice& b) const { + PERF_COUNTER_ADD(user_key_comparison_count, 1); + return user_comparator_->CompareWithoutTimestamp(a, b); + } + int CompareWithoutTimestamp(const Slice& a, bool a_has_ts, const Slice& b, - bool b_has_ts) const override { + bool b_has_ts) const { PERF_COUNTER_ADD(user_key_comparison_count, 1); return user_comparator_->CompareWithoutTimestamp(a, a_has_ts, b, b_has_ts); } - bool EqualWithoutTimestamp(const Slice& a, const Slice& b) const override { + bool EqualWithoutTimestamp(const Slice& a, const Slice& b) const { return user_comparator_->EqualWithoutTimestamp(a, b); }