From 6e63e77af1ee93e81b5bcd625cf2353f48b94aee Mon Sep 17 00:00:00 2001 From: mrambacher Date: Wed, 25 Aug 2021 17:46:31 -0700 Subject: [PATCH] Make Configurable/Customizable options copyable (#8704) Summary: The atomic variable "is_prepared_" was keeping Configurable objects from being copy-constructed. Removed the atomic to allow copies. Since the variable is only changed from false to true (and never back), there is no reason it had to be atomic. Added tests that simple Configurable and Customizable objects can be put on the stack and copied. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8704 Reviewed By: anand1976 Differential Revision: D30530526 Pulled By: ltamasi fbshipit-source-id: 4dd4439b3e5ad7fa396573d0b25d9fb709160576 --- include/rocksdb/configurable.h | 9 ------ options/configurable.cc | 5 ---- options/configurable_test.cc | 34 +++++++++++++++++++++ options/customizable_test.cc | 55 ++++++++++++++++++++++------------ options/options_test.cc | 2 -- 5 files changed, 70 insertions(+), 35 deletions(-) diff --git a/include/rocksdb/configurable.h b/include/rocksdb/configurable.h index f43d78f864..de693eb9e1 100644 --- a/include/rocksdb/configurable.h +++ b/include/rocksdb/configurable.h @@ -56,7 +56,6 @@ class Configurable { }; public: - Configurable(); virtual ~Configurable() {} // Returns the raw pointer of the named options that is used by this @@ -262,10 +261,6 @@ class Configurable { virtual Status ValidateOptions(const DBOptions& db_opts, const ColumnFamilyOptions& cf_opts) const; - // Returns true if this object has been initialized via PrepareOptions, false - // otherwise. Once an object has been prepared, only mutable options may be - // changed. - virtual bool IsPrepared() const { return is_prepared_; } // Splits the input opt_value into the ID field and the remaining options. // The input opt_value can be in the form of "name" or "name=value @@ -286,10 +281,6 @@ class Configurable { std::string* id, std::unordered_map* options); protected: - // True once the object is prepared. Once the object is prepared, only - // mutable options can be configured. - std::atomic is_prepared_; - // Returns the raw pointer for the associated named option. // The name is typically the name of an option registered via the // Classes may override this method to provide further specialization (such as diff --git a/options/configurable.cc b/options/configurable.cc index 5469c96d6e..a98d2646b4 100644 --- a/options/configurable.cc +++ b/options/configurable.cc @@ -17,8 +17,6 @@ namespace ROCKSDB_NAMESPACE { -Configurable::Configurable() : is_prepared_(false) {} - void Configurable::RegisterOptions( const std::string& name, void* opt_ptr, const std::unordered_map* type_map) { @@ -68,9 +66,6 @@ Status Configurable::PrepareOptions(const ConfigOptions& opts) { #else (void)opts; #endif // ROCKSDB_LITE - if (status.ok()) { - is_prepared_ = true; - } return status; } diff --git a/options/configurable_test.cc b/options/configurable_test.cc index 5983e2dc61..fd412528fd 100644 --- a/options/configurable_test.cc +++ b/options/configurable_test.cc @@ -331,6 +331,40 @@ TEST_F(ConfigurableTest, PrepareOptionsTest) { ASSERT_EQ(*up, 0); } +TEST_F(ConfigurableTest, CopyObjectTest) { + class CopyConfigurable : public Configurable { + public: + CopyConfigurable() : prepared_(0), validated_(0) {} + Status PrepareOptions(const ConfigOptions& options) override { + prepared_++; + return Configurable::PrepareOptions(options); + } + Status ValidateOptions(const DBOptions& db_opts, + const ColumnFamilyOptions& cf_opts) const override { + validated_++; + return Configurable::ValidateOptions(db_opts, cf_opts); + } + int prepared_; + mutable int validated_; + }; + + CopyConfigurable c1; + ConfigOptions config_options; + Options options; + + ASSERT_OK(c1.PrepareOptions(config_options)); + ASSERT_OK(c1.ValidateOptions(options, options)); + ASSERT_EQ(c1.prepared_, 1); + ASSERT_EQ(c1.validated_, 1); + CopyConfigurable c2 = c1; + ASSERT_OK(c1.PrepareOptions(config_options)); + ASSERT_OK(c1.ValidateOptions(options, options)); + ASSERT_EQ(c2.prepared_, 1); + ASSERT_EQ(c2.validated_, 1); + ASSERT_EQ(c1.prepared_, 2); + ASSERT_EQ(c1.validated_, 2); +} + TEST_F(ConfigurableTest, MutableOptionsTest) { static std::unordered_map imm_option_info = { #ifndef ROCKSDB_LITE diff --git a/options/customizable_test.cc b/options/customizable_test.cc index cb1700f69d..193bfe5f67 100644 --- a/options/customizable_test.cc +++ b/options/customizable_test.cc @@ -553,7 +553,6 @@ TEST_F(CustomizableTest, PrepareOptionsTest) { ConfigOptions prepared(config_options_); prepared.invoke_prepare_options = true; - ASSERT_FALSE(base->IsPrepared()); ASSERT_OK(base->ConfigureFromString( prepared, "unique=A_1; shared={id=B;string=s}; pointer.id=S")); SimpleOptions* simple = base->GetOptions(); @@ -561,10 +560,6 @@ TEST_F(CustomizableTest, PrepareOptionsTest) { ASSERT_NE(simple->cu, nullptr); ASSERT_NE(simple->cs, nullptr); ASSERT_NE(simple->cp, nullptr); - ASSERT_TRUE(base->IsPrepared()); - ASSERT_TRUE(simple->cu->IsPrepared()); - ASSERT_TRUE(simple->cs->IsPrepared()); - ASSERT_TRUE(simple->cp->IsPrepared()); delete simple->cp; base.reset(new SimpleConfigurable()); ASSERT_OK(base->ConfigureFromString( @@ -575,16 +570,8 @@ TEST_F(CustomizableTest, PrepareOptionsTest) { ASSERT_NE(simple->cu, nullptr); ASSERT_NE(simple->cs, nullptr); ASSERT_NE(simple->cp, nullptr); - ASSERT_FALSE(base->IsPrepared()); - ASSERT_FALSE(simple->cu->IsPrepared()); - ASSERT_FALSE(simple->cs->IsPrepared()); - ASSERT_FALSE(simple->cp->IsPrepared()); ASSERT_OK(base->PrepareOptions(config_options_)); - ASSERT_TRUE(base->IsPrepared()); - ASSERT_TRUE(simple->cu->IsPrepared()); - ASSERT_TRUE(simple->cs->IsPrepared()); - ASSERT_TRUE(simple->cp->IsPrepared()); delete simple->cp; base.reset(new SimpleConfigurable()); simple = base->GetOptions(); @@ -597,21 +584,16 @@ TEST_F(CustomizableTest, PrepareOptionsTest) { ASSERT_OK( base->ConfigureFromString(prepared, "unique={id=P; can_prepare=true}")); ASSERT_NE(simple->cu, nullptr); - ASSERT_TRUE(simple->cu->IsPrepared()); ASSERT_OK(base->ConfigureFromString(config_options_, "unique={id=P; can_prepare=true}")); ASSERT_NE(simple->cu, nullptr); - ASSERT_FALSE(simple->cu->IsPrepared()); ASSERT_OK(simple->cu->PrepareOptions(prepared)); - ASSERT_TRUE(simple->cu->IsPrepared()); ASSERT_OK(base->ConfigureFromString(config_options_, "unique={id=P; can_prepare=false}")); ASSERT_NE(simple->cu, nullptr); - ASSERT_FALSE(simple->cu->IsPrepared()); ASSERT_NOK(simple->cu->PrepareOptions(prepared)); - ASSERT_FALSE(simple->cu->IsPrepared()); } namespace { @@ -688,6 +670,42 @@ TEST_F(CustomizableTest, WrappedInnerTest) { ASSERT_EQ(wc2->CheckedCast(), ac.get()); } +TEST_F(CustomizableTest, CopyObjectTest) { + class CopyCustomizable : public Customizable { + public: + CopyCustomizable() : prepared_(0), validated_(0) {} + const char* Name() const override { return "CopyCustomizable"; } + + Status PrepareOptions(const ConfigOptions& options) override { + prepared_++; + return Customizable::PrepareOptions(options); + } + Status ValidateOptions(const DBOptions& db_opts, + const ColumnFamilyOptions& cf_opts) const override { + validated_++; + return Customizable::ValidateOptions(db_opts, cf_opts); + } + int prepared_; + mutable int validated_; + }; + + CopyCustomizable c1; + ConfigOptions config_options; + Options options; + + ASSERT_OK(c1.PrepareOptions(config_options)); + ASSERT_OK(c1.ValidateOptions(options, options)); + ASSERT_EQ(c1.prepared_, 1); + ASSERT_EQ(c1.validated_, 1); + CopyCustomizable c2 = c1; + ASSERT_OK(c1.PrepareOptions(config_options)); + ASSERT_OK(c1.ValidateOptions(options, options)); + ASSERT_EQ(c2.prepared_, 1); + ASSERT_EQ(c2.validated_, 1); + ASSERT_EQ(c1.prepared_, 2); + ASSERT_EQ(c1.validated_, 2); +} + TEST_F(CustomizableTest, TestStringDepth) { class ShallowCustomizable : public Customizable { public: @@ -983,7 +1001,6 @@ TEST_F(CustomizableTest, MutableOptionsTest) { std::string opt_str; ConfigOptions options = config_options_; - ASSERT_FALSE(mc.IsPrepared()); ASSERT_OK(mc.ConfigureOption(options, "mutable", "{id=B;}")); options.mutable_options_only = true; ASSERT_OK(mc.GetOptionString(options, &opt_str)); diff --git a/options/options_test.cc b/options/options_test.cc index 08496aa044..2a478a0e06 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -1482,13 +1482,11 @@ TEST_F(OptionsTest, MutableTableOptions) { bbtf.reset(NewBlockBasedTableFactory()); auto bbto = bbtf->GetOptions(); ASSERT_NE(bbto, nullptr); - ASSERT_FALSE(bbtf->IsPrepared()); ASSERT_OK(bbtf->ConfigureOption(config_options, "block_align", "true")); ASSERT_OK(bbtf->ConfigureOption(config_options, "block_size", "1024")); ASSERT_EQ(bbto->block_align, true); ASSERT_EQ(bbto->block_size, 1024); ASSERT_OK(bbtf->PrepareOptions(config_options)); - ASSERT_TRUE(bbtf->IsPrepared()); config_options.mutable_options_only = true; ASSERT_OK(bbtf->ConfigureOption(config_options, "block_size", "1024")); ASSERT_EQ(bbto->block_align, true);