From 9fef1cd9f2684c7c86ec9f051a0042ab48c690fb Mon Sep 17 00:00:00 2001 From: Yedidya Feldblum Date: Sat, 23 Dec 2017 00:09:39 -0800 Subject: [PATCH] Move expensive Singleton code to .cpp Summary: [Folly] Move expensive `Singleton` code to `.cpp`. Including string manipulation, `ostream::operator<<` operations, `throw` statements, etc, Reviewed By: ot Differential Revision: D6632052 fbshipit-source-id: 82e0033d5042b02951cf3b05c55cf62c97cc2b6d --- folly/Singleton-inl.h | 54 +++-------------- folly/Singleton.cpp | 134 +++++++++++++++++++++++++++++++++++++++--- folly/Singleton.h | 116 ++++++++++++++++++++---------------- 3 files changed, 198 insertions(+), 106 deletions(-) diff --git a/folly/Singleton-inl.h b/folly/Singleton-inl.h index 762a9ec5..12612acc 100644 --- a/folly/Singleton-inl.h +++ b/folly/Singleton-inl.h @@ -66,8 +66,7 @@ void SingletonHolder::registerSingleton(CreateFunc c, TeardownFunc t) { template void SingletonHolder::registerSingletonMock(CreateFunc c, TeardownFunc t) { if (state_ == SingletonHolderState::NotRegistered) { - LOG(FATAL) << "Registering mock before singleton was registered: " - << type().name(); + detail::singletonWarnRegisterMockEarlyAndAbort(type()); } if (state_ == SingletonHolderState::Living) { destroyInstance(); @@ -97,10 +96,7 @@ T* SingletonHolder::get() { createInstance(); if (instance_weak_.expired()) { - throw std::runtime_error( - "Raw pointer to a singleton requested after its destruction." - " Singleton type is: " + - type().name()); + detail::singletonThrowGetInvokedAfterDestruction(type()); } return instance_ptr_; @@ -161,14 +157,7 @@ void SingletonHolder::destroyInstance() { teardown_(instance_ptr_); } else { print_destructor_stack_trace_->store(true); - LOG(ERROR) << "Singleton of type " << type().name() << " has a " - << "living reference at destroyInstances time; beware! Raw " - << "pointer is " << instance_ptr_ << ". It is very likely " - << "that some other singleton is holding a shared_ptr to it. " - << "This singleton will be leaked (even if a shared_ptr to it " - << "is eventually released)." - << "Make sure dependencies between these singletons are " - << "properly defined."; + detail::singletonWarnDestroyInstanceLeak(type(), instance_ptr_); } } } @@ -199,7 +188,7 @@ template void SingletonHolder::createInstance() { if (creating_thread_.load(std::memory_order_acquire) == std::this_thread::get_id()) { - LOG(FATAL) << "circular singleton dependency: " << type().name(); + detail::singletonWarnCreateCircularDependencyAndAbort(type()); } std::lock_guard entry_lock(mutex_); @@ -208,12 +197,7 @@ void SingletonHolder::createInstance() { } if (state_.load(std::memory_order_acquire) == SingletonHolderState::NotRegistered) { - auto ptr = SingletonVault::stackTraceGetter().load(); - LOG(FATAL) << "Creating instance for unregistered singleton: " - << type().name() << "\n" - << "Stacktrace:" - << "\n" - << (ptr ? (*ptr)() : "(not available)"); + detail::singletonWarnCreateUnregisteredAndAbort(type()); } if (state_.load(std::memory_order_acquire) == SingletonHolderState::Living) { @@ -232,20 +216,9 @@ void SingletonHolder::createInstance() { auto state = vault_.state_.rlock(); if (vault_.type_ != SingletonVault::Type::Relaxed && !state->registrationComplete) { - auto stack_trace_getter = SingletonVault::stackTraceGetter().load(); - auto stack_trace = stack_trace_getter ? stack_trace_getter() : ""; - if (!stack_trace.empty()) { - stack_trace = "Stack trace:\n" + stack_trace; - } - - LOG(FATAL) << "Singleton " << type().name() << " requested before " - << "registrationComplete() call.\n" - << "This usually means that either main() never called " - << "folly::init, or singleton was requested before main() " - << "(which is not allowed).\n" - << stack_trace; + detail::singletonWarnCreateBeforeRegistrationCompleteAndAbort(type()); } - if (state->state == SingletonVault::SingletonVaultState::Quiescing) { + if (state->state == detail::SingletonVaultState::Type::Quiescing) { return; } @@ -260,18 +233,7 @@ void SingletonHolder::createInstance() { T*) mutable { destroy_baton->post(); if (print_destructor_stack_trace->load()) { - std::string output = "Singleton " + type.name() + " was released.\n"; - - auto stack_trace_getter = SingletonVault::stackTraceGetter().load(); - auto stack_trace = stack_trace_getter ? stack_trace_getter() : ""; - if (stack_trace.empty()) { - output += "Failed to get release stack trace."; - } else { - output += "Release stack trace:\n"; - output += stack_trace; - } - - LOG(ERROR) << output; + detail::singletonPrintDestructionStackTrace(type); } }); diff --git a/folly/Singleton.cpp b/folly/Singleton.cpp index 81c103a6..f02cd3f6 100644 --- a/folly/Singleton.cpp +++ b/folly/Singleton.cpp @@ -26,6 +26,8 @@ #include #include +#include +#include #include #if !defined(_WIN32) && !defined(__APPLE__) && !defined(__ANDROID__) @@ -57,6 +59,15 @@ SingletonVault::Type SingletonVault::defaultVaultType() { namespace detail { +std::string TypeDescriptor::name() const { + auto ret = demangle(ti_.name()); + if (tag_ti_ != std::type_index(typeid(DefaultTag))) { + ret += "/"; + ret += demangle(tag_ti_.name()); + } + return ret.toStdString(); +} + [[noreturn]] void singletonWarnDoubleRegistrationAndAbort( const TypeDescriptor& type) { // Ensure the availability of std::cerr @@ -67,6 +78,111 @@ namespace detail { << type.name() << ">\n"; std::abort(); } + +[[noreturn]] void singletonWarnLeakyDoubleRegistrationAndAbort( + const TypeDescriptor& type) { + // Ensure the availability of std::cerr + std::ios_base::Init ioInit; + std::cerr << "Double registration of singletons of the same " + "underlying type; check for multiple definitions " + "of type folly::LeakySingleton<" + << type.name() << ">\n"; + std::abort(); +} + +[[noreturn]] void singletonWarnLeakyInstantiatingNotRegisteredAndAbort( + const TypeDescriptor& type) { + auto ptr = SingletonVault::stackTraceGetter().load(); + LOG(FATAL) << "Creating instance for unregistered singleton: " + << type.name() << "\n" + << "Stacktrace:" + << "\n" << (ptr ? (*ptr)() : "(not available)"); +} + +[[noreturn]] void singletonWarnRegisterMockEarlyAndAbort( + const TypeDescriptor& type) { + LOG(FATAL) << "Registering mock before singleton was registered: " + << type.name(); +} + +void singletonWarnDestroyInstanceLeak( + const TypeDescriptor& type, + const void* ptr) { + LOG(ERROR) << "Singleton of type " << type.name() << " has a " + << "living reference at destroyInstances time; beware! Raw " + << "pointer is " << ptr << ". It is very likely " + << "that some other singleton is holding a shared_ptr to it. " + << "This singleton will be leaked (even if a shared_ptr to it " + << "is eventually released)." + << "Make sure dependencies between these singletons are " + << "properly defined."; +} + +[[noreturn]] void singletonWarnCreateCircularDependencyAndAbort( + const TypeDescriptor& type) { + LOG(FATAL) << "circular singleton dependency: " << type.name(); +} + +[[noreturn]] void singletonWarnCreateUnregisteredAndAbort( + const TypeDescriptor& type) { + auto ptr = SingletonVault::stackTraceGetter().load(); + LOG(FATAL) << "Creating instance for unregistered singleton: " + << type.name() << "\n" + << "Stacktrace:" + << "\n" + << (ptr ? (*ptr)() : "(not available)"); +} + +[[noreturn]] void singletonWarnCreateBeforeRegistrationCompleteAndAbort( + const TypeDescriptor& type) { + auto stack_trace_getter = SingletonVault::stackTraceGetter().load(); + auto stack_trace = stack_trace_getter ? stack_trace_getter() : ""; + if (!stack_trace.empty()) { + stack_trace = "Stack trace:\n" + stack_trace; + } + + LOG(FATAL) << "Singleton " << type.name() << " requested before " + << "registrationComplete() call.\n" + << "This usually means that either main() never called " + << "folly::init, or singleton was requested before main() " + << "(which is not allowed).\n" + << stack_trace; +} + +void singletonPrintDestructionStackTrace(const TypeDescriptor& type) { + std::string output = "Singleton " + type.name() + " was released.\n"; + + auto stack_trace_getter = SingletonVault::stackTraceGetter().load(); + auto stack_trace = stack_trace_getter ? stack_trace_getter() : ""; + if (stack_trace.empty()) { + output += "Failed to get release stack trace."; + } else { + output += "Release stack trace:\n"; + output += stack_trace; + } + + LOG(ERROR) << output; +} + +[[noreturn]] void singletonThrowNullCreator(const std::type_info& type) { + auto const msg = sformat( + "nullptr_t should be passed if you want {} to be default constructed", + demangle(type)); + throw std::logic_error(msg); +} + +[[noreturn]] void singletonThrowGetInvokedAfterDestruction( + const TypeDescriptor& type) { + throw std::runtime_error( + "Raw pointer to a singleton requested after its destruction." + " Singleton type is: " + + type.name()); +} + +[[noreturn]] void SingletonVaultState::throwUnexpectedState(const char* msg) { + throw std::logic_error(msg); +} + } // namespace detail namespace { @@ -101,7 +217,7 @@ SingletonVault::~SingletonVault() { destroyInstances(); } void SingletonVault::registerSingleton(detail::SingletonHolderBase* entry) { auto state = state_.rlock(); - stateCheck(SingletonVaultState::Running, *state); + state->check(detail::SingletonVaultState::Type::Running); if (UNLIKELY(state->registrationComplete)) { LOG(ERROR) << "Registering singleton after registrationComplete()."; @@ -114,7 +230,7 @@ void SingletonVault::registerSingleton(detail::SingletonHolderBase* entry) { void SingletonVault::addEagerInitSingleton(detail::SingletonHolderBase* entry) { auto state = state_.rlock(); - stateCheck(SingletonVaultState::Running, *state); + state->check(detail::SingletonVaultState::Type::Running); if (UNLIKELY(state->registrationComplete)) { LOG(ERROR) << "Registering for eager-load after registrationComplete()."; @@ -130,7 +246,7 @@ void SingletonVault::registrationComplete() { std::atexit([](){ SingletonVault::singleton()->destroyInstances(); }); auto state = state_.wlock(); - stateCheck(SingletonVaultState::Running, *state); + state->check(detail::SingletonVaultState::Type::Running); if (state->registrationComplete) { return; @@ -153,7 +269,7 @@ void SingletonVault::registrationComplete() { void SingletonVault::doEagerInit() { { auto state = state_.rlock(); - stateCheck(SingletonVaultState::Running, *state); + state->check(detail::SingletonVaultState::Type::Running); if (UNLIKELY(!state->registrationComplete)) { throw std::logic_error("registrationComplete() not yet called"); } @@ -168,7 +284,7 @@ void SingletonVault::doEagerInit() { void SingletonVault::doEagerInitVia(Executor& exe, folly::Baton<>* done) { { auto state = state_.rlock(); - stateCheck(SingletonVaultState::Running, *state); + state->check(detail::SingletonVaultState::Type::Running); if (UNLIKELY(!state->registrationComplete)) { throw std::logic_error("registrationComplete() not yet called"); } @@ -204,10 +320,10 @@ void SingletonVault::doEagerInitVia(Executor& exe, folly::Baton<>* done) { void SingletonVault::destroyInstances() { auto stateW = state_.wlock(); - if (stateW->state == SingletonVaultState::Quiescing) { + if (stateW->state == detail::SingletonVaultState::Type::Quiescing) { return; } - stateW->state = SingletonVaultState::Quiescing; + stateW->state = detail::SingletonVaultState::Type::Quiescing; auto stateR = stateW.moveFromWriteToRead(); { @@ -249,9 +365,9 @@ void SingletonVault::destroyInstances() { void SingletonVault::reenableInstances() { auto state = state_.wlock(); - stateCheck(SingletonVaultState::Quiescing, *state); + state->check(detail::SingletonVaultState::Type::Quiescing); - state->state = SingletonVaultState::Running; + state->state = detail::SingletonVaultState::Type::Running; } void SingletonVault::scheduleDestroyInstances() { diff --git a/folly/Singleton.h b/folly/Singleton.h index 921c443a..a08954fc 100644 --- a/folly/Singleton.h +++ b/folly/Singleton.h @@ -121,7 +121,7 @@ // should call reenableInstances. #pragma once -#include + #include #include #include @@ -206,14 +206,7 @@ class TypeDescriptor { return *this; } - std::string name() const { - auto ret = demangle(ti_.name()); - if (tag_ti_ != std::type_index(typeid(DefaultTag))) { - ret += "/"; - ret += demangle(tag_ti_.name()); - } - return ret.toStdString(); - } + std::string name() const; friend class TypeDescriptorHasher; @@ -233,6 +226,59 @@ class TypeDescriptorHasher { } }; +[[noreturn]] void singletonWarnLeakyDoubleRegistrationAndAbort( + const TypeDescriptor& type); + +[[noreturn]] void singletonWarnLeakyInstantiatingNotRegisteredAndAbort( + const TypeDescriptor& type); + +[[noreturn]] void singletonWarnRegisterMockEarlyAndAbort( + const TypeDescriptor& type); + +void singletonWarnDestroyInstanceLeak( + const TypeDescriptor& type, + const void* ptr); + +[[noreturn]] void singletonWarnCreateCircularDependencyAndAbort( + const TypeDescriptor& type); + +[[noreturn]] void singletonWarnCreateUnregisteredAndAbort( + const TypeDescriptor& type); + +[[noreturn]] void singletonWarnCreateBeforeRegistrationCompleteAndAbort( + const TypeDescriptor& type); + +void singletonPrintDestructionStackTrace(const TypeDescriptor& type); + +[[noreturn]] void singletonThrowNullCreator(const std::type_info& type); + +[[noreturn]] void singletonThrowGetInvokedAfterDestruction( + const TypeDescriptor& type); + +struct SingletonVaultState { + // The two stages of life for a vault, as mentioned in the class comment. + enum class Type { + Running, + Quiescing, + }; + + Type state{Type::Running}; + bool registrationComplete{false}; + + // Each singleton in the vault can be in two states: dead + // (registered but never created), living (CreateFunc returned an instance). + + void check( + Type expected, + const char* msg = "Unexpected singleton state change") const { + if (expected != state) { + throwUnexpectedState(msg); + } + } + + [[noreturn]] static void throwUnexpectedState(const char* msg); +}; + // This interface is used by SingletonVault to interact with SingletonHolders. // Having a non-template interface allows SingletonVault to keep a list of all // SingletonHolders. @@ -475,29 +521,6 @@ class SingletonVault { template friend struct detail::SingletonHolder; - // The two stages of life for a vault, as mentioned in the class comment. - enum class SingletonVaultState { - Running, - Quiescing, - }; - - struct State { - SingletonVaultState state{SingletonVaultState::Running}; - bool registrationComplete{false}; - }; - - // Each singleton in the vault can be in two states: dead - // (registered but never created), living (CreateFunc returned an instance). - - static void stateCheck( - SingletonVaultState expected, - const State& state, - const char* msg = "Unexpected singleton state change") { - if (expected != state.state) { - throw std::logic_error(msg); - } - } - // This method only matters if registrationComplete() is never called. // Otherwise destroyInstances is scheduled to be executed atexit. // @@ -514,15 +537,15 @@ class SingletonVault { typedef std::unordered_map SingletonMap; - folly::Synchronized singletons_; - folly::Synchronized> + Synchronized singletons_; + Synchronized> eagerInitSingletons_; - folly::Synchronized> creationOrder_; + Synchronized> creationOrder_; // Using SharedMutexReadPriority is important here, because we want to make // sure we don't block nested singleton creation happening concurrently with // destroyInstances(). - folly::Synchronized state_; + Synchronized state_; Type type_; }; @@ -576,8 +599,7 @@ class Singleton { explicit Singleton(typename Singleton::CreateFunc c, typename Singleton::TeardownFunc t = nullptr) { if (c == nullptr) { - throw std::logic_error( - "nullptr_t should be passed if you want T to be default constructed"); + detail::singletonThrowNullCreator(typeid(T)); } auto vault = SingletonVault::singleton(); @@ -625,8 +647,7 @@ class Singleton { static void make_mock(CreateFunc c, typename Singleton::TeardownFunc t = nullptr) { if (c == nullptr) { - throw std::logic_error( - "nullptr_t should be passed if you want T to be default constructed"); + detail::singletonThrowNullCreator(typeid(T)); } auto& entry = getEntry(); @@ -660,9 +681,7 @@ class LeakySingleton { explicit LeakySingleton(CreateFunc createFunc) { auto& entry = entryInstance(); if (entry.state != State::NotRegistered) { - LOG(FATAL) << "Double registration of singletons of the same " - << "underlying type; check for multiple definitions " - << "of type folly::LeakySingleton<" + entry.type_.name() + ">"; + detail::singletonWarnLeakyDoubleRegistrationAndAbort(entry.type_); } entry.createFunc = createFunc; entry.state = State::Dead; @@ -675,12 +694,11 @@ class LeakySingleton { } static void make_mock(CreateFunc createFunc) { - auto& entry = entryInstance(); if (createFunc == nullptr) { - throw std::logic_error( - "nullptr_t should be passed if you want T to be default constructed"); + detail::singletonThrowNullCreator(typeid(T)); } + auto& entry = entryInstance(); entry.createFunc = createFunc; entry.state = State::Dead; } @@ -723,11 +741,7 @@ class LeakySingleton { } if (entry.state == State::NotRegistered) { - auto ptr = SingletonVault::stackTraceGetter().load(); - LOG(FATAL) << "Creating instance for unregistered singleton: " - << entry.type_.name() << "\n" - << "Stacktrace:" - << "\n" << (ptr ? (*ptr)() : "(not available)"); + detail::singletonWarnLeakyInstantiatingNotRegisteredAndAbort(entry.type_); } entry.ptr = entry.createFunc(); -- 2.34.1