From fd1ad05917f84c2c15ff489b23cdfcc6ebcc02a6 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Wed, 26 May 2021 16:34:21 -0400 Subject: [PATCH 1/6] Make Comparator into a Customizable Object --- include/rocksdb/comparator.h | 13 ++++- include/rocksdb/utilities/options_type.h | 1 - options/cf_options.cc | 40 +++++++++------ options/customizable_test.cc | 34 ++++++++++++- options/options_helper.cc | 17 ------- test_util/testutil.h | 6 +-- util/comparator.cc | 62 ++++++++++++++++++++++-- 7 files changed, 128 insertions(+), 45 deletions(-) diff --git a/include/rocksdb/comparator.h b/include/rocksdb/comparator.h index 37c2925bc33..013fbfe6b7d 100644 --- a/include/rocksdb/comparator.h +++ b/include/rocksdb/comparator.h @@ -10,6 +10,7 @@ #include +#include "rocksdb/customizable.h" #include "rocksdb/rocksdb_namespace.h" namespace ROCKSDB_NAMESPACE { @@ -20,7 +21,7 @@ class Slice; // used as keys in an sstable or a database. A Comparator implementation // must be thread-safe since rocksdb may invoke its methods concurrently // from multiple threads. -class Comparator { +class Comparator : public Customizable { public: Comparator() : timestamp_size_(0) {} @@ -37,7 +38,17 @@ class Comparator { virtual ~Comparator() {} + static Status CreateFromString(const ConfigOptions& opts, + const std::string& id, + const Comparator** comp); static const char* Type() { return "Comparator"; } + static const char* kBytewiseClassName() { + return "leveldb.BytewiseComparator"; + } + static const char* kReverseBytewiseClassName() { + return "rocksdb.ReverseBytewiseComparator"; + } + // Three-way comparison. Returns value: // < 0 iff "a" < "b", // == 0 iff "a" == "b", diff --git a/include/rocksdb/utilities/options_type.h b/include/rocksdb/utilities/options_type.h index 7057c78ac20..d7d38898507 100644 --- a/include/rocksdb/utilities/options_type.h +++ b/include/rocksdb/utilities/options_type.h @@ -35,7 +35,6 @@ enum class OptionType { kCompactionPri, kSliceTransform, kCompressionType, - kComparator, kCompactionFilter, kCompactionFilterFactory, kCompactionStopStyle, diff --git a/options/cf_options.cc b/options/cf_options.cc index 005a90c8554..3f2b8bb7320 100644 --- a/options/cf_options.cc +++ b/options/cf_options.cc @@ -535,22 +535,30 @@ static std::unordered_map OptionVerificationType::kNormal, OptionTypeFlags::kNone, {0, OptionType::kCompressionType})}, {"comparator", - {offset_of(&ImmutableCFOptions::user_comparator), - OptionType::kComparator, OptionVerificationType::kByName, - OptionTypeFlags::kCompareLoose, - // Parses the string and sets the corresponding comparator - [](const ConfigOptions& opts, const std::string& /*name*/, - const std::string& value, void* addr) { - auto old_comparator = static_cast(addr); - const Comparator* new_comparator = *old_comparator; - Status status = - opts.registry->NewStaticObject(value, &new_comparator); - if (status.ok()) { - *old_comparator = new_comparator; - return status; - } - return Status::OK(); - }}}, + OptionTypeInfo::AsCustomRawPtr( + offset_of(&ImmutableCFOptions::user_comparator), + OptionVerificationType::kByName, OptionTypeFlags::kCompareLoose, + // Serializes a Comparator + [](const ConfigOptions& /*opts*/, const std::string&, + const void* addr, std::string* value) { + // it's a const pointer of const Comparator* + const auto* ptr = static_cast(addr); + + // Since the user-specified comparator will be wrapped by + // InternalKeyComparator, we should persist the user-specified + // one instead of InternalKeyComparator. + if (*ptr == nullptr) { + *value = kNullptrString; + } else { + const Comparator* root_comp = (*ptr)->GetRootComparator(); + if (root_comp == nullptr) { + root_comp = (*ptr); + } + *value = root_comp->Name(); + } + return Status::OK(); + }, + /* Use the default match function*/ nullptr)}, {"memtable_insert_with_hint_prefix_extractor", {offset_of( &ImmutableCFOptions::memtable_insert_with_hint_prefix_extractor), diff --git a/options/customizable_test.cc b/options/customizable_test.cc index 9cf6b912688..9eb4ca6ada5 100644 --- a/options/customizable_test.cc +++ b/options/customizable_test.cc @@ -707,8 +707,17 @@ static int RegisterTestObjects(ObjectLibrary& library, guard->reset(new mock::MockTableFactory()); return guard->get(); }); + library.Register( + test::SimpleSuffixReverseComparator::kClassName(), + [](const std::string& /*uri*/, std::unique_ptr* /*guard*/, + std::string* /* errmsg */) { + static test::SimpleSuffixReverseComparator ssrc; + return &ssrc; + }); + return static_cast(library.GetFactoryCount(&num_types)); } +#endif // !ROCKSDB_LITE static int RegisterLocalObjects(ObjectLibrary& library, const std::string& /*arg*/) { @@ -755,7 +764,30 @@ TEST_F(LoadCustomizableTest, LoadTableFactoryTest) { ASSERT_STREQ(factory->Name(), "MockTable"); } } -#endif // !ROCKSDB_LITE + +TEST_F(LoadCustomizableTest, LoadComparatorTest) { + const Comparator* result = nullptr; + ASSERT_NOK(Comparator::CreateFromString( + config_options_, test::SimpleSuffixReverseComparator::kClassName(), + &result)); + ASSERT_OK(Comparator::CreateFromString( + config_options_, Comparator::kBytewiseClassName(), &result)); + ASSERT_NE(result, nullptr); + ASSERT_STREQ(result->Name(), Comparator::kBytewiseClassName()); + ASSERT_OK(Comparator::CreateFromString( + config_options_, Comparator::kReverseBytewiseClassName(), &result)); + ASSERT_NE(result, nullptr); + ASSERT_STREQ(result->Name(), Comparator::kReverseBytewiseClassName()); + + if (RegisterTests("Test")) { + ASSERT_OK(Comparator::CreateFromString( + config_options_, test::SimpleSuffixReverseComparator::kClassName(), + &result)); + ASSERT_NE(result, nullptr); + ASSERT_STREQ(result->Name(), + test::SimpleSuffixReverseComparator::kClassName()); + } +} } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) { diff --git a/options/options_helper.cc b/options/options_helper.cc index 823ef4a4544..a554ca358a8 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -562,23 +562,6 @@ bool SerializeSingleOptionHelper(const void* opt_address, : kNullptrString; break; } - case OptionType::kComparator: { - // it's a const pointer of const Comparator* - const auto* ptr = static_cast(opt_address); - // Since the user-specified comparator will be wrapped by - // InternalKeyComparator, we should persist the user-specified one - // instead of InternalKeyComparator. - if (*ptr == nullptr) { - *value = kNullptrString; - } else { - const Comparator* root_comp = (*ptr)->GetRootComparator(); - if (root_comp == nullptr) { - root_comp = (*ptr); - } - *value = root_comp->Name(); - } - break; - } case OptionType::kCompactionFilter: { // it's a const pointer of const CompactionFilter* const auto* ptr = diff --git a/test_util/testutil.h b/test_util/testutil.h index 5992783b95c..39fd5d9fb63 100644 --- a/test_util/testutil.h +++ b/test_util/testutil.h @@ -98,10 +98,8 @@ class PlainInternalKeyComparator : public InternalKeyComparator { class SimpleSuffixReverseComparator : public Comparator { public: SimpleSuffixReverseComparator() {} - - virtual const char* Name() const override { - return "SimpleSuffixReverseComparator"; - } + static const char* kClassName() { return "SimpleSuffixReverseComparator"; } + virtual const char* Name() const override { return kClassName(); } virtual int Compare(const Slice& a, const Slice& b) const override { Slice prefix_a = Slice(a.data(), 8); diff --git a/util/comparator.cc b/util/comparator.cc index f115a73e953..14a873ef966 100644 --- a/util/comparator.cc +++ b/util/comparator.cc @@ -8,9 +8,14 @@ // found in the LICENSE file. See the AUTHORS file for names of contributors. #include "rocksdb/comparator.h" + #include + #include #include +#include + +#include "options/customizable_helper.h" #include "port/port.h" #include "rocksdb/slice.h" @@ -20,8 +25,8 @@ namespace { class BytewiseComparatorImpl : public Comparator { public: BytewiseComparatorImpl() { } - - const char* Name() const override { return "leveldb.BytewiseComparator"; } + static const char* kClassName() { return kBytewiseClassName(); } + const char* Name() const override { return kClassName(); } int Compare(const Slice& a, const Slice& b) const override { return a.compare(b); @@ -139,9 +144,8 @@ class ReverseBytewiseComparatorImpl : public BytewiseComparatorImpl { public: ReverseBytewiseComparatorImpl() { } - const char* Name() const override { - return "rocksdb.ReverseBytewiseComparator"; - } + static const char* kClassName() { return kReverseBytewiseClassName(); } + const char* Name() const override { return kClassName(); } int Compare(const Slice& a, const Slice& b) const override { return -a.compare(b); @@ -220,4 +224,52 @@ const Comparator* ReverseBytewiseComparator() { return &rbytewise; } +#ifndef ROCKSDB_LITE +static int RegisterBuiltinComparators(ObjectLibrary& library, + const std::string& /*arg*/) { + library.Register( + BytewiseComparatorImpl::kClassName(), + [](const std::string& /*uri*/, std::unique_ptr* /*guard */, + std::string* /* errmsg */) { + return const_cast(BytewiseComparator()); + }); + library.Register( + ReverseBytewiseComparatorImpl::kClassName(), + [](const std::string& /*uri*/, std::unique_ptr* /*guard */, + std::string* /* errmsg */) { + return const_cast(ReverseBytewiseComparator()); + }); + return 2; +} +#endif // ROCKSDB_LITE + +static bool LoadComparator(const std::string& id, Comparator** result) { + bool success = true; + if (id == BytewiseComparatorImpl::kClassName()) { + *result = const_cast(BytewiseComparator()); + } else if (id == ReverseBytewiseComparatorImpl::kClassName()) { + *result = const_cast(ReverseBytewiseComparator()); + } else { + success = false; + } + return success; +} + +Status Comparator::CreateFromString(const ConfigOptions& config_options, + const std::string& value, + const Comparator** result) { + Comparator* comparator = const_cast(*result); +#ifndef ROCKSDB_LITE + static std::once_flag once; + std::call_once(once, [&]() { + RegisterBuiltinComparators(*(ObjectLibrary::Default().get()), ""); + }); +#endif // ROCKSDB_LITE + Status s = LoadStaticObject(config_options, value, LoadComparator, + &comparator); + if (s.ok()) { + *result = const_cast(comparator); + } + return s; +} } // namespace ROCKSDB_NAMESPACE From 286a415663d0123ccd4ed0662ea2d85890d2e82e Mon Sep 17 00:00:00 2001 From: mrambacher Date: Wed, 26 May 2021 19:35:09 -0400 Subject: [PATCH 2/6] Fix LITE mode --- options/customizable_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/options/customizable_test.cc b/options/customizable_test.cc index 9eb4ca6ada5..5298b80f464 100644 --- a/options/customizable_test.cc +++ b/options/customizable_test.cc @@ -717,7 +717,6 @@ static int RegisterTestObjects(ObjectLibrary& library, return static_cast(library.GetFactoryCount(&num_types)); } -#endif // !ROCKSDB_LITE static int RegisterLocalObjects(ObjectLibrary& library, const std::string& /*arg*/) { @@ -725,6 +724,7 @@ static int RegisterLocalObjects(ObjectLibrary& library, // Load any locally defined objects here return static_cast(library.GetFactoryCount(&num_types)); } +#endif // !ROCKSDB_LITE class LoadCustomizableTest : public testing::Test { public: From ce7e30e53205abb468f52bd64b4aa44eeae7f439 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Wed, 26 May 2021 21:53:14 -0400 Subject: [PATCH 3/6] Attempt to fix UBSAN failures --- util/comparator.cc | 84 ++++++++++++++++++++++++++++++---------------- 1 file changed, 55 insertions(+), 29 deletions(-) diff --git a/util/comparator.cc b/util/comparator.cc index 14a873ef966..4c2e7d024d6 100644 --- a/util/comparator.cc +++ b/util/comparator.cc @@ -15,9 +15,10 @@ #include #include -#include "options/customizable_helper.h" +#include "options/configurable_helper.h" #include "port/port.h" #include "rocksdb/slice.h" +#include "rocksdb/utilities/object_registry.h" namespace ROCKSDB_NAMESPACE { @@ -227,49 +228,74 @@ const Comparator* ReverseBytewiseComparator() { #ifndef ROCKSDB_LITE static int RegisterBuiltinComparators(ObjectLibrary& library, const std::string& /*arg*/) { - library.Register( + library.Register( BytewiseComparatorImpl::kClassName(), - [](const std::string& /*uri*/, std::unique_ptr* /*guard */, - std::string* /* errmsg */) { - return const_cast(BytewiseComparator()); - }); - library.Register( + [](const std::string& /*uri*/, + std::unique_ptr* /*guard */, + std::string* /* errmsg */) { return BytewiseComparator(); }); + library.Register( ReverseBytewiseComparatorImpl::kClassName(), - [](const std::string& /*uri*/, std::unique_ptr* /*guard */, - std::string* /* errmsg */) { - return const_cast(ReverseBytewiseComparator()); - }); + [](const std::string& /*uri*/, + std::unique_ptr* /*guard */, + std::string* /* errmsg */) { return ReverseBytewiseComparator(); }); return 2; } #endif // ROCKSDB_LITE -static bool LoadComparator(const std::string& id, Comparator** result) { - bool success = true; - if (id == BytewiseComparatorImpl::kClassName()) { - *result = const_cast(BytewiseComparator()); - } else if (id == ReverseBytewiseComparatorImpl::kClassName()) { - *result = const_cast(ReverseBytewiseComparator()); - } else { - success = false; - } - return success; -} - Status Comparator::CreateFromString(const ConfigOptions& config_options, const std::string& value, const Comparator** result) { - Comparator* comparator = const_cast(*result); #ifndef ROCKSDB_LITE static std::once_flag once; std::call_once(once, [&]() { RegisterBuiltinComparators(*(ObjectLibrary::Default().get()), ""); }); #endif // ROCKSDB_LITE - Status s = LoadStaticObject(config_options, value, LoadComparator, - &comparator); - if (s.ok()) { - *result = const_cast(comparator); + std::string id; + std::unordered_map opt_map; + Status status = + ConfigurableHelper::GetOptionsMap(value, *result, &id, &opt_map); + if (!status.ok()) { // GetOptionsMap failed + return status; + } + std::string curr_opts; +#ifndef ROCKSDB_LITE + if (*result != nullptr && (*result)->GetId() == id) { + // Try to get the existing options, ignoring any errors + ConfigOptions embedded = config_options; + embedded.delimiter = ";"; + (*result)->GetOptionString(embedded, &curr_opts).PermitUncheckedError(); + } +#endif + if (id == BytewiseComparatorImpl::kClassName()) { + *result = BytewiseComparator(); + } else if (id == ReverseBytewiseComparatorImpl::kClassName()) { + *result = ReverseBytewiseComparator(); + } else if (value.empty()) { + // No Id and no options. Clear the object + *result = nullptr; + return Status::OK(); + } else if (id.empty()) { // We have no Id but have options. Not good + return Status::NotSupported("Cannot reset object ", id); + } else { +#ifndef ROCKSDB_LITE + status = config_options.registry->NewStaticObject(id, result); +#else + status = Status::NotSupported("Cannot load object in LITE mode ", id); +#endif // ROCKSDB_LITE + if (!status.ok()) { + if (config_options.ignore_unsupported_options && + status.IsNotSupported()) { + return Status::OK(); + } else { + return status; + } + } else if (!curr_opts.empty() || !opt_map.empty()) { + Comparator* comparator = const_cast(*result); + status = ConfigurableHelper::ConfigureNewObject( + config_options, comparator, id, curr_opts, opt_map); + } } - return s; + return status; } } // namespace ROCKSDB_NAMESPACE From 16e995853f31b2b842d216b8f19e1706a3dc41a3 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Thu, 27 May 2021 08:30:24 -0400 Subject: [PATCH 4/6] Fix customizable test for UBSAN --- options/customizable_test.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/options/customizable_test.cc b/options/customizable_test.cc index 5298b80f464..0dd73c6b5a5 100644 --- a/options/customizable_test.cc +++ b/options/customizable_test.cc @@ -707,9 +707,10 @@ static int RegisterTestObjects(ObjectLibrary& library, guard->reset(new mock::MockTableFactory()); return guard->get(); }); - library.Register( + library.Register( test::SimpleSuffixReverseComparator::kClassName(), - [](const std::string& /*uri*/, std::unique_ptr* /*guard*/, + [](const std::string& /*uri*/, + std::unique_ptr* /*guard*/, std::string* /* errmsg */) { static test::SimpleSuffixReverseComparator ssrc; return &ssrc; From cdc40cfe533ae4f8ec58bddf099d089058095763 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Mon, 7 Jun 2021 10:28:45 -0400 Subject: [PATCH 5/6] Fix TSAN Error --- include/rocksdb/configurable.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/rocksdb/configurable.h b/include/rocksdb/configurable.h index b56072dbeae..78ca771bad4 100644 --- a/include/rocksdb/configurable.h +++ b/include/rocksdb/configurable.h @@ -8,6 +8,7 @@ #pragma once +#include #include #include #include @@ -269,7 +270,7 @@ class Configurable { protected: // True once the object is prepared. Once the object is prepared, only // mutable options can be configured. - bool prepared_; + std::atomic prepared_; // Returns the raw pointer for the associated named option. // The name is typically the name of an option registered via the From 2bda2c6b0f551ddad34e6c5bbfe836a0e285abaf Mon Sep 17 00:00:00 2001 From: mrambacher Date: Tue, 8 Jun 2021 21:28:15 -0400 Subject: [PATCH 6/6] Remove names of derived classes from public/base class --- include/rocksdb/comparator.h | 6 ------ options/customizable_test.cc | 17 +++++++++-------- util/comparator.cc | 6 ++++-- 3 files changed, 13 insertions(+), 16 deletions(-) diff --git a/include/rocksdb/comparator.h b/include/rocksdb/comparator.h index 013fbfe6b7d..fe96eb0c756 100644 --- a/include/rocksdb/comparator.h +++ b/include/rocksdb/comparator.h @@ -42,12 +42,6 @@ class Comparator : public Customizable { const std::string& id, const Comparator** comp); static const char* Type() { return "Comparator"; } - static const char* kBytewiseClassName() { - return "leveldb.BytewiseComparator"; - } - static const char* kReverseBytewiseClassName() { - return "rocksdb.ReverseBytewiseComparator"; - } // Three-way comparison. Returns value: // < 0 iff "a" < "b", diff --git a/options/customizable_test.cc b/options/customizable_test.cc index 0dd73c6b5a5..3263b6ac973 100644 --- a/options/customizable_test.cc +++ b/options/customizable_test.cc @@ -767,18 +767,19 @@ TEST_F(LoadCustomizableTest, LoadTableFactoryTest) { } TEST_F(LoadCustomizableTest, LoadComparatorTest) { + const Comparator* bytewise = BytewiseComparator(); + const Comparator* reverse = ReverseBytewiseComparator(); + const Comparator* result = nullptr; ASSERT_NOK(Comparator::CreateFromString( config_options_, test::SimpleSuffixReverseComparator::kClassName(), &result)); - ASSERT_OK(Comparator::CreateFromString( - config_options_, Comparator::kBytewiseClassName(), &result)); - ASSERT_NE(result, nullptr); - ASSERT_STREQ(result->Name(), Comparator::kBytewiseClassName()); - ASSERT_OK(Comparator::CreateFromString( - config_options_, Comparator::kReverseBytewiseClassName(), &result)); - ASSERT_NE(result, nullptr); - ASSERT_STREQ(result->Name(), Comparator::kReverseBytewiseClassName()); + ASSERT_OK( + Comparator::CreateFromString(config_options_, bytewise->Name(), &result)); + ASSERT_EQ(result, bytewise); + ASSERT_OK( + Comparator::CreateFromString(config_options_, reverse->Name(), &result)); + ASSERT_EQ(result, reverse); if (RegisterTests("Test")) { ASSERT_OK(Comparator::CreateFromString( diff --git a/util/comparator.cc b/util/comparator.cc index 4c2e7d024d6..0ed391f9be7 100644 --- a/util/comparator.cc +++ b/util/comparator.cc @@ -26,7 +26,7 @@ namespace { class BytewiseComparatorImpl : public Comparator { public: BytewiseComparatorImpl() { } - static const char* kClassName() { return kBytewiseClassName(); } + static const char* kClassName() { return "leveldb.BytewiseComparator"; } const char* Name() const override { return kClassName(); } int Compare(const Slice& a, const Slice& b) const override { @@ -145,7 +145,9 @@ class ReverseBytewiseComparatorImpl : public BytewiseComparatorImpl { public: ReverseBytewiseComparatorImpl() { } - static const char* kClassName() { return kReverseBytewiseClassName(); } + static const char* kClassName() { + return "rocksdb.ReverseBytewiseComparator"; + } const char* Name() const override { return kClassName(); } int Compare(const Slice& a, const Slice& b) const override {