From: Teng Qin Date: Tue, 31 Oct 2017 20:49:15 +0000 (-0700) Subject: Improve folly::RequestContext onSet and onUnset efficiency X-Git-Tag: v2017.11.06.00~25 X-Git-Url: http://plrg.eecs.uci.edu/git/?p=folly.git;a=commitdiff_plain;h=b82902de6f5002cb46b4c0ae88f5cc7ae11b2585 Improve folly::RequestContext onSet and onUnset efficiency Summary: In previous discussions, it has been pointed out that `folly::RequestContext::setContext` consumes considerable amount of CPU cycles in production environments. After some investigation, we thought that might be caused by looping over all `RequestData` instances can calling the virtual `onSet` and `onUnset` callback of them. Both the iteration and invoking virtual methods are not cheap. As you can see from this change, most of the derived classes of `RequestData` don't override the `onSet` and `onUnset` methods. Mostly likely they are only used for per-Request tracking. So the natural idea is to skip those instances when iterating and avoid the unnecessary virtual method invoke. I have explored the solution to dynamically examine if the `RequestData` instance added has `onSet` and `onUnset` method overridden. That is possible with GCC's PMF extension, but not [[ http://lists.llvm.org/pipermail/llvm-bugs/2015-July/041164.html | for Clang ]] and probably many other compilers. This definitely won't be very good for `folly`'s probability, even if we gate it by compiler flags. Therefore, this Diff adds the `hasCallback` method to `RequestData` class indicating whether the instance would have `onSet` and `onUnset` overridden. To make it clear to users that they need to correctly override it in order for their `onSet` and `onUnset` callback to work, making it abstract so that user must override it to something and would aware of that. Also made some improvements on documentation. Reviewed By: myreg Differential Revision: D6144049 fbshipit-source-id: 4c9fd72e9efaeb6763d55f63760eaf582ee4839e --- diff --git a/folly/executors/test/ThreadPoolExecutorTest.cpp b/folly/executors/test/ThreadPoolExecutorTest.cpp index 85d26187..3e9b68a2 100644 --- a/folly/executors/test/ThreadPoolExecutorTest.cpp +++ b/folly/executors/test/ThreadPoolExecutorTest.cpp @@ -438,6 +438,11 @@ class TestData : public folly::RequestData { public: explicit TestData(int data) : data_(data) {} ~TestData() override {} + + bool hasCallback() override { + return false; + } + int data_; }; diff --git a/folly/futures/test/ContextTest.cpp b/folly/futures/test/ContextTest.cpp index a3dca7ec..952c9fa2 100644 --- a/folly/futures/test/ContextTest.cpp +++ b/folly/futures/test/ContextTest.cpp @@ -25,6 +25,11 @@ class TestData : public RequestData { public: explicit TestData(int data) : data_(data) {} ~TestData() override {} + + bool hasCallback() override { + return false; + } + int data_; }; diff --git a/folly/futures/test/FutureTest.cpp b/folly/futures/test/FutureTest.cpp index 08b712e6..e269aa15 100644 --- a/folly/futures/test/FutureTest.cpp +++ b/folly/futures/test/FutureTest.cpp @@ -854,6 +854,11 @@ TEST(Future, RequestContext) { struct MyRequestData : RequestData { MyRequestData(bool value = false) : value(value) {} + + bool hasCallback() override { + return false; + } + bool value; }; diff --git a/folly/io/async/EventBase.h b/folly/io/async/EventBase.h index 83e43cb4..e688abe7 100644 --- a/folly/io/async/EventBase.h +++ b/folly/io/async/EventBase.h @@ -95,6 +95,10 @@ class RequestEventBase : public RequestData { std::unique_ptr(new RequestEventBase(eb))); } + bool hasCallback() override { + return false; + } + private: explicit RequestEventBase(EventBase* eb) : eb_(eb) {} EventBase* eb_; diff --git a/folly/io/async/Request.cpp b/folly/io/async/Request.cpp index b1dd0f79..5a0092c4 100644 --- a/folly/io/async/Request.cpp +++ b/folly/io/async/Request.cpp @@ -24,63 +24,81 @@ namespace folly { +bool RequestContext::doSetContextData( + const std::string& val, + std::unique_ptr& data, + bool strict) { + auto ulock = state_.ulock(); + + bool conflict = false; + auto it = ulock->requestData_.find(val); + if (it != ulock->requestData_.end()) { + if (strict) { + return false; + } else { + LOG_FIRST_N(WARNING, 1) << "Calling RequestContext::setContextData for " + << val << " but it is already set"; + conflict = true; + } + } + + auto wlock = ulock.moveFromUpgradeToWrite(); + if (conflict) { + if (it->second) { + if (it->second->hasCallback()) { + wlock->callbackData_.erase(it->second.get()); + } + it->second.reset(nullptr); + } + return true; + } + + if (data && data->hasCallback()) { + wlock->callbackData_.insert(data.get()); + } + wlock->requestData_[val] = std::move(data); + + return true; +} + void RequestContext::setContextData( const std::string& val, std::unique_ptr data) { - auto wlock = data_.wlock(); - if (wlock->count(val)) { - LOG_FIRST_N(WARNING, 1) - << "Called RequestContext::setContextData with data already set"; - - (*wlock)[val] = nullptr; - } else { - (*wlock)[val] = std::move(data); - } + doSetContextData(val, data, false /* strict */); } bool RequestContext::setContextDataIfAbsent( const std::string& val, std::unique_ptr data) { - auto ulock = data_.ulock(); - if (ulock->count(val)) { - return false; - } - - auto wlock = ulock.moveFromUpgradeToWrite(); - (*wlock)[val] = std::move(data); - return true; + return doSetContextData(val, data, true /* strict */); } bool RequestContext::hasContextData(const std::string& val) const { - return data_.rlock()->count(val); + return state_.rlock()->requestData_.count(val); } RequestData* RequestContext::getContextData(const std::string& val) { const std::unique_ptr dflt{nullptr}; - return get_ref_default(*data_.rlock(), val, dflt).get(); + return get_ref_default(state_.rlock()->requestData_, val, dflt).get(); } const RequestData* RequestContext::getContextData( const std::string& val) const { const std::unique_ptr dflt{nullptr}; - return get_ref_default(*data_.rlock(), val, dflt).get(); + return get_ref_default(state_.rlock()->requestData_, val, dflt).get(); } void RequestContext::onSet() { - auto rlock = data_.rlock(); - for (auto const& ent : *rlock) { - if (auto& data = ent.second) { - data->onSet(); - } + auto rlock = state_.rlock(); + for (const auto& data : rlock->callbackData_) { + data->onSet(); } } void RequestContext::onUnset() { - auto rlock = data_.rlock(); - for (auto const& ent : *rlock) { - if (auto& data = ent.second) { - data->onUnset(); - } + auto rlock = state_.rlock(); + for (const auto& data : rlock->callbackData_) { + data->onUnset(); } } @@ -89,12 +107,19 @@ void RequestContext::clearContextData(const std::string& val) { // Delete the RequestData after giving up the wlock just in case one of the // RequestData destructors will try to grab the lock again. { - auto wlock = data_.wlock(); - auto it = wlock->find(val); - if (it != wlock->end()) { - requestData = std::move(it->second); - wlock->erase(it); + auto ulock = state_.ulock(); + auto it = ulock->requestData_.find(val); + if (it == ulock->requestData_.end()) { + return; } + + auto wlock = ulock.moveFromUpgradeToWrite(); + if (it->second && it->second->hasCallback()) { + wlock->callbackData_.erase(it->second.get()); + } + + requestData = std::move(it->second); + wlock->requestData_.erase(it); } } diff --git a/folly/io/async/Request.h b/folly/io/async/Request.h index ce733301..7398acb2 100644 --- a/folly/io/async/Request.h +++ b/folly/io/async/Request.h @@ -18,8 +18,8 @@ #include #include +#include -#include #include namespace folly { @@ -30,16 +30,23 @@ namespace folly { class RequestData { public: virtual ~RequestData() = default; + // Avoid calling RequestContext::setContextData, setContextDataIfAbsent, or // clearContextData from these callbacks. Doing so will cause deadlock. We // could fix these deadlocks, but only at significant performance penalty, so // just don't do it! + + virtual bool hasCallback() = 0; + // Callback executed when setting RequestContext. Make sure your RequestData + // instance overrides the hasCallback method to return true otherwise + // the callback will not be executed virtual void onSet() {} + // Callback executed when unsetting RequestContext. Make sure your RequestData + // instance overrides the hasCallback method to return true otherwise + // the callback will not be executed virtual void onUnset() {} }; -class RequestContext; - // If you do not call create() to create a unique request context, // this default request context will always be returned, and is never // copied between threads. @@ -55,29 +62,42 @@ class RequestContext { // Get the current context. static RequestContext* get(); - // The following API may be used to set per-request data in a thread-safe way. - // This access is still performance sensitive, so please ask if you need help - // profiling any use of these functions. + // The following APIs are used to add, remove and access RequestData instance + // in the RequestContext instance, normally used for per-RequestContext + // tracking or callback on set and unset. These APIs are Thread-safe. + // These APIs are performance sensitive, so please ask if you need help + // profiling any use of these APIs. + + // Add RequestData instance "data" to this RequestContext instance, with + // string identifier "val". If the same string identifier has already been + // used, will print a warning message for the first time, clear the existing + // RequestData instance for "val", and **not** add "data". void setContextData( const std::string& val, std::unique_ptr data); - // Unlike setContextData, this method does not panic if the key is already - // present. Returns true iff the new value has been inserted. + // Add RequestData instance "data" to this RequestContext instance, with + // string identifier "val". If the same string identifier has already been + // used, return false and do nothing. Otherwise add "data" and return true. bool setContextDataIfAbsent( const std::string& val, std::unique_ptr data); + // Remove the RequestData instance with string identifier "val", if it exists. + void clearContextData(const std::string& val); + + // Returns true if and only if the RequestData instance with string identifier + // "val" exists in this RequestContext instnace. bool hasContextData(const std::string& val) const; + // Get (constant) raw pointer of the RequestData instance with string + // identifier "val" if it exists, otherwise returns null pointer. RequestData* getContextData(const std::string& val); const RequestData* getContextData(const std::string& val) const; void onSet(); void onUnset(); - void clearContextData(const std::string& val); - // The following API is used to pass the context through queues / threads. // saveContext is called to get a shared_ptr to the context, and // setContext is used to reset it on the other side of the queue. @@ -98,8 +118,16 @@ class RequestContext { private: static std::shared_ptr& getStaticContext(); - using Data = std::map>; - folly::Synchronized data_; + bool doSetContextData( + const std::string& val, + std::unique_ptr& data, + bool strict); + + struct State { + std::map> requestData_; + std::set callbackData_; + }; + folly::Synchronized state_; }; class RequestContextScopeGuard { @@ -121,8 +149,7 @@ class RequestContextScopeGuard { // Set a RequestContext that was previously captured by saveContext(). It will // be automatically reset to the original value when this goes out of scope. explicit RequestContextScopeGuard(std::shared_ptr ctx) - : prev_(RequestContext::setContext(std::move(ctx))) { - } + : prev_(RequestContext::setContext(std::move(ctx))) {} ~RequestContextScopeGuard() { RequestContext::setContext(std::move(prev_)); diff --git a/folly/io/async/test/RequestContextTest.cpp b/folly/io/async/test/RequestContextTest.cpp index 750ff7fa..6758a9a9 100644 --- a/folly/io/async/test/RequestContextTest.cpp +++ b/folly/io/async/test/RequestContextTest.cpp @@ -27,12 +27,19 @@ class TestData : public RequestData { public: explicit TestData(int data) : data_(data) {} ~TestData() override {} + + bool hasCallback() override { + return true; + } + void onSet() override { set_++; } + void onUnset() override { unset_++; } + int set_ = 0, unset_ = 0; int data_; }; @@ -136,9 +143,9 @@ TEST(RequestContext, deadlockTest) { val_, std::make_unique(1)); } - void onSet() override {} - - void onUnset() override {} + bool hasCallback() override { + return false; + } std::string val_; };