Commit b82902de authored by Teng Qin's avatar Teng Qin Committed by Facebook Github Bot

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
parent 3efddaff
...@@ -438,6 +438,11 @@ class TestData : public folly::RequestData { ...@@ -438,6 +438,11 @@ class TestData : public folly::RequestData {
public: public:
explicit TestData(int data) : data_(data) {} explicit TestData(int data) : data_(data) {}
~TestData() override {} ~TestData() override {}
bool hasCallback() override {
return false;
}
int data_; int data_;
}; };
......
...@@ -25,6 +25,11 @@ class TestData : public RequestData { ...@@ -25,6 +25,11 @@ class TestData : public RequestData {
public: public:
explicit TestData(int data) : data_(data) {} explicit TestData(int data) : data_(data) {}
~TestData() override {} ~TestData() override {}
bool hasCallback() override {
return false;
}
int data_; int data_;
}; };
......
...@@ -854,6 +854,11 @@ TEST(Future, RequestContext) { ...@@ -854,6 +854,11 @@ TEST(Future, RequestContext) {
struct MyRequestData : RequestData { struct MyRequestData : RequestData {
MyRequestData(bool value = false) : value(value) {} MyRequestData(bool value = false) : value(value) {}
bool hasCallback() override {
return false;
}
bool value; bool value;
}; };
......
...@@ -95,6 +95,10 @@ class RequestEventBase : public RequestData { ...@@ -95,6 +95,10 @@ class RequestEventBase : public RequestData {
std::unique_ptr<RequestEventBase>(new RequestEventBase(eb))); std::unique_ptr<RequestEventBase>(new RequestEventBase(eb)));
} }
bool hasCallback() override {
return false;
}
private: private:
explicit RequestEventBase(EventBase* eb) : eb_(eb) {} explicit RequestEventBase(EventBase* eb) : eb_(eb) {}
EventBase* eb_; EventBase* eb_;
......
...@@ -24,63 +24,81 @@ ...@@ -24,63 +24,81 @@
namespace folly { namespace folly {
bool RequestContext::doSetContextData(
const std::string& val,
std::unique_ptr<RequestData>& 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( void RequestContext::setContextData(
const std::string& val, const std::string& val,
std::unique_ptr<RequestData> data) { std::unique_ptr<RequestData> data) {
auto wlock = data_.wlock(); doSetContextData(val, data, false /* strict */);
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);
}
} }
bool RequestContext::setContextDataIfAbsent( bool RequestContext::setContextDataIfAbsent(
const std::string& val, const std::string& val,
std::unique_ptr<RequestData> data) { std::unique_ptr<RequestData> data) {
auto ulock = data_.ulock(); return doSetContextData(val, data, true /* strict */);
if (ulock->count(val)) {
return false;
}
auto wlock = ulock.moveFromUpgradeToWrite();
(*wlock)[val] = std::move(data);
return true;
} }
bool RequestContext::hasContextData(const std::string& val) const { 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) { RequestData* RequestContext::getContextData(const std::string& val) {
const std::unique_ptr<RequestData> dflt{nullptr}; const std::unique_ptr<RequestData> 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 RequestData* RequestContext::getContextData(
const std::string& val) const { const std::string& val) const {
const std::unique_ptr<RequestData> dflt{nullptr}; const std::unique_ptr<RequestData> dflt{nullptr};
return get_ref_default(*data_.rlock(), val, dflt).get(); return get_ref_default(state_.rlock()->requestData_, val, dflt).get();
} }
void RequestContext::onSet() { void RequestContext::onSet() {
auto rlock = data_.rlock(); auto rlock = state_.rlock();
for (auto const& ent : *rlock) { for (const auto& data : rlock->callbackData_) {
if (auto& data = ent.second) { data->onSet();
data->onSet();
}
} }
} }
void RequestContext::onUnset() { void RequestContext::onUnset() {
auto rlock = data_.rlock(); auto rlock = state_.rlock();
for (auto const& ent : *rlock) { for (const auto& data : rlock->callbackData_) {
if (auto& data = ent.second) { data->onUnset();
data->onUnset();
}
} }
} }
...@@ -89,12 +107,19 @@ void RequestContext::clearContextData(const std::string& val) { ...@@ -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 // Delete the RequestData after giving up the wlock just in case one of the
// RequestData destructors will try to grab the lock again. // RequestData destructors will try to grab the lock again.
{ {
auto wlock = data_.wlock(); auto ulock = state_.ulock();
auto it = wlock->find(val); auto it = ulock->requestData_.find(val);
if (it != wlock->end()) { if (it == ulock->requestData_.end()) {
requestData = std::move(it->second); return;
wlock->erase(it);
} }
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);
} }
} }
......
...@@ -18,8 +18,8 @@ ...@@ -18,8 +18,8 @@
#include <map> #include <map>
#include <memory> #include <memory>
#include <set>
#include <folly/SharedMutex.h>
#include <folly/Synchronized.h> #include <folly/Synchronized.h>
namespace folly { namespace folly {
...@@ -30,16 +30,23 @@ namespace folly { ...@@ -30,16 +30,23 @@ namespace folly {
class RequestData { class RequestData {
public: public:
virtual ~RequestData() = default; virtual ~RequestData() = default;
// Avoid calling RequestContext::setContextData, setContextDataIfAbsent, or // Avoid calling RequestContext::setContextData, setContextDataIfAbsent, or
// clearContextData from these callbacks. Doing so will cause deadlock. We // clearContextData from these callbacks. Doing so will cause deadlock. We
// could fix these deadlocks, but only at significant performance penalty, so // could fix these deadlocks, but only at significant performance penalty, so
// just don't do it! // 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() {} 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() {} virtual void onUnset() {}
}; };
class RequestContext;
// If you do not call create() to create a unique request context, // If you do not call create() to create a unique request context,
// this default request context will always be returned, and is never // this default request context will always be returned, and is never
// copied between threads. // copied between threads.
...@@ -55,29 +62,42 @@ class RequestContext { ...@@ -55,29 +62,42 @@ class RequestContext {
// Get the current context. // Get the current context.
static RequestContext* get(); static RequestContext* get();
// The following API may be used to set per-request data in a thread-safe way. // The following APIs are used to add, remove and access RequestData instance
// This access is still performance sensitive, so please ask if you need help // in the RequestContext instance, normally used for per-RequestContext
// profiling any use of these functions. // 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( void setContextData(
const std::string& val, const std::string& val,
std::unique_ptr<RequestData> data); std::unique_ptr<RequestData> data);
// Unlike setContextData, this method does not panic if the key is already // Add RequestData instance "data" to this RequestContext instance, with
// present. Returns true iff the new value has been inserted. // 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( bool setContextDataIfAbsent(
const std::string& val, const std::string& val,
std::unique_ptr<RequestData> data); std::unique_ptr<RequestData> 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; 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); RequestData* getContextData(const std::string& val);
const RequestData* getContextData(const std::string& val) const; const RequestData* getContextData(const std::string& val) const;
void onSet(); void onSet();
void onUnset(); void onUnset();
void clearContextData(const std::string& val);
// The following API is used to pass the context through queues / threads. // The following API is used to pass the context through queues / threads.
// saveContext is called to get a shared_ptr to the context, and // 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. // setContext is used to reset it on the other side of the queue.
...@@ -98,8 +118,16 @@ class RequestContext { ...@@ -98,8 +118,16 @@ class RequestContext {
private: private:
static std::shared_ptr<RequestContext>& getStaticContext(); static std::shared_ptr<RequestContext>& getStaticContext();
using Data = std::map<std::string, std::unique_ptr<RequestData>>; bool doSetContextData(
folly::Synchronized<Data, folly::SharedMutex> data_; const std::string& val,
std::unique_ptr<RequestData>& data,
bool strict);
struct State {
std::map<std::string, std::unique_ptr<RequestData>> requestData_;
std::set<RequestData*> callbackData_;
};
folly::Synchronized<State> state_;
}; };
class RequestContextScopeGuard { class RequestContextScopeGuard {
...@@ -121,8 +149,7 @@ class RequestContextScopeGuard { ...@@ -121,8 +149,7 @@ class RequestContextScopeGuard {
// Set a RequestContext that was previously captured by saveContext(). It will // Set a RequestContext that was previously captured by saveContext(). It will
// be automatically reset to the original value when this goes out of scope. // be automatically reset to the original value when this goes out of scope.
explicit RequestContextScopeGuard(std::shared_ptr<RequestContext> ctx) explicit RequestContextScopeGuard(std::shared_ptr<RequestContext> ctx)
: prev_(RequestContext::setContext(std::move(ctx))) { : prev_(RequestContext::setContext(std::move(ctx))) {}
}
~RequestContextScopeGuard() { ~RequestContextScopeGuard() {
RequestContext::setContext(std::move(prev_)); RequestContext::setContext(std::move(prev_));
......
...@@ -27,12 +27,19 @@ class TestData : public RequestData { ...@@ -27,12 +27,19 @@ class TestData : public RequestData {
public: public:
explicit TestData(int data) : data_(data) {} explicit TestData(int data) : data_(data) {}
~TestData() override {} ~TestData() override {}
bool hasCallback() override {
return true;
}
void onSet() override { void onSet() override {
set_++; set_++;
} }
void onUnset() override { void onUnset() override {
unset_++; unset_++;
} }
int set_ = 0, unset_ = 0; int set_ = 0, unset_ = 0;
int data_; int data_;
}; };
...@@ -136,9 +143,9 @@ TEST(RequestContext, deadlockTest) { ...@@ -136,9 +143,9 @@ TEST(RequestContext, deadlockTest) {
val_, std::make_unique<TestData>(1)); val_, std::make_unique<TestData>(1));
} }
void onSet() override {} bool hasCallback() override {
return false;
void onUnset() override {} }
std::string val_; std::string val_;
}; };
......
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