Commit 7de5a995 authored by Andrii Grynenko's avatar Andrii Grynenko Committed by Facebook Github Bot

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
parent 049e3d99
...@@ -183,12 +183,10 @@ EventBase::~EventBase() { ...@@ -183,12 +183,10 @@ EventBase::~EventBase() {
event_base_free(evb_); event_base_free(evb_);
} }
{ for (auto storage : localStorageToDtor_) {
std::lock_guard<std::mutex> lock(localStorageMutex_); storage->onEventBaseDestruction(*this);
for (auto storage : localStorageToDtor_) {
storage->onEventBaseDestruction(*this);
}
} }
VLOG(5) << "EventBase(): Destroyed."; VLOG(5) << "EventBase(): Destroyed.";
} }
......
...@@ -743,7 +743,6 @@ class EventBase : private boost::noncopyable, ...@@ -743,7 +743,6 @@ class EventBase : private boost::noncopyable,
// see EventBaseLocal // see EventBaseLocal
friend class detail::EventBaseLocalBase; friend class detail::EventBaseLocalBase;
template <typename T> friend class EventBaseLocal; template <typename T> friend class EventBaseLocal;
std::mutex localStorageMutex_;
std::unordered_map<uint64_t, std::shared_ptr<void>> localStorage_; std::unordered_map<uint64_t, std::shared_ptr<void>> localStorage_;
std::unordered_set<detail::EventBaseLocalBaseBase*> localStorageToDtor_; std::unordered_set<detail::EventBaseLocalBaseBase*> localStorageToDtor_;
......
...@@ -15,50 +15,30 @@ ...@@ -15,50 +15,30 @@
*/ */
#include <folly/io/async/EventBaseLocal.h> #include <folly/io/async/EventBaseLocal.h>
#include <folly/MapUtil.h>
#include <atomic> #include <atomic>
#include <thread> #include <thread>
namespace folly { namespace detail { namespace folly { namespace detail {
EventBaseLocalBase::~EventBaseLocalBase() { EventBaseLocalBase::~EventBaseLocalBase() {
// There's a race condition if an EventBase and an EventBaseLocal destruct for (auto* evb : *eventBases_.rlock()) {
// at the same time (each will lock eventBases_ and localStorageMutex_ evb->runInEventBaseThread([ this, evb, key = key_ ] {
// in the opposite order), so we dance around it with a loop and try_lock. evb->localStorage_.erase(key);
while (true) { evb->localStorageToDtor_.erase(this);
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
} }
} }
void* EventBaseLocalBase::getVoid(EventBase& evb) { void* EventBaseLocalBase::getVoid(EventBase& evb) {
std::lock_guard<std::mutex> lg(evb.localStorageMutex_); DCHECK(evb.isInEventBaseThread());
auto it2 = evb.localStorage_.find(key_);
if (UNLIKELY(it2 != evb.localStorage_.end())) {
return it2->second.get();
}
return nullptr; return folly::get_default(evb.localStorage_, key_, {}).get();
} }
void EventBaseLocalBase::erase(EventBase& evb) { void EventBaseLocalBase::erase(EventBase& evb) {
std::lock_guard<std::mutex> lg(evb.localStorageMutex_); DCHECK(evb.isInEventBaseThread());
evb.localStorage_.erase(key_); evb.localStorage_.erase(key_);
evb.localStorageToDtor_.erase(this); evb.localStorageToDtor_.erase(this);
...@@ -68,18 +48,15 @@ void EventBaseLocalBase::erase(EventBase& evb) { ...@@ -68,18 +48,15 @@ void EventBaseLocalBase::erase(EventBase& evb) {
} }
void EventBaseLocalBase::onEventBaseDestruction(EventBase& evb) { void EventBaseLocalBase::onEventBaseDestruction(EventBase& evb) {
DCHECK(evb.isInEventBaseThread());
SYNCHRONIZED(eventBases_) { SYNCHRONIZED(eventBases_) {
eventBases_.erase(&evb); eventBases_.erase(&evb);
} }
} }
void EventBaseLocalBase::setVoid(EventBase& evb, std::shared_ptr<void>&& ptr) { void EventBaseLocalBase::setVoid(EventBase& evb, std::shared_ptr<void>&& ptr) {
std::lock_guard<std::mutex> lg(evb.localStorageMutex_); DCHECK(evb.isInEventBaseThread());
setVoidUnlocked(evb, std::move(ptr));
}
void EventBaseLocalBase::setVoidUnlocked(
EventBase& evb, std::shared_ptr<void>&& ptr) {
auto alreadyExists = auto alreadyExists =
evb.localStorage_.find(key_) != evb.localStorage_.end(); evb.localStorage_.find(key_) != evb.localStorage_.end();
...@@ -87,9 +64,7 @@ void EventBaseLocalBase::setVoidUnlocked( ...@@ -87,9 +64,7 @@ void EventBaseLocalBase::setVoidUnlocked(
evb.localStorage_.emplace(key_, std::move(ptr)); evb.localStorage_.emplace(key_, std::move(ptr));
if (!alreadyExists) { if (!alreadyExists) {
SYNCHRONIZED(eventBases_) { eventBases_.wlock()->insert(&evb);
eventBases_.insert(&evb);
}
evb.localStorageToDtor_.insert(this); evb.localStorageToDtor_.insert(this);
} }
} }
......
...@@ -37,7 +37,6 @@ class EventBaseLocalBase : public EventBaseLocalBaseBase, boost::noncopyable { ...@@ -37,7 +37,6 @@ class EventBaseLocalBase : public EventBaseLocalBaseBase, boost::noncopyable {
protected: protected:
void setVoid(EventBase& evb, std::shared_ptr<void>&& ptr); void setVoid(EventBase& evb, std::shared_ptr<void>&& ptr);
void setVoidUnlocked(EventBase& evb, std::shared_ptr<void>&& ptr);
void* getVoid(EventBase& evb); void* getVoid(EventBase& evb);
folly::Synchronized<std::unordered_set<EventBase*>> eventBases_; folly::Synchronized<std::unordered_set<EventBase*>> eventBases_;
...@@ -92,17 +91,13 @@ class EventBaseLocal : public detail::EventBaseLocalBase { ...@@ -92,17 +91,13 @@ class EventBaseLocal : public detail::EventBaseLocalBase {
template <typename... Args> template <typename... Args>
T& getOrCreate(EventBase& evb, Args&&... args) { T& getOrCreate(EventBase& evb, Args&&... args) {
std::lock_guard<std::mutex> lg(evb.localStorageMutex_); if (auto ptr = getVoid(evb)) {
return *static_cast<T*>(ptr);
auto it2 = evb.localStorage_.find(key_);
if (LIKELY(it2 != evb.localStorage_.end())) {
return *static_cast<T*>(it2->second.get());
} else {
auto smartPtr = std::make_shared<T>(std::forward<Args>(args)...);
auto ptr = smartPtr.get();
setVoidUnlocked(evb, std::move(smartPtr));
return *ptr;
} }
auto smartPtr = std::make_shared<T>(std::forward<Args>(args)...);
auto& ref = *smartPtr;
setVoid(evb, std::move(smartPtr));
return ref;
} }
template <typename Func> template <typename Func>
...@@ -110,17 +105,13 @@ class EventBaseLocal : public detail::EventBaseLocalBase { ...@@ -110,17 +105,13 @@ class EventBaseLocal : public detail::EventBaseLocalBase {
// If this looks like it's copy/pasted from above, that's because it is. // 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 // gcc has a bug (fixed in 4.9) that doesn't allow capturing variadic
// params in a lambda. // params in a lambda.
std::lock_guard<std::mutex> lg(evb.localStorageMutex_); if (auto ptr = getVoid(evb)) {
return *static_cast<T*>(ptr);
auto it2 = evb.localStorage_.find(key_);
if (LIKELY(it2 != evb.localStorage_.end())) {
return *static_cast<T*>(it2->second.get());
} else {
std::shared_ptr<T> smartPtr(fn());
auto ptr = smartPtr.get();
setVoidUnlocked(evb, std::move(smartPtr));
return *ptr;
} }
std::shared_ptr<T> smartPtr(fn());
auto& ref = *smartPtr;
setVoid(evb, std::move(smartPtr));
return ref;
} }
}; };
......
...@@ -71,7 +71,9 @@ TEST(EventBaseLocalTest, Basic) { ...@@ -71,7 +71,9 @@ TEST(EventBaseLocalTest, Basic) {
EXPECT_EQ(dtorCnt, 2); // should dtor a Foo when evb2 destructs 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) { TEST(EventBaseLocalTest, getOrCreate) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment