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

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<std::mutex> 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() {

folly/experimental/Singleton.h

@@ -81,6 +81,7 @@
 #pragma once
 #include <folly/Exception.h>
 #include <folly/Hash.h>
+#include <folly/Memory.h>
 #include <folly/RWSpinLock.h>
 
 #include <algorithm>
@@ -221,18 +222,8 @@ class SingletonVault {
                              TeardownFunc teardown) {
     RWSpinLock::UpgradedHolder wh(&mutex_);
 
-    auto& entry = singletons_[type];
-    if (!entry) {
-      entry.reset(new SingletonEntry);
-    }
-
-    std::lock_guard<std::mutex> 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<SingletonEntry>(std::move(create),
+                                                           std::move(teardown));
   }
 
   /* Register a mock singleton used for testing of singletons which
@@ -244,14 +235,25 @@ class SingletonVault {
     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);
+      std::lock_guard<std::mutex> entry_lg(entry.mutex);
+
+      destroyInstance(entry_it);
+      entry.create = create;
+      entry.teardown = 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.
@@ -265,13 +267,6 @@ class SingletonVault {
     }
   }
 
-    // This method will re-upgrade to write lock for &mutex_.
-    registerSingletonImpl(
-      type,
-      create,
-      teardown);
-  }
-
   // Mark registration is complete; no more singletons can be
   // registered at this point.
   void registrationComplete() {
@@ -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<std::mutex> 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<SingletonEntryState> 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<void> 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<void> 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,32 +428,32 @@ class SingletonVault {
   SingletonEntry* get_entry_create(detail::TypeDescriptor type) {
     auto entry = get_entry(type);
 
-    std::unique_lock<std::mutex> entry_lock(entry->mutex);
+    if (LIKELY(entry->state == SingletonEntryState::Living)) {
+      return entry;
+    }
 
-    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.
+    // 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());
     }
 
-      entry->state_condvar.wait(entry_lock, [&entry]() {
-        return entry->state != SingletonEntryState::BeingBorn;
-      });
+    std::lock_guard<std::mutex> entry_lock(entry->mutex);
+
+    if (entry->state == SingletonEntryState::Living) {
+      return entry;
     }
 
-    if (entry->instance == nullptr) {
+    entry->creating_thread = std::this_thread::get_id();
+
     RWSpinLock::ReadHolder rh(&stateMutex_);
     if (state_ == SingletonVaultState::Quiescing) {
+      entry->creating_thread = std::thread::id();
       return entry;
     }
 
-      CHECK(entry->state == SingletonEntryState::Dead);
-      entry->state = SingletonEntryState::BeingBorn;
-      entry->creating_thread = std::this_thread::get_id();
-
-      entry_lock.unlock();
     // Can't use make_shared -- no support for a custom deleter, sadly.
     auto instance = std::shared_ptr<void>(entry->create(), entry->teardown);
 
@@ -468,30 +463,35 @@ class SingletonVault {
     // constructor
     scheduleDestroyInstances();
 
-      entry_lock.lock();
-
-      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();
+    entry->creating_thread = std::thread::id();
+
+    // 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<SingletonEntry> SingletonEntryPtr;
-  std::unordered_map<detail::TypeDescriptor,
+  typedef std::unordered_map<detail::TypeDescriptor,
                              SingletonEntryPtr,
-                     detail::TypeDescriptorHasher> singletons_;
+                             detail::TypeDescriptorHasher> 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<detail::TypeDescriptor> creation_order_;
   SingletonVaultState state_{SingletonVaultState::Running};
   bool registrationComplete_{false};

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> slowpoke_singleton(nullptr, nullptr, &vault);
+
+  std::vector<std::thread> 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."