Improve folly::RequestContext onSet and onUnset efficiency
authorTeng Qin <qinteng@fb.com>
Tue, 31 Oct 2017 20:49:15 +0000 (13:49 -0700)
committerFacebook Github Bot <facebook-github-bot@users.noreply.github.com>
Tue, 31 Oct 2017 20:54:54 +0000 (13:54 -0700)
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

folly/executors/test/ThreadPoolExecutorTest.cpp
folly/futures/test/ContextTest.cpp
folly/futures/test/FutureTest.cpp
folly/io/async/EventBase.h
folly/io/async/Request.cpp
folly/io/async/Request.h
folly/io/async/test/RequestContextTest.cpp

index 85d2618..3e9b68a 100644 (file)
@@ -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_;
 };
 
index a3dca7e..952c9fa 100644 (file)
@@ -25,6 +25,11 @@ class TestData : public RequestData {
  public:
   explicit TestData(int data) : data_(data) {}
   ~TestData() override {}
+
+  bool hasCallback() override {
+    return false;
+  }
+
   int data_;
 };
 
index 08b712e..e269aa1 100644 (file)
@@ -854,6 +854,11 @@ TEST(Future, RequestContext) {
 
   struct MyRequestData : RequestData {
     MyRequestData(bool value = false) : value(value) {}
+
+    bool hasCallback() override {
+      return false;
+    }
+
     bool value;
   };
 
index 83e43cb..e688abe 100644 (file)
@@ -95,6 +95,10 @@ class RequestEventBase : public RequestData {
         std::unique_ptr<RequestEventBase>(new RequestEventBase(eb)));
   }
 
+  bool hasCallback() override {
+    return false;
+  }
+
  private:
   explicit RequestEventBase(EventBase* eb) : eb_(eb) {}
   EventBase* eb_;
index b1dd0f7..5a0092c 100644 (file)
 
 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(
     const std::string& val,
     std::unique_ptr<RequestData> 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<RequestData> 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<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 std::string& val) const {
   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() {
-  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);
   }
 }
 
index ce73330..7398acb 100644 (file)
@@ -18,8 +18,8 @@
 
 #include <map>
 #include <memory>
+#include <set>
 
-#include <folly/SharedMutex.h>
 #include <folly/Synchronized.h>
 
 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<RequestData> 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<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;
 
+  // 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<RequestContext>& getStaticContext();
 
-  using Data = std::map<std::string, std::unique_ptr<RequestData>>;
-  folly::Synchronized<Data, folly::SharedMutex> data_;
+  bool doSetContextData(
+      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 {
@@ -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<RequestContext> ctx)
-      : prev_(RequestContext::setContext(std::move(ctx))) {
-  }
+      : prev_(RequestContext::setContext(std::move(ctx))) {}
 
   ~RequestContextScopeGuard() {
     RequestContext::setContext(std::move(prev_));
index 750ff7f..6758a9a 100644 (file)
@@ -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<TestData>(1));
     }
 
-    void onSet() override {}
-
-    void onUnset() override {}
+    bool hasCallback() override {
+      return false;
+    }
 
     std::string val_;
   };