From 37ec9d0c12abe071f321fe6989f6e42fbcc56c93 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Thu, 27 Jan 2022 10:04:35 -0800 Subject: [PATCH] Improve performance of SliceTransform::AsString (#9401) Summary: 1. Removed the options from the Capped/Fixed SliceTransforms. Instead these classes are created with id.number. This allows the GetID() id to be calculated and stored at class construction time. This change puts the construction back to similar to how it was prior to the Customizable changes for SliceTransform. 2. Improve the performance of AsString by using the ID only if there are no option properties (which is the case for all of the builtin transforms). Ran tests of calling AsString in a loop 5M times and found approximately a 10x performance increase vs the original code. Pull Request resolved: https://github.com/facebook/rocksdb/pull/9401 Reviewed By: pdillinger Differential Revision: D33668672 Pulled By: mrambacher fbshipit-source-id: d0075912c6ece8ed754ee543bc6b0b49a169b309 --- include/rocksdb/configurable.h | 3 ++ include/rocksdb/slice_transform.h | 2 +- options/options_test.cc | 35 +++++++++++-- util/slice.cc | 87 +++++++++++++++++-------------- 4 files changed, 83 insertions(+), 44 deletions(-) diff --git a/include/rocksdb/configurable.h b/include/rocksdb/configurable.h index ee1afd87c..60ae89f97 100644 --- a/include/rocksdb/configurable.h +++ b/include/rocksdb/configurable.h @@ -388,6 +388,9 @@ class Configurable { const std::string& name, void* opt_ptr, const std::unordered_map* opt_map); + // Returns true if there are registered options for this Configurable object + inline bool HasRegisteredOptions() const { return !options_.empty(); } + private: // Contains the collection of options (name, opt_ptr, opt_map) associated with // this object. This collection is typically set in the constructor of the diff --git a/include/rocksdb/slice_transform.h b/include/rocksdb/slice_transform.h index 7369e2887..334aa81e0 100644 --- a/include/rocksdb/slice_transform.h +++ b/include/rocksdb/slice_transform.h @@ -47,7 +47,7 @@ class SliceTransform : public Customizable { std::shared_ptr* result); // Returns a string representation of this SliceTransform, representing the ID - // and any additional properties + // and any additional properties. std::string AsString() const; // Extract a prefix from a specified key. This method is called when diff --git a/options/options_test.cc b/options/options_test.cc index 95521f1f0..544030b3c 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -2731,22 +2731,49 @@ TEST_F(OptionsTest, SliceTransformCreateFromString) { SliceTransform::CreateFromString(config_options, "fixed", &transform)); ASSERT_NOK( SliceTransform::CreateFromString(config_options, "capped", &transform)); + ASSERT_NOK( + SliceTransform::CreateFromString(config_options, "fixed:", &transform)); + ASSERT_NOK( + SliceTransform::CreateFromString(config_options, "capped:", &transform)); ASSERT_NOK(SliceTransform::CreateFromString( config_options, "rocksdb.FixedPrefix:42", &transform)); ASSERT_NOK(SliceTransform::CreateFromString( config_options, "rocksdb.CappedPrefix:42", &transform)); + ASSERT_NOK(SliceTransform::CreateFromString( + config_options, "rocksdb.FixedPrefix", &transform)); + ASSERT_NOK(SliceTransform::CreateFromString( + config_options, "rocksdb.CappedPrefix", &transform)); + ASSERT_NOK(SliceTransform::CreateFromString( + config_options, "rocksdb.FixedPrefix.", &transform)); + ASSERT_NOK(SliceTransform::CreateFromString( + config_options, "rocksdb.CappedPrefix.", &transform)); ASSERT_NOK( SliceTransform::CreateFromString(config_options, "invalid", &transform)); #ifndef ROCKSDB_LITE ASSERT_OK(SliceTransform::CreateFromString( - config_options, "id=rocksdb.CappedPrefix; length=11", &transform)); + config_options, "rocksdb.CappedPrefix.11", &transform)); ASSERT_NE(transform, nullptr); ASSERT_EQ(transform->GetId(), "rocksdb.CappedPrefix.11"); + ASSERT_TRUE(transform->IsInstanceOf("capped")); + ASSERT_TRUE(transform->IsInstanceOf("capped:11")); + ASSERT_TRUE(transform->IsInstanceOf("rocksdb.CappedPrefix")); + ASSERT_TRUE(transform->IsInstanceOf("rocksdb.CappedPrefix.11")); + ASSERT_FALSE(transform->IsInstanceOf("fixed")); + ASSERT_FALSE(transform->IsInstanceOf("fixed:11")); + ASSERT_FALSE(transform->IsInstanceOf("rocksdb.FixedPrefix")); + ASSERT_FALSE(transform->IsInstanceOf("rocksdb.FixedPrefix.11")); - ASSERT_NOK(SliceTransform::CreateFromString( - config_options, "id=rocksdb.CappedPrefix; length=11; invalid=true", - &transform)); + ASSERT_OK(SliceTransform::CreateFromString( + config_options, "rocksdb.FixedPrefix.11", &transform)); + ASSERT_TRUE(transform->IsInstanceOf("fixed")); + ASSERT_TRUE(transform->IsInstanceOf("fixed:11")); + ASSERT_TRUE(transform->IsInstanceOf("rocksdb.FixedPrefix")); + ASSERT_TRUE(transform->IsInstanceOf("rocksdb.FixedPrefix.11")); + ASSERT_FALSE(transform->IsInstanceOf("capped")); + ASSERT_FALSE(transform->IsInstanceOf("capped:11")); + ASSERT_FALSE(transform->IsInstanceOf("rocksdb.CappedPrefix")); + ASSERT_FALSE(transform->IsInstanceOf("rocksdb.CappedPrefix.11")); #endif // ROCKSDB_LITE } diff --git a/util/slice.cc b/util/slice.cc index 3c3656de9..1623c2a52 100644 --- a/util/slice.cc +++ b/util/slice.cc @@ -22,22 +22,16 @@ namespace ROCKSDB_NAMESPACE { namespace { -static std::unordered_map - slice_transform_length_info = { -#ifndef ROCKSDB_LITE - {"length", - {0, OptionType::kSizeT, OptionVerificationType::kNormal, - OptionTypeFlags::kDontSerialize | OptionTypeFlags::kCompareNever}}, -#endif // ROCKSDB_LITE -}; class FixedPrefixTransform : public SliceTransform { private: size_t prefix_len_; + std::string id_; public: explicit FixedPrefixTransform(size_t prefix_len) : prefix_len_(prefix_len) { - RegisterOptions(Name(), &prefix_len_, &slice_transform_length_info); + id_ = std::string(kClassName()) + "." + + ROCKSDB_NAMESPACE::ToString(prefix_len_); } static const char* kClassName() { return "rocksdb.FixedPrefix"; } @@ -45,10 +39,21 @@ class FixedPrefixTransform : public SliceTransform { const char* Name() const override { return kClassName(); } const char* NickName() const override { return kNickName(); } - std::string GetId() const override { - return std::string(Name()) + "." + ROCKSDB_NAMESPACE::ToString(prefix_len_); + bool IsInstanceOf(const std::string& name) const override { + if (name == id_) { + return true; + } else if (StartsWith(name, kNickName())) { + std::string alt_id = std::string(kNickName()) + ":" + + ROCKSDB_NAMESPACE::ToString(prefix_len_); + if (name == alt_id) { + return true; + } + } + return SliceTransform::IsInstanceOf(name); } + std::string GetId() const override { return id_; } + Slice Transform(const Slice& src) const override { assert(InDomain(src)); return Slice(src.data(), prefix_len_); @@ -75,18 +80,31 @@ class FixedPrefixTransform : public SliceTransform { class CappedPrefixTransform : public SliceTransform { private: size_t cap_len_; + std::string id_; public: explicit CappedPrefixTransform(size_t cap_len) : cap_len_(cap_len) { - RegisterOptions(Name(), &cap_len_, &slice_transform_length_info); + id_ = + std::string(kClassName()) + "." + ROCKSDB_NAMESPACE::ToString(cap_len_); } static const char* kClassName() { return "rocksdb.CappedPrefix"; } static const char* kNickName() { return "capped"; } const char* Name() const override { return kClassName(); } const char* NickName() const override { return kNickName(); } - std::string GetId() const override { - return std::string(Name()) + "." + ROCKSDB_NAMESPACE::ToString(cap_len_); + std::string GetId() const override { return id_; } + + bool IsInstanceOf(const std::string& name) const override { + if (name == id_) { + return true; + } else if (StartsWith(name, kNickName())) { + std::string alt_id = std::string(kNickName()) + ":" + + ROCKSDB_NAMESPACE::ToString(cap_len_); + if (name == alt_id) { + return true; + } + } + return SliceTransform::IsInstanceOf(name); } Slice Transform(const Slice& src) const override { @@ -144,8 +162,7 @@ const SliceTransform* NewNoopTransform() { return new NoopTransform; } static int RegisterBuiltinSliceTransform(ObjectLibrary& library, const std::string& /*arg*/) { // For the builtin transforms, the format is typically - // [Name] or [Name].[0-9]+ - // [NickName]:[0-9]+ + // [Name].[0-9]+ or [NickName]:[0-9]+ library.AddFactory( NoopTransform::kClassName(), [](const std::string& /*uri*/, @@ -165,17 +182,13 @@ static int RegisterBuiltinSliceTransform(ObjectLibrary& library, return guard->get(); }); library.AddFactory( - ObjectLibrary::PatternEntry(FixedPrefixTransform::kClassName(), true) + ObjectLibrary::PatternEntry(FixedPrefixTransform::kClassName(), false) .AddNumber("."), [](const std::string& uri, std::unique_ptr* guard, std::string* /*errmsg*/) { - if (uri == FixedPrefixTransform::kClassName()) { - guard->reset(NewFixedPrefixTransform(0)); - } else { - auto len = ParseSizeT( - uri.substr(strlen(FixedPrefixTransform::kClassName()) + 1)); - guard->reset(NewFixedPrefixTransform(len)); - } + auto len = ParseSizeT( + uri.substr(strlen(FixedPrefixTransform::kClassName()) + 1)); + guard->reset(NewFixedPrefixTransform(len)); return guard->get(); }); library.AddFactory( @@ -189,17 +202,13 @@ static int RegisterBuiltinSliceTransform(ObjectLibrary& library, return guard->get(); }); library.AddFactory( - ObjectLibrary::PatternEntry(CappedPrefixTransform::kClassName(), true) + ObjectLibrary::PatternEntry(CappedPrefixTransform::kClassName(), false) .AddNumber("."), [](const std::string& uri, std::unique_ptr* guard, std::string* /*errmsg*/) { - if (uri == CappedPrefixTransform::kClassName()) { - guard->reset(NewCappedPrefixTransform(0)); - } else { - auto len = ParseSizeT( - uri.substr(strlen(CappedPrefixTransform::kClassName()) + 1)); - guard->reset(NewCappedPrefixTransform(len)); - } + auto len = ParseSizeT( + uri.substr(strlen(CappedPrefixTransform::kClassName()) + 1)); + guard->reset(NewCappedPrefixTransform(len)); return guard->get(); }); size_t num_types; @@ -270,13 +279,13 @@ Status SliceTransform::CreateFromString( } std::string SliceTransform::AsString() const { -#ifndef ROCKSDB_LITE - ConfigOptions config_options; - config_options.delimiter = ";"; - return ToString(config_options); -#else - return GetId(); -#endif // ROCKSDB_LITE + if (HasRegisteredOptions()) { + ConfigOptions opts; + opts.delimiter = ";"; + return ToString(opts); + } else { + return GetId(); + } } // 2 small internal utility functions, for efficient hex conversions