From 4833e0ef95b46a9b6afd9bd712dc0aca63d9077e Mon Sep 17 00:00:00 2001 From: Andrii Grynenko Date: Thu, 25 Sep 2014 19:02:37 -0700 Subject: [PATCH] Use RWSpinLock for Singleton mutex Summary: We only need exclusive lock when we add items to singletons_. Each SingletonEntry has its own mutex, so it's safe to rely on it for any modifications within individual entries. Test Plan: Applied D1573880 and ran fbconfig -r servicerouter/client/cpp2 && fbmake runtests Reviewed By: chip@fb.com Subscribers: trunkagent, njormrod, hitesh, mshneer FB internal diff: D1579877 --- folly/experimental/Singleton.cpp | 46 +++++++++++++---------- folly/experimental/Singleton.h | 64 +++++++++++++++++--------------- 2 files changed, 60 insertions(+), 50 deletions(-) diff --git a/folly/experimental/Singleton.cpp b/folly/experimental/Singleton.cpp index 7ad7ec0a..7934ab1a 100644 --- a/folly/experimental/Singleton.cpp +++ b/folly/experimental/Singleton.cpp @@ -23,29 +23,35 @@ namespace folly { SingletonVault::~SingletonVault() { destroyInstances(); } void SingletonVault::destroyInstances() { - std::lock_guard guard(mutex_); - CHECK_GE(singletons_.size(), creation_order_.size()); - - for (auto type_iter = creation_order_.rbegin(); - type_iter != creation_order_.rend(); - ++type_iter) { - auto type = *type_iter; - auto it = singletons_.find(type); - 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 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(); + { + RWSpinLock::ReadHolder rh(&mutex_); + + CHECK_GE(singletons_.size(), creation_order_.size()); + + for (auto type_iter = creation_order_.rbegin(); + type_iter != creation_order_.rend(); + ++type_iter) { + auto type = *type_iter; + auto it = singletons_.find(type); + 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 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(); + } + entry->instance.reset(); + entry->state = SingletonEntryState::Dead; + entry->state_condvar.notify_all(); } - entry->instance.reset(); - entry->state = SingletonEntryState::Dead; - entry->state_condvar.notify_all(); } - creation_order_.clear(); + { + RWSpinLock::WriteHolder wh(&mutex_); + creation_order_.clear(); + } } SingletonVault* SingletonVault::singleton() { diff --git a/folly/experimental/Singleton.h b/folly/experimental/Singleton.h index 73625a99..6c6a2eff 100644 --- a/folly/experimental/Singleton.h +++ b/folly/experimental/Singleton.h @@ -78,6 +78,7 @@ #pragma once #include #include +#include #include #include @@ -176,14 +177,12 @@ class SingletonVault { void registerSingleton(detail::TypeDescriptor type, CreateFunc create, TeardownFunc teardown) { - std::lock_guard guard(mutex_); + RWSpinLock::WriteHolder wh(&mutex_); stateCheck(SingletonVaultState::Registering); CHECK_THROW(singletons_.find(type) == singletons_.end(), std::logic_error); auto& entry = singletons_[type]; - if (!entry) { - entry.reset(new SingletonEntry); - } + entry.reset(new SingletonEntry); std::lock_guard entry_guard(entry->mutex); CHECK(entry->instance == nullptr); @@ -197,7 +196,8 @@ class SingletonVault { // Mark registration is complete; no more singletons can be // registered at this point. void registrationComplete() { - std::lock_guard guard(mutex_); + RWSpinLock::WriteHolder wh(&mutex_); + stateCheck(SingletonVaultState::Registering); state_ = SingletonVaultState::Running; } @@ -208,8 +208,7 @@ class SingletonVault { // Retrieve a singleton from the vault, creating it if necessary. std::shared_ptr get_shared(detail::TypeDescriptor type) { - std::unique_lock lock(mutex_); - auto entry = get_entry(type, &lock); + auto entry = get_entry_create(type); return entry->instance; } @@ -218,21 +217,23 @@ class SingletonVault { // responsibility to be sane with this, but it is preferable to use // the weak_ptr interface for true safety. void* get_ptr(detail::TypeDescriptor type) { - std::unique_lock lock(mutex_); - auto entry = get_entry(type, &lock); + auto entry = get_entry_create(type); return entry->instance_ptr; } // For testing; how many registered and living singletons we have. size_t registeredSingletonCount() const { - std::lock_guard guard(mutex_); + RWSpinLock::ReadHolder rh(&mutex_); + return singletons_.size(); } size_t livingSingletonCount() const { - std::lock_guard guard(mutex_); + RWSpinLock::ReadHolder rh(&mutex_); + size_t ret = 0; for (const auto& p : singletons_) { + std::lock_guard entry_guard(p.second->mutex); if (p.second->instance) { ++ret; } @@ -295,17 +296,15 @@ class SingletonVault { SingletonEntry(SingletonEntry&&) = delete; }; - // Get a pointer to the living SingletonEntry for the specified - // type. The singleton is created as part of this function, if - // necessary. - SingletonEntry* get_entry(detail::TypeDescriptor type, - std::unique_lock* lock) { + SingletonEntry* get_entry(detail::TypeDescriptor type) { + RWSpinLock::ReadHolder rh(&mutex_); + // mutex must be held when calling this function stateCheck( - SingletonVaultState::Running, - "Attempt to load a singleton before " - "SingletonVault::registrationComplete was called (hint: you probably " - "didn't call initFacebook)"); + SingletonVaultState::Running, + "Attempt to load a singleton before " + "SingletonVault::registrationComplete was called (hint: you probably " + "didn't call initFacebook)"); auto it = singletons_.find(type); if (it == singletons_.end()) { @@ -313,7 +312,15 @@ class SingletonVault { type.name()); } - auto entry = it->second.get(); + return it->second.get(); + } + + // Get a pointer to the living SingletonEntry for the specified + // type. The singleton is created as part of this function, if + // necessary. + SingletonEntry* get_entry_create(detail::TypeDescriptor type) { + auto entry = get_entry(type); + std::unique_lock entry_lock(entry->mutex); if (entry->state == SingletonEntryState::BeingBorn) { @@ -324,14 +331,9 @@ class SingletonVault { type.name()); } - // Otherwise, another thread is constructing the singleton; - // let's wait on a condvar to see it complete. We release and - // reaquire lock while waiting on the entry to resolve its state. - lock->unlock(); entry->state_condvar.wait(entry_lock, [&entry]() { return entry->state != SingletonEntryState::BeingBorn; }); - lock->lock(); } if (entry->instance == nullptr) { @@ -340,10 +342,8 @@ class SingletonVault { entry->creating_thread = std::this_thread::get_id(); entry_lock.unlock(); - lock->unlock(); // Can't use make_shared -- no support for a custom deleter, sadly. auto instance = std::shared_ptr(entry->create(), entry->teardown); - lock->lock(); entry_lock.lock(); CHECK(entry->state == SingletonEntryState::BeingBorn); @@ -352,13 +352,17 @@ class SingletonVault { entry->state = SingletonEntryState::Living; entry->state_condvar.notify_all(); - creation_order_.push_back(type); + { + RWSpinLock::WriteHolder wh(&mutex_); + + creation_order_.push_back(type); + } } CHECK(entry->state == SingletonEntryState::Living); return entry; } - mutable std::mutex mutex_; + mutable folly::RWSpinLock mutex_; typedef std::unique_ptr SingletonEntryPtr; std::unordered_map