From 7de5a995c46b4ef706eda82483fe7d25f33a11ea Mon Sep 17 00:00:00 2001 From: Andrii Grynenko Date: Thu, 20 Apr 2017 14:54:37 -0700 Subject: [PATCH] EventBaseLocal cleanup Summary: 1. Restrict EventBaseLocal API to only be used from EventBase thread to avoid extra locking. 2. Make sure objects stored in EventBaseLocal are destroyed in EventBase thread. Reviewed By: yfeldblum Differential Revision: D4918282 fbshipit-source-id: b7cb4c2b62fef85a9b1d796fa71af8af9087479d --- folly/io/async/EventBase.cpp | 8 ++-- folly/io/async/EventBase.h | 1 - folly/io/async/EventBaseLocal.cpp | 53 ++++++---------------- folly/io/async/EventBaseLocal.h | 33 +++++--------- folly/io/async/test/EventBaseLocalTest.cpp | 4 +- 5 files changed, 32 insertions(+), 67 deletions(-) diff --git a/folly/io/async/EventBase.cpp b/folly/io/async/EventBase.cpp index 8d6c03f2..d0d58def 100644 --- a/folly/io/async/EventBase.cpp +++ b/folly/io/async/EventBase.cpp @@ -183,12 +183,10 @@ EventBase::~EventBase() { event_base_free(evb_); } - { - std::lock_guard lock(localStorageMutex_); - for (auto storage : localStorageToDtor_) { - storage->onEventBaseDestruction(*this); - } + for (auto storage : localStorageToDtor_) { + storage->onEventBaseDestruction(*this); } + VLOG(5) << "EventBase(): Destroyed."; } diff --git a/folly/io/async/EventBase.h b/folly/io/async/EventBase.h index 8527e559..fe6dcf1e 100644 --- a/folly/io/async/EventBase.h +++ b/folly/io/async/EventBase.h @@ -743,7 +743,6 @@ class EventBase : private boost::noncopyable, // see EventBaseLocal friend class detail::EventBaseLocalBase; template friend class EventBaseLocal; - std::mutex localStorageMutex_; std::unordered_map> localStorage_; std::unordered_set localStorageToDtor_; diff --git a/folly/io/async/EventBaseLocal.cpp b/folly/io/async/EventBaseLocal.cpp index 8dd55185..999616fe 100644 --- a/folly/io/async/EventBaseLocal.cpp +++ b/folly/io/async/EventBaseLocal.cpp @@ -15,50 +15,30 @@ */ #include +#include #include #include namespace folly { namespace detail { EventBaseLocalBase::~EventBaseLocalBase() { - // There's a race condition if an EventBase and an EventBaseLocal destruct - // at the same time (each will lock eventBases_ and localStorageMutex_ - // in the opposite order), so we dance around it with a loop and try_lock. - while (true) { - SYNCHRONIZED(eventBases_) { - auto it = eventBases_.begin(); - while (it != eventBases_.end()) { - auto evb = *it; - if (evb->localStorageMutex_.try_lock()) { - evb->localStorage_.erase(key_); - evb->localStorageToDtor_.erase(this); - it = eventBases_.erase(it); - evb->localStorageMutex_.unlock(); - } else { - ++it; - } - } - - if (eventBases_.empty()) { - return; - } - } - std::this_thread::yield(); // let the other thread take the eventBases_ lock + for (auto* evb : *eventBases_.rlock()) { + evb->runInEventBaseThread([ this, evb, key = key_ ] { + evb->localStorage_.erase(key); + evb->localStorageToDtor_.erase(this); + }); } } void* EventBaseLocalBase::getVoid(EventBase& evb) { - std::lock_guard lg(evb.localStorageMutex_); - auto it2 = evb.localStorage_.find(key_); - if (UNLIKELY(it2 != evb.localStorage_.end())) { - return it2->second.get(); - } + DCHECK(evb.isInEventBaseThread()); - return nullptr; + return folly::get_default(evb.localStorage_, key_, {}).get(); } void EventBaseLocalBase::erase(EventBase& evb) { - std::lock_guard lg(evb.localStorageMutex_); + DCHECK(evb.isInEventBaseThread()); + evb.localStorage_.erase(key_); evb.localStorageToDtor_.erase(this); @@ -68,18 +48,15 @@ void EventBaseLocalBase::erase(EventBase& evb) { } void EventBaseLocalBase::onEventBaseDestruction(EventBase& evb) { + DCHECK(evb.isInEventBaseThread()); + SYNCHRONIZED(eventBases_) { eventBases_.erase(&evb); } } void EventBaseLocalBase::setVoid(EventBase& evb, std::shared_ptr&& ptr) { - std::lock_guard lg(evb.localStorageMutex_); - setVoidUnlocked(evb, std::move(ptr)); -} - -void EventBaseLocalBase::setVoidUnlocked( - EventBase& evb, std::shared_ptr&& ptr) { + DCHECK(evb.isInEventBaseThread()); auto alreadyExists = evb.localStorage_.find(key_) != evb.localStorage_.end(); @@ -87,9 +64,7 @@ void EventBaseLocalBase::setVoidUnlocked( evb.localStorage_.emplace(key_, std::move(ptr)); if (!alreadyExists) { - SYNCHRONIZED(eventBases_) { - eventBases_.insert(&evb); - } + eventBases_.wlock()->insert(&evb); evb.localStorageToDtor_.insert(this); } } diff --git a/folly/io/async/EventBaseLocal.h b/folly/io/async/EventBaseLocal.h index 41f76a75..f7276375 100644 --- a/folly/io/async/EventBaseLocal.h +++ b/folly/io/async/EventBaseLocal.h @@ -37,7 +37,6 @@ class EventBaseLocalBase : public EventBaseLocalBaseBase, boost::noncopyable { protected: void setVoid(EventBase& evb, std::shared_ptr&& ptr); - void setVoidUnlocked(EventBase& evb, std::shared_ptr&& ptr); void* getVoid(EventBase& evb); folly::Synchronized> eventBases_; @@ -92,17 +91,13 @@ class EventBaseLocal : public detail::EventBaseLocalBase { template T& getOrCreate(EventBase& evb, Args&&... args) { - std::lock_guard lg(evb.localStorageMutex_); - - auto it2 = evb.localStorage_.find(key_); - if (LIKELY(it2 != evb.localStorage_.end())) { - return *static_cast(it2->second.get()); - } else { - auto smartPtr = std::make_shared(std::forward(args)...); - auto ptr = smartPtr.get(); - setVoidUnlocked(evb, std::move(smartPtr)); - return *ptr; + if (auto ptr = getVoid(evb)) { + return *static_cast(ptr); } + auto smartPtr = std::make_shared(std::forward(args)...); + auto& ref = *smartPtr; + setVoid(evb, std::move(smartPtr)); + return ref; } template @@ -110,17 +105,13 @@ class EventBaseLocal : public detail::EventBaseLocalBase { // If this looks like it's copy/pasted from above, that's because it is. // gcc has a bug (fixed in 4.9) that doesn't allow capturing variadic // params in a lambda. - std::lock_guard lg(evb.localStorageMutex_); - - auto it2 = evb.localStorage_.find(key_); - if (LIKELY(it2 != evb.localStorage_.end())) { - return *static_cast(it2->second.get()); - } else { - std::shared_ptr smartPtr(fn()); - auto ptr = smartPtr.get(); - setVoidUnlocked(evb, std::move(smartPtr)); - return *ptr; + if (auto ptr = getVoid(evb)) { + return *static_cast(ptr); } + std::shared_ptr smartPtr(fn()); + auto& ref = *smartPtr; + setVoid(evb, std::move(smartPtr)); + return ref; } }; diff --git a/folly/io/async/test/EventBaseLocalTest.cpp b/folly/io/async/test/EventBaseLocalTest.cpp index f7b963aa..4e74a8ca 100644 --- a/folly/io/async/test/EventBaseLocalTest.cpp +++ b/folly/io/async/test/EventBaseLocalTest.cpp @@ -71,7 +71,9 @@ TEST(EventBaseLocalTest, Basic) { EXPECT_EQ(dtorCnt, 2); // should dtor a Foo when evb2 destructs } - EXPECT_EQ(dtorCnt, 3); // should dtor a Foo when foo destructs + EXPECT_EQ(dtorCnt, 2); // should schedule Foo destructor, when foo destructs + evb1.loop(); + EXPECT_EQ(dtorCnt, 3); // Foo will be destroyed in EventBase loop } TEST(EventBaseLocalTest, getOrCreate) { -- 2.34.1