From: Andrii Grynenko Date: Fri, 5 Dec 2014 22:17:50 +0000 (-0800) Subject: Remove locking when getting ptr to Singleton X-Git-Tag: v0.22.0~101 X-Git-Url: http://plrg.eecs.uci.edu/git/?a=commitdiff_plain;h=67bc1788ab32efeb4fa337e5942bcc3c8ee0f25e;p=folly.git Remove locking when getting ptr to Singleton Summary: This removes one layer on locking on the fast path, when ptr to singleton object is read from SingletonEntry. Test Plan: unit test Before: ============================================================================ folly/experimental/test/SingletonTest.cpp relative time/iter iters/s ============================================================================ NormalSingleton 335.26ps 2.98G MeyersSingleton 99.50% 336.96ps 2.97G FollySingleton 0.28% 120.64ns 8.29M ============================================================================ After: ============================================================================ folly/experimental/test/SingletonTest.cpp relative time/iter iters/s ============================================================================ NormalSingleton 336.76ps 2.97G MeyersSingleton 99.91% 337.07ps 2.97G FollySingleton 0.36% 92.69ns 10.79M ============================================================================ Reviewed By: alikhtarov@fb.com Subscribers: trunkagent, folly-diffs@ FB internal diff: D1727604 Signature: t1:1727604:1418701171:1728b516191a8ec4439e981d78634370b4bcf7a1 --- diff --git a/folly/experimental/Singleton.cpp b/folly/experimental/Singleton.cpp index dc032d3a..310d9bdb 100644 --- a/folly/experimental/Singleton.cpp +++ b/folly/experimental/Singleton.cpp @@ -40,8 +40,7 @@ void SingletonVault::destroyInstances() { for (auto type_iter = creation_order_.rbegin(); type_iter != creation_order_.rend(); ++type_iter) { - auto type = *type_iter; - destroyInstance(type); + destroyInstance(singletons_.find(*type_iter)); } } @@ -55,22 +54,17 @@ void SingletonVault::destroyInstances() { * a read lock on mutex_. * @param typeDescriptor - the type key for the removed singleton. */ -void SingletonVault::destroyInstance( - const detail::TypeDescriptor& typeDescriptor) { - auto it = singletons_.find(typeDescriptor); - CHECK(it != singletons_.end()); - auto& entry = it->second; - std::lock_guard entry_guard(entry->mutex); - if (entry->instance.use_count() > 1) { - LOG(ERROR) << "Singleton of typeDescriptor " - << typeDescriptor.name() << " has a living " +void SingletonVault::destroyInstance(SingletonMap::iterator entry_it) { + const auto& type = entry_it->first; + auto& entry = *(entry_it->second); + if (entry.instance.use_count() > 1) { + LOG(ERROR) << "Singleton of type " << type.name() << " has a living " << "reference at destroyInstances time; beware! Raw pointer " - << "is " << entry->instance.get() << " with use_count of " - << entry->instance.use_count(); + << "is " << entry.instance.get() << " with use_count of " + << entry.instance.use_count(); } - entry->instance.reset(); - entry->state = SingletonEntryState::Dead; - entry->state_condvar.notify_all(); + entry.state = SingletonEntryState::Dead; + entry.instance.reset(); } void SingletonVault::reenableInstances() { diff --git a/folly/experimental/Singleton.h b/folly/experimental/Singleton.h index 2e342077..d29527db 100644 --- a/folly/experimental/Singleton.h +++ b/folly/experimental/Singleton.h @@ -81,6 +81,7 @@ #pragma once #include #include +#include #include #include @@ -217,59 +218,53 @@ class SingletonVault { // functions. Must hold reader locks on stateMutex_ and mutex_ // when invoking this function. void registerSingletonImpl(detail::TypeDescriptor type, - CreateFunc create, - TeardownFunc teardown) { + CreateFunc create, + TeardownFunc teardown) { RWSpinLock::UpgradedHolder wh(&mutex_); - auto& entry = singletons_[type]; - if (!entry) { - entry.reset(new SingletonEntry); - } - - std::lock_guard entry_guard(entry->mutex); - CHECK(entry->instance == nullptr); - CHECK(create); - CHECK(teardown); - entry->create = create; - entry->teardown = teardown; - entry->state = SingletonEntryState::Dead; + singletons_[type] = folly::make_unique(std::move(create), + std::move(teardown)); } /* Register a mock singleton used for testing of singletons which * depend on other private singletons which cannot be otherwise injected. */ void registerMockSingleton(detail::TypeDescriptor type, - CreateFunc create, - TeardownFunc teardown) { + CreateFunc create, + TeardownFunc teardown) { RWSpinLock::ReadHolder rh(&stateMutex_); RWSpinLock::ReadHolder rhMutex(&mutex_); - auto existing_entry_it = singletons_.find(type); + auto entry_it = singletons_.find(type); // Mock singleton registration, we allow existing entry to be overridden. - if (existing_entry_it != singletons_.end()) { - // Upgrade to write lock. - RWSpinLock::UpgradedHolder whMutex(&mutex_); + if (entry_it == singletons_.end()) { + throw std::logic_error( + "Registering mock before the singleton was registered"); + } + { + auto& entry = *(entry_it->second); // Destroy existing singleton. - destroyInstance(type); - - // Remove singleton from creation order and singletons_. - // This happens only in test code and not frequently. - // Performance is not a concern here. - auto creation_order_it = std::find( - creation_order_.begin(), - creation_order_.end(), - type); - if (creation_order_it != creation_order_.end()) { - creation_order_.erase(creation_order_it); - } + std::lock_guard entry_lg(entry.mutex); + + destroyInstance(entry_it); + entry.create = create; + entry.teardown = teardown; } - // This method will re-upgrade to write lock for &mutex_. - registerSingletonImpl( - type, - create, - teardown); + // Upgrade to write lock. + RWSpinLock::UpgradedHolder whMutex(&mutex_); + + // Remove singleton from creation order and singletons_. + // This happens only in test code and not frequently. + // Performance is not a concern here. + auto creation_order_it = std::find( + creation_order_.begin(), + creation_order_.end(), + type); + if (creation_order_it != creation_order_.end()) { + creation_order_.erase(creation_order_it); + } } // Mark registration is complete; no more singletons can be @@ -298,12 +293,6 @@ class SingletonVault { // singletons once again until reenableInstances() is called. void destroyInstances(); - /* Destroy and clean-up one singleton. Must be invoked while holding - * a read lock on mutex_. - * @param typeDescriptor - the type key for the removed singleton. - */ - void destroyInstance(const detail::TypeDescriptor& typeDescriptor); - // Enable re-creating singletons after destroyInstances() was called. void reenableInstances(); @@ -338,8 +327,7 @@ class SingletonVault { size_t ret = 0; for (const auto& p : singletons_) { - std::lock_guard entry_guard(p.second->mutex); - if (p.second->instance) { + if (p.second->state == SingletonEntryState::Living) { ++ret; } } @@ -358,12 +346,10 @@ class SingletonVault { Quiescing, }; - // Each singleton in the vault can be in three states: dead - // (registered but never created), being born (running the - // CreateFunc), and living (CreateFunc returned an instance). + // Each singleton in the vault can be in two states: dead + // (registered but never created), living (CreateFunc returned an instance). enum class SingletonEntryState { Dead, - BeingBorn, Living, }; @@ -378,24 +364,33 @@ class SingletonVault { // its state as described above, and the create and teardown // functions. struct SingletonEntry { - // mutex protects the entire entry + SingletonEntry(CreateFunc c, TeardownFunc t) : + create(std::move(c)), teardown(std::move(t)) {} + + // mutex protects the entire entry during construction/destruction std::mutex mutex; - // state changes notify state_condvar - SingletonEntryState state = SingletonEntryState::Dead; - std::condition_variable state_condvar; + // State of the singleton entry. If state is Living, instance_ptr and + // instance_weak can be safely accessed w/o synchronization. + std::atomic state{SingletonEntryState::Dead}; - // the thread creating the singleton + // the thread creating the singleton (only valid while creating an object) std::thread::id creating_thread; // The singleton itself and related functions. + + // holds a shared_ptr to singleton instance, set when state is changed from + // Dead to Living. Reset when state is changed from Living to Dead. std::shared_ptr instance; + // weak_ptr to the singleton instance, set when state is changed from Dead + // to Living. We never write to this object after initialization, so it is + // safe to read it from different threads w/o synchronization if we know + // that state is set to Living std::weak_ptr instance_weak; void* instance_ptr = nullptr; CreateFunc create = nullptr; TeardownFunc teardown = nullptr; - SingletonEntry() = default; SingletonEntry(const SingletonEntry&) = delete; SingletonEntry& operator=(const SingletonEntry&) = delete; SingletonEntry& operator=(SingletonEntry&&) = delete; @@ -433,65 +428,70 @@ class SingletonVault { SingletonEntry* get_entry_create(detail::TypeDescriptor type) { auto entry = get_entry(type); - std::unique_lock entry_lock(entry->mutex); - - if (entry->state == SingletonEntryState::BeingBorn) { - // If this thread is trying to give birth to the singleton, it's - // a circular dependency and we must panic. - if (entry->creating_thread == std::this_thread::get_id()) { - throw std::out_of_range(std::string("circular singleton dependency: ") + - type.name()); - } + if (LIKELY(entry->state == SingletonEntryState::Living)) { + return entry; + } - entry->state_condvar.wait(entry_lock, [&entry]() { - return entry->state != SingletonEntryState::BeingBorn; - }); + // There's no synchronization here, so we may not see the current value + // for creating_thread if it was set by other thread, but we only care about + // it if it was set by current thread anyways. + if (entry->creating_thread == std::this_thread::get_id()) { + throw std::out_of_range(std::string("circular singleton dependency: ") + + type.name()); } - if (entry->instance == nullptr) { - RWSpinLock::ReadHolder rh(&stateMutex_); - if (state_ == SingletonVaultState::Quiescing) { - return entry; - } + std::lock_guard entry_lock(entry->mutex); - CHECK(entry->state == SingletonEntryState::Dead); - entry->state = SingletonEntryState::BeingBorn; - entry->creating_thread = std::this_thread::get_id(); + if (entry->state == SingletonEntryState::Living) { + return entry; + } - entry_lock.unlock(); - // Can't use make_shared -- no support for a custom deleter, sadly. - auto instance = std::shared_ptr(entry->create(), entry->teardown); + entry->creating_thread = std::this_thread::get_id(); - // We should schedule destroyInstances() only after the singleton was - // created. This will ensure it will be destroyed before singletons, - // not managed by folly::Singleton, which were initialized in its - // constructor - scheduleDestroyInstances(); + RWSpinLock::ReadHolder rh(&stateMutex_); + if (state_ == SingletonVaultState::Quiescing) { + entry->creating_thread = std::thread::id(); + return entry; + } - entry_lock.lock(); + // Can't use make_shared -- no support for a custom deleter, sadly. + auto instance = std::shared_ptr(entry->create(), entry->teardown); - CHECK(entry->state == SingletonEntryState::BeingBorn); - entry->instance = instance; - entry->instance_weak = instance; - entry->instance_ptr = instance.get(); - entry->state = SingletonEntryState::Living; - entry->state_condvar.notify_all(); + // We should schedule destroyInstances() only after the singleton was + // created. This will ensure it will be destroyed before singletons, + // not managed by folly::Singleton, which were initialized in its + // constructor + scheduleDestroyInstances(); - { - RWSpinLock::WriteHolder wh(&mutex_); + entry->instance = instance; + entry->instance_weak = instance; + entry->instance_ptr = instance.get(); + entry->creating_thread = std::thread::id(); - creation_order_.push_back(type); - } + // This has to be the last step, because once state is Living other threads + // may access instance and instance_weak w/o synchronization. + entry->state.store(SingletonEntryState::Living); + + { + RWSpinLock::WriteHolder wh(&mutex_); + creation_order_.push_back(type); } - CHECK(entry->state == SingletonEntryState::Living); return entry; } - mutable folly::RWSpinLock mutex_; typedef std::unique_ptr SingletonEntryPtr; - std::unordered_map singletons_; + typedef std::unordered_map SingletonMap; + + /* Destroy and clean-up one singleton. Must be invoked while holding + * a read lock on mutex_. + * @param typeDescriptor - the type key for the removed singleton. + */ + void destroyInstance(SingletonMap::iterator entry_it); + + mutable folly::RWSpinLock mutex_; + SingletonMap singletons_; std::vector creation_order_; SingletonVaultState state_{SingletonVaultState::Running}; bool registrationComplete_{false}; diff --git a/folly/experimental/test/SingletonTest.cpp b/folly/experimental/test/SingletonTest.cpp index 38a7598a..3087f538 100644 --- a/folly/experimental/test/SingletonTest.cpp +++ b/folly/experimental/test/SingletonTest.cpp @@ -315,9 +315,9 @@ TEST(Singleton, SingletonDependencies) { // A test to ensure multiple threads contending on singleton creation // properly wait for creation rather than thinking it is a circular // dependency. -class Slowpoke { +class Slowpoke : public Watchdog { public: - Slowpoke() { std::this_thread::sleep_for(std::chrono::seconds(1)); } + Slowpoke() { std::this_thread::sleep_for(std::chrono::milliseconds(10)); } }; TEST(Singleton, SingletonConcurrency) { @@ -348,6 +348,31 @@ TEST(Singleton, SingletonConcurrency) { EXPECT_EQ(vault.livingSingletonCount(), 1); } +TEST(Singleton, SingletonConcurrencyStress) { + SingletonVault vault; + Singleton slowpoke_singleton(nullptr, nullptr, &vault); + + std::vector ts; + for (size_t i = 0; i < 100; ++i) { + ts.emplace_back([&]() { + slowpoke_singleton.get_weak(&vault).lock(); + }); + } + + for (size_t i = 0; i < 100; ++i) { + std::chrono::milliseconds d(20); + + std::this_thread::sleep_for(d); + vault.destroyInstances(); + std::this_thread::sleep_for(d); + vault.destroyInstances(); + } + + for (auto& t : ts) { + t.join(); + } +} + // Benchmarking a normal singleton vs a Meyers singleton vs a Folly // singleton. Meyers are insanely fast, but (hopefully) Folly // singletons are fast "enough."