From 9fd84b4ba6b48327942149f96935e145642c4a46 Mon Sep 17 00:00:00 2001
From: Yedidya Feldblum <yfeldblum@meta.com>
Date: Wed, 4 Dec 2024 12:22:16 -0800
Subject: [PATCH] let ServiceData::trimRegexCache avoid exclusive lock

Summary: Currently, `ServiceData::trimRegexCache` always takes an exclusive lock and invokes `RegexMatchCache::purge`, even if there would be nothing to purge (i.e., all regexes in the cache are fresh / non-stale). It is possible to avoid tail risk of taking an exclusive lock by prefacing with `RegexMatchCache::hasItemsToPurge`, done under an upgrade lock.

Reviewed By: mdas7

Differential Revision: D66730361

fbshipit-source-id: 6a8368ba701ef35c1f347e8a61669fb9392825b0
---
 fb303/CallbackValuesMap-inl.h  | 6 ++++++
 fb303/CallbackValuesMap.h      | 4 +---
 fb303/ServiceData.cpp          | 2 +-
 fb303/detail/QuantileStatMap.h | 4 ++--
 fb303/detail/RegexUtil.h       | 9 +++++++++
 5 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/fb303/CallbackValuesMap-inl.h b/fb303/CallbackValuesMap-inl.h
index 9981a0783..70e524604 100644
--- a/fb303/CallbackValuesMap-inl.h
+++ b/fb303/CallbackValuesMap-inl.h
@@ -124,6 +124,12 @@ void CallbackValuesMap<T>::clear() {
   detail::cachedClearStrings(*wlock);
 }
 
+template <typename T>
+void CallbackValuesMap<T>::trimRegexCache(
+    const folly::RegexMatchCache::time_point expiry) {
+  detail::cachedTrimStale(callbackMap_, expiry);
+}
+
 template <typename T>
 std::shared_ptr<typename CallbackValuesMap<T>::CallbackEntry>
 CallbackValuesMap<T>::getCallback(folly::StringPiece name) {
diff --git a/fb303/CallbackValuesMap.h b/fb303/CallbackValuesMap.h
index 6d3924c49..4ee268abe 100644
--- a/fb303/CallbackValuesMap.h
+++ b/fb303/CallbackValuesMap.h
@@ -92,9 +92,7 @@ class CallbackValuesMap {
    */
   void clear();
 
-  void trimRegexCache(folly::RegexMatchCache::time_point expiry) {
-    callbackMap_.wlock()->matches.purge(expiry);
-  }
+  void trimRegexCache(folly::RegexMatchCache::time_point expiry);
 
   class CallbackEntry {
    public:
diff --git a/fb303/ServiceData.cpp b/fb303/ServiceData.cpp
index dba4c25d6..e753de05e 100644
--- a/fb303/ServiceData.cpp
+++ b/fb303/ServiceData.cpp
@@ -511,7 +511,7 @@ map<string, int64_t> ServiceData::getRegexCountersOptimized(
 void ServiceData::trimRegexCache(const std::chrono::seconds maxstale) {
   const auto now = folly::RegexMatchCache::clock::now();
   const auto expiry = now - maxstale;
-  counters_.wlock()->matches.purge(expiry);
+  detail::cachedTrimStale(counters_, expiry);
   quantileMap_.trimRegexCache(expiry);
   dynamicCounters_.trimRegexCache(expiry);
 }
diff --git a/fb303/detail/QuantileStatMap.h b/fb303/detail/QuantileStatMap.h
index 84e6c325e..1d13d456d 100644
--- a/fb303/detail/QuantileStatMap.h
+++ b/fb303/detail/QuantileStatMap.h
@@ -116,8 +116,8 @@ class BasicQuantileStatMap {
     countersWLock->bases.clear();
   }
 
-  void trimRegexCache(folly::RegexMatchCache::time_point expiry) {
-    counters_.wlock()->matches.purge(expiry);
+  void trimRegexCache(const folly::RegexMatchCache::time_point expiry) {
+    detail::cachedTrimStale(counters_, expiry);
   }
 
  private:
diff --git a/fb303/detail/RegexUtil.h b/fb303/detail/RegexUtil.h
index aa801de6b..d8122ab38 100644
--- a/fb303/detail/RegexUtil.h
+++ b/fb303/detail/RegexUtil.h
@@ -84,4 +84,13 @@ void cachedFindMatches(
   cachedFindMatchesCopyUnderSharedLock(out, r->matches, regex, now);
 }
 
+template <typename SyncMap>
+void cachedTrimStale(
+    SyncMap& map,
+    folly::RegexMatchCache::time_point const expiry) {
+  if (auto ulock = map.ulock(); ulock->matches.hasItemsToPurge(expiry)) {
+    ulock.moveFromUpgradeToWrite()->matches.purge(expiry);
+  }
+}
+
 } // namespace facebook::fb303::detail