Kill MutexWrapper

Summary: `fb303` uses `SharedMutex` for its `Synchronized` objects, but it does so through the `MutexWrapper` adapter, so that all the `lock()` calls did not have to be codemodded to `wlock()`. However, in some codepaths it is actually possible to use an rlock, so it's best to remove the wrapper and just codemod all the call sites.

Reviewed By: uvdn7

Differential Revision: D84917811

fbshipit-source-id: 33c3fbee5d9058c027da821bca30f2e725c124a5

Author: ot

fb303/BUCK

@@ -74,7 +74,6 @@ cpp_library(
     ],
     exported_deps = [
         ":export_type",
-        ":mutex_wrapper",
         ":timeseries",
         "//folly:synchronized",
         "//folly/container:f14_hash",
@@ -162,7 +161,6 @@ cpp_library(
         ":dynamic_counters",
         ":export_type",
         ":exported_stat_map_impl",
-        ":mutex_wrapper",
         ":timeseries_histogram",
         "//folly:function",
         "//folly:map_util",
@@ -175,17 +173,6 @@ cpp_library(
     ],
 )
 
-cpp_library(
-    name = "mutex_wrapper",
-    srcs = [],
-    headers = ["MutexWrapper.h"],
-    modular_headers = True,
-    exported_deps = [
-        "//folly:shared_mutex",
-        "//folly:synchronized",
-    ],
-)
-
 cpp_library(
     name = "service_data",
     srcs = ["ServiceData.cpp"],
@@ -337,8 +324,8 @@ cpp_library(
     ],
     exported_deps = [
         ":export_type",
-        ":mutex_wrapper",
         ":timeseries",
+        "//folly:synchronized",
     ],
     external_deps = [
         "glog",

fb303/ExportedHistogramMap.cpp

@@ -26,7 +26,7 @@ CounterType getHistogramPercentile(
     const ExportedHistogramMap::HistogramPtr& hist,
     int level,
     double percentile) {
-  auto lockedHist = hist->lock();
+  auto lockedHist = hist->wlock();
 
   // make sure the histogram is up to date and data is decayed appropriately
   lockedHist->update(get_legacy_stats_time());
@@ -145,7 +145,7 @@ void ExportedHistogramMap::checkAdd(
     int64_t max) const {
   // Log an error if someone tries to create an existing histogram with
   // different parameters.
-  auto lockedHist = item->lock();
+  auto lockedHist = item->wlock();
   if (lockedHist->getBucketSize() != bucketWidth ||
       lockedHist->getMin() != min || lockedHist->getMax() != max) {
     LOG(ERROR) << "Attempted to create an existing histogram with "
@@ -202,7 +202,7 @@ void ExportedHistogramMap::unexportStat(StringPiece name, ExportType type) {
 void ExportedHistogramMap::clearAllHistograms() {
   auto lockedHistMap = histMap_.wlock();
   for (auto& histPtrKvp : *lockedHistMap) {
-    histPtrKvp.second->lock()->clear();
+    histPtrKvp.second->wlock()->clear();
   }
 }
 } // namespace facebook::fb303

fb303/ExportedHistogramMap.h

@@ -18,7 +18,6 @@
 
 #include <fb303/DynamicCounters.h>
 #include <fb303/ExportType.h>
-#include <fb303/MutexWrapper.h>
 #include <fb303/TimeseriesHistogram.h>
 #include <folly/Function.h>
 #include <folly/MapUtil.h>
@@ -62,9 +61,9 @@ using ExportedStatForHistogram = MultiLevelTimeSeries<
 
 class ExportedHistogramMap {
  public:
-  using SyncHistogram = folly::Synchronized<ExportedHistogram, MutexWrapper>;
+  using SyncHistogram = folly::Synchronized<ExportedHistogram>;
   using HistogramPtr = std::shared_ptr<SyncHistogram>;
-  using LockedHistogramPtr = SyncHistogram::LockedPtr;
+  using LockedHistogramPtr = SyncHistogram::WLockedPtr;
   using HistMap = folly::F14NodeMap<std::string, HistogramPtr>;
   using MakeExportedHistogram = folly::FunctionRef<ExportedHistogram()>;
 
@@ -136,7 +135,7 @@ class ExportedHistogramMap {
   LockedHistogramPtr getHistogram(folly::StringPiece name) {
     auto hist = getHistogramUnlocked(name);
     if (hist) {
-      return hist->lock();
+      return hist->wlock();
     }
     return LockedHistogramPtr();
   }
@@ -302,7 +301,7 @@ class ExportedHistogramMap {
       int64_t times = 1) {
     HistogramPtr hist = getHistogramUnlocked(name);
     if (hist) {
-      hist->lock()->addValue(now, value, times);
+      hist->wlock()->addValue(now, value, times);
     }
   }
 
@@ -312,7 +311,7 @@ class ExportedHistogramMap {
       const folly::Histogram<CounterType>& values) {
     HistogramPtr hist = getHistogramUnlocked(name);
     if (hist) {
-      hist->lock()->addValues(now, values);
+      hist->wlock()->addValues(now, values);
     }
   }
 
@@ -360,7 +359,7 @@ class ExportedHistogramMap {
   void clearHistogram(folly::StringPiece name) {
     HistogramPtr hist = getHistogramUnlocked(name);
     if (hist) {
-      hist->lock()->clear();
+      hist->wlock()->clear();
     }
   }
 

fb303/ExportedHistogramMapImpl.h

@@ -47,7 +47,7 @@ class ExportedHistogramMapImpl : public ExportedHistogramMap {
      * locked as long as the LockedHistogramPtr remains in scope.
      */
     LockedHistogramPtr makeLockGuard() const {
-      return hist_->lock();
+      return hist_->wlock();
     }
 
     /** Return true if histogram is null. */
@@ -65,23 +65,23 @@ class ExportedHistogramMapImpl : public ExportedHistogramMap {
      * locked for the duration of this call.
      */
     CounterType getBucketSize() const {
-      return hist_->lock()->getBucketSize();
+      return hist_->wlock()->getBucketSize();
     }
 
     /**
      * Return the minimum value of this associated histogram. The histogram
      * remains locked for the duration of this call.
      */
     CounterType getMin() const {
-      return hist_->lock()->getMin();
+      return hist_->wlock()->getMin();
     }
 
     /**
      * Return the maximum value of the associated histogram. The histogram
      * remains locked for the duration of this call.
      */
     CounterType getMax() const {
-      return hist_->lock()->getMax();
+      return hist_->wlock()->getMax();
     }
 
     /**
@@ -91,7 +91,7 @@ class ExportedHistogramMapImpl : public ExportedHistogramMap {
      */
     void addValue(time_t now, const CounterType value, int64_t times = 1)
         const {
-      hist_->lock()->addValue(now, value, times);
+      hist_->wlock()->addValue(now, value, times);
     }
 
     /**
@@ -115,10 +115,10 @@ class ExportedHistogramMapImpl : public ExportedHistogramMap {
      * histogram. The histogram remains locked for the duration of this call.
      */
     void addValues(std::chrono::seconds now, const Histogram& values) const {
-      hist_->lock()->addValues(now, values);
+      hist_->wlock()->addValues(now, values);
     }
     void addValues(time_t now, const Histogram& values) const {
-      hist_->lock()->addValues(now, values);
+      hist_->wlock()->addValues(now, values);
     }
 
     /**
@@ -217,7 +217,7 @@ class ExportedHistogramMapImpl : public ExportedHistogramMap {
       time_t now,
       CounterType value,
       int64_t times = 1) {
-    histogram->lock()->addValue(now, value, times);
+    histogram->wlock()->addValue(now, value, times);
   }
 
   /*

fb303/ExportedStatMap.cpp

@@ -135,14 +135,14 @@ void ExportedStatMap::forgetStatsFor(StringPiece name) {
 void ExportedStatMap::flushAllStats() {
   auto lockedStatMap = statMap_.wlock();
   for (auto& [_, ptr] : *lockedStatMap) {
-    ptr->lock()->flush();
+    ptr->wlock()->flush();
   }
 }
 
 void ExportedStatMap::clearAllStats() {
   auto lockedStatMap = statMap_.wlock();
   for (auto& [_, ptr] : *lockedStatMap) {
-    ptr->lock()->clear();
+    ptr->wlock()->clear();
   }
 }
 

fb303/ExportedStatMap.h

@@ -19,7 +19,6 @@
 #include <memory>
 
 #include <fb303/ExportType.h>
-#include <fb303/MutexWrapper.h>
 #include <fb303/Timeseries.h>
 #include <folly/Synchronized.h>
 #include <folly/container/F14Map.h>
@@ -40,9 +39,9 @@ using TimePoint = ExportedStat::TimePoint;
 
 class ExportedStatMap {
  public:
-  using SyncStat = folly::Synchronized<ExportedStat, MutexWrapper>;
+  using SyncStat = folly::Synchronized<ExportedStat>;
   using StatPtr = std::shared_ptr<SyncStat>;
-  using LockedStatPtr = SyncStat::LockedPtr;
+  using LockedStatPtr = SyncStat::WLockedPtr;
   using StatMap = folly::F14FastMap<std::string, StatPtr>;
 
   /*
@@ -119,7 +118,7 @@ class ExportedStatMap {
    * --
    */
   LockedStatPtr getLockedStatPtr(folly::StringPiece name) {
-    auto result = getStatPtr(name)->lock();
+    auto result = getStatPtr(name)->wlock();
     result->flush();
     return result;
   }
@@ -179,7 +178,7 @@ class ExportedStatMap {
       TimePoint now,
       CounterType value,
       ExportType type) {
-    getStatPtr(name, &type)->lock()->addValue(now, value);
+    getStatPtr(name, &type)->wlock()->addValue(now, value);
   }
 
   /*
@@ -191,7 +190,7 @@ class ExportedStatMap {
       TimePoint now,
       CounterType value,
       folly::Range<const ExportType*> exportTypes) {
-    getStatPtr(name, exportTypes)->lock()->addValue(now, value);
+    getStatPtr(name, exportTypes)->wlock()->addValue(now, value);
   }
 
   /*
@@ -203,7 +202,7 @@ class ExportedStatMap {
       TimePoint now,
       CounterType value,
       int64_t times = 1) {
-    getStatPtr(name)->lock()->addValue(now, value, times);
+    getStatPtr(name)->wlock()->addValue(now, value, times);
   }
 
   /*
@@ -215,14 +214,14 @@ class ExportedStatMap {
       TimePoint now,
       CounterType sum,
       int64_t nsamples) {
-    getStatPtr(name)->lock()->addValueAggregated(now, sum, nsamples);
+    getStatPtr(name)->wlock()->addValueAggregated(now, sum, nsamples);
   }
 
   /*
    * Removes all entries from the map specified by 'name.'
    */
   void clearValue(folly::StringPiece name) {
-    getStatPtr(name)->lock()->clear();
+    getStatPtr(name)->wlock()->clear();
   }
 
   /*

fb303/ExportedStatMapImpl.h

@@ -41,11 +41,11 @@ class ExportedStatMapImpl : public ExportedStatMap {
 
     /*
      * Locks the timeseries object that is held by this LockableStat object
-     * and returns the LockedPtr for it. The histogram remains locked as long as
-     * the LockedPtr object remains in scope.
+     * and returns the LockedStatPtr for it. The histogram remains locked as
+     * long as the LockedStatPtr object remains in scope.
      */
     LockedStatPtr lock() const {
-      return stat_->lock();
+      return stat_->wlock();
     }
 
     /* Return true if the stat held by this object is null. */
@@ -70,7 +70,7 @@ class ExportedStatMapImpl : public ExportedStatMap {
      */
     void addValue(TimePoint now, const CounterType value, int64_t times = 1)
         const {
-      stat_->lock()->addValue(now, value, times);
+      stat_->wlock()->addValue(now, value, times);
     }
 
     /*
@@ -81,15 +81,15 @@ class ExportedStatMapImpl : public ExportedStatMap {
         TimePoint now,
         const CounterType value,
         int64_t numSamples) const {
-      stat_->lock()->addValueAggregated(now, value, numSamples);
+      stat_->wlock()->addValueAggregated(now, value, numSamples);
     }
 
     /*
      * Add a value at time 'now' to all levels. The optional 'times' parameter
      * can be used here to add multiple copies of the value at a time.
      *
      * This method assumes that the object has already been locked, and requires
-     * the appropriate LockedPtr object as a parameter.
+     * the appropriate LockedStatPtr object as a parameter.
      */
     void addValueLocked(
         const LockedStatPtr& lockedObj,
@@ -104,7 +104,7 @@ class ExportedStatMapImpl : public ExportedStatMap {
      * Update the histogram with the given time value.
      *
      * This method assumes that the object has already been locked, and requires
-     * the appropriate LockedPtr object as a parameter.
+     * the appropriate LockedStatPtr object as a parameter.
      */
     void updateLocked(const LockedStatPtr& lockedObj, TimePoint now) {
       DCHECK(!lockedObj.isNull());
@@ -115,7 +115,7 @@ class ExportedStatMapImpl : public ExportedStatMap {
      * Flush all cached updates.
      *
      * This method assumes that the object has already been locked, and requires
-     * the appropriate LockedPtr object as a parameter.
+     * the appropriate LockedStatPtr object as a parameter.
      */
     void flushLocked(const LockedStatPtr& lockedObj) {
       DCHECK(!lockedObj.isNull());
@@ -128,14 +128,14 @@ class ExportedStatMapImpl : public ExportedStatMap {
      */
     template <typename ReturnType>
     ReturnType rate(int level) {
-      return stat_->lock()->rate<ReturnType>(level);
+      return stat_->wlock()->rate<ReturnType>(level);
     }
 
     /*
      * Return the sum of all the data points currently tracked at this level.
      *
      * This method assumes that the object has already been locked, and requires
-     * the appropriate LockedPtr object as a parameter.
+     * the appropriate LockedStatPtr object as a parameter.
      */
     CounterType getSumLocked(const LockedStatPtr& lockedObj, int level) {
       DCHECK(!lockedObj.isNull());
@@ -217,7 +217,7 @@ class ExportedStatMapImpl : public ExportedStatMap {
    */
   void
   addValue(StatPtr& item, TimePoint now, CounterType value, int64_t times = 1) {
-    item->lock()->addValue(now, value, times);
+    item->wlock()->addValue(now, value, times);
   }
 
   using ExportedStatMap::addValueAggregated;
@@ -238,7 +238,7 @@ class ExportedStatMapImpl : public ExportedStatMap {
       TimePoint now,
       CounterType sum,
       int64_t nsamples) {
-    item->lock()->addValueAggregated(now, sum, nsamples);
+    item->wlock()->addValueAggregated(now, sum, nsamples);
   }
 
   using ExportedStatMap::exportStat;