CacheStorage: Refactor scheduler UMA recording to reduce code size.

Previously CacheStorage used some very large macros to record a number
of scheduler related histograms.  These macros produce a lot of code
bloat and are difficult to read.  This CL addresses those issues by
replacing the macros with functions.

Bug: 909894
Change-Id: Icc3adc91d72de2aeaac0f927c13f3e2bfb557e77
Reviewed-on: https://chromium-review.googlesource.com/c/1357400
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612864}

content/browser/cache_storage/cache_storage_histogram_utils.cc

@@ -3,6 +3,8 @@
 // found in the LICENSE file.
 
 #include "content/browser/cache_storage/cache_storage_histogram_utils.h"
+#include "base/metrics/histogram_functions.h"
+#include "base/metrics/histogram_macros.h"
 
 namespace content {
 
@@ -11,4 +13,131 @@ blink::mojom::CacheStorageError MakeErrorStorage(ErrorStorageType type) {
   return blink::mojom::CacheStorageError::kErrorStorage;
 }
 
+namespace {
+
+// Helper macro to return a literal base::StringPiece.  Once
+// we can use c++17 we can use string view literals instead.
+#define RETURN_LITERAL_STRING_PIECE(target)                \
+  do {                                                     \
+    static constexpr base::StringPiece kValue("." target); \
+    return kValue;                                         \
+  } while (0)
+
+base::StringPiece UMAToName(CacheStorageSchedulerUMA uma_type) {
+  switch (uma_type) {
+    case CacheStorageSchedulerUMA::kIsOperationSlow:
+      RETURN_LITERAL_STRING_PIECE("IsOperationSlow");
+    case CacheStorageSchedulerUMA::kOperationDuration:
+      RETURN_LITERAL_STRING_PIECE("OperationDuration2");
+    case CacheStorageSchedulerUMA::kQueueDuration:
+      RETURN_LITERAL_STRING_PIECE("QueueDuration2");
+    case CacheStorageSchedulerUMA::kQueueLength:
+      RETURN_LITERAL_STRING_PIECE("QueueLength");
+  }
+}
+
+base::StringPiece ClientToName(CacheStorageSchedulerClient client_type) {
+  switch (client_type) {
+    case CacheStorageSchedulerClient::kBackgroundSync:
+      RETURN_LITERAL_STRING_PIECE("BackgroundSyncManager");
+    case CacheStorageSchedulerClient::kCache:
+      RETURN_LITERAL_STRING_PIECE("Cache");
+    case CacheStorageSchedulerClient::kStorage:
+      RETURN_LITERAL_STRING_PIECE("CacheStorage");
+  }
+}
+
+bool ShouldRecordOpUMA(CacheStorageSchedulerOp op_type) {
+  return op_type != CacheStorageSchedulerOp::kBackgroundSync &&
+         op_type != CacheStorageSchedulerOp::kTest;
+}
+
+base::StringPiece OpToName(CacheStorageSchedulerOp op_type) {
+  switch (op_type) {
+    case CacheStorageSchedulerOp::kBackgroundSync:
+      NOTREACHED();
+      return "";
+    case CacheStorageSchedulerOp::kClose:
+      RETURN_LITERAL_STRING_PIECE("Close");
+    case CacheStorageSchedulerOp::kDelete:
+      RETURN_LITERAL_STRING_PIECE("Delete");
+    case CacheStorageSchedulerOp::kGetAllMatched:
+      RETURN_LITERAL_STRING_PIECE("GetAllMatched");
+    case CacheStorageSchedulerOp::kHas:
+      RETURN_LITERAL_STRING_PIECE("Has");
+    case CacheStorageSchedulerOp::kInit:
+      RETURN_LITERAL_STRING_PIECE("Init");
+    case CacheStorageSchedulerOp::kKeys:
+      RETURN_LITERAL_STRING_PIECE("Keys");
+    case CacheStorageSchedulerOp::kMatch:
+      RETURN_LITERAL_STRING_PIECE("Match");
+    case CacheStorageSchedulerOp::kMatchAll:
+      RETURN_LITERAL_STRING_PIECE("MatchAll");
+    case CacheStorageSchedulerOp::kOpen:
+      RETURN_LITERAL_STRING_PIECE("Open");
+    case CacheStorageSchedulerOp::kPut:
+      RETURN_LITERAL_STRING_PIECE("Put");
+    case CacheStorageSchedulerOp::kSize:
+      RETURN_LITERAL_STRING_PIECE("Size");
+    case CacheStorageSchedulerOp::kSizeThenClose:
+      RETURN_LITERAL_STRING_PIECE("SizeThenClose");
+    case CacheStorageSchedulerOp::kTest:
+      NOTREACHED();
+      return "";
+    case CacheStorageSchedulerOp::kWriteIndex:
+      RETURN_LITERAL_STRING_PIECE("WriteIndex");
+    case CacheStorageSchedulerOp::kWriteSideData:
+      RETURN_LITERAL_STRING_PIECE("WriteSideData");
+  }
+}
+
+std::string GetClientHistogramName(CacheStorageSchedulerUMA uma_type,
+                                   CacheStorageSchedulerClient client_type) {
+  std::string histogram_name("ServiceWorkerCache");
+  histogram_name.append(ClientToName(client_type).as_string());
+  histogram_name.append(".Scheduler");
+  histogram_name.append(UMAToName(uma_type).as_string());
+  return histogram_name;
+}
+
+}  // namespace
+
+void RecordCacheStorageSchedulerUMA(CacheStorageSchedulerUMA uma_type,
+                                    CacheStorageSchedulerClient client_type,
+                                    CacheStorageSchedulerOp op_type,
+                                    int value) {
+  DCHECK(uma_type == CacheStorageSchedulerUMA::kIsOperationSlow ||
+         uma_type == CacheStorageSchedulerUMA::kQueueLength);
+  DCHECK(client_type != CacheStorageSchedulerClient::kBackgroundSync ||
+         op_type == CacheStorageSchedulerOp::kBackgroundSync);
+  std::string histogram_name = GetClientHistogramName(uma_type, client_type);
+  if (uma_type == CacheStorageSchedulerUMA::kIsOperationSlow)
+    base::UmaHistogramBoolean(histogram_name, value);
+  else
+    base::UmaHistogramCounts10000(histogram_name, value);
+  if (!ShouldRecordOpUMA(op_type))
+    return;
+  histogram_name.append(OpToName(op_type).as_string());
+  if (uma_type == CacheStorageSchedulerUMA::kIsOperationSlow)
+    base::UmaHistogramBoolean(histogram_name, value);
+  else
+    base::UmaHistogramCounts10000(histogram_name, value);
+}
+
+void RecordCacheStorageSchedulerUMA(CacheStorageSchedulerUMA uma_type,
+                                    CacheStorageSchedulerClient client_type,
+                                    CacheStorageSchedulerOp op_type,
+                                    base::TimeDelta value) {
+  DCHECK(uma_type == CacheStorageSchedulerUMA::kOperationDuration ||
+         uma_type == CacheStorageSchedulerUMA::kQueueDuration);
+  DCHECK(client_type != CacheStorageSchedulerClient::kBackgroundSync ||
+         op_type == CacheStorageSchedulerOp::kBackgroundSync);
+  std::string histogram_name = GetClientHistogramName(uma_type, client_type);
+  base::UmaHistogramLongTimes(histogram_name, value);
+  if (!ShouldRecordOpUMA(op_type))
+    return;
+  histogram_name.append(OpToName(op_type).as_string());
+  base::UmaHistogramLongTimes(histogram_name, value);
+}
+
 }  // namespace content

content/browser/cache_storage/cache_storage_histogram_utils.h

@@ -5,7 +5,6 @@
 #ifndef CONTENT_BROWSER_CACHE_STORAGE_CACHE_STORAGE_HISTOGRAM_UTILS_H_
 #define CONTENT_BROWSER_CACHE_STORAGE_CACHE_STORAGE_HISTOGRAM_UTILS_H_
 
-#include "base/metrics/histogram_macros.h"
 #include "content/browser/cache_storage/cache_storage_scheduler_types.h"
 #include "third_party/blink/public/mojom/cache_storage/cache_storage.mojom.h"
 
@@ -42,107 +41,25 @@ enum class ErrorStorageType {
 
 blink::mojom::CacheStorageError MakeErrorStorage(ErrorStorageType type);
 
-// Metrics to make it easier to write histograms for several clients.
-#define CACHE_STORAGE_SCHEDULER_UMA_THUNK2(uma_type, args) \
-  UMA_HISTOGRAM_##uma_type args
+enum class CacheStorageSchedulerUMA {
+  kIsOperationSlow = 0,
+  kOperationDuration = 1,
+  kQueueDuration = 2,
+  kQueueLength = 3,
+};
 
-#define CACHE_STORAGE_SCHEDULER_UMA_THUNK(uma_type, op_type, histogram_name, \
-                                          ...)                               \
-  CACHE_STORAGE_SCHEDULER_UMA_THUNK2(uma_type,                               \
-                                     (histogram_name, ##__VA_ARGS__));       \
-  do {                                                                       \
-    switch (op_type) {                                                       \
-      case CacheStorageSchedulerOp::kBackgroundSync:                         \
-        /* do not record op-specific histograms for background sync */       \
-        break;                                                               \
-      case CacheStorageSchedulerOp::kClose:                                  \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK2(                                  \
-            uma_type, (histogram_name ".Close", ##__VA_ARGS__));             \
-        break;                                                               \
-      case CacheStorageSchedulerOp::kDelete:                                 \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK2(                                  \
-            uma_type, (histogram_name ".Delete", ##__VA_ARGS__));            \
-        break;                                                               \
-      case CacheStorageSchedulerOp::kGetAllMatched:                          \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK2(                                  \
-            uma_type, (histogram_name ".GetAllMatched", ##__VA_ARGS__));     \
-        break;                                                               \
-      case CacheStorageSchedulerOp::kHas:                                    \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK2(                                  \
-            uma_type, (histogram_name ".Has", ##__VA_ARGS__));               \
-        break;                                                               \
-      case CacheStorageSchedulerOp::kInit:                                   \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK2(                                  \
-            uma_type, (histogram_name ".Init", ##__VA_ARGS__));              \
-        break;                                                               \
-      case CacheStorageSchedulerOp::kKeys:                                   \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK2(                                  \
-            uma_type, (histogram_name ".Keys", ##__VA_ARGS__));              \
-        break;                                                               \
-      case CacheStorageSchedulerOp::kMatch:                                  \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK2(                                  \
-            uma_type, (histogram_name ".Match", ##__VA_ARGS__));             \
-        break;                                                               \
-      case CacheStorageSchedulerOp::kMatchAll:                               \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK2(                                  \
-            uma_type, (histogram_name ".MatchAll", ##__VA_ARGS__));          \
-        break;                                                               \
-      case CacheStorageSchedulerOp::kOpen:                                   \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK2(                                  \
-            uma_type, (histogram_name ".Open", ##__VA_ARGS__));              \
-        break;                                                               \
-      case CacheStorageSchedulerOp::kPut:                                    \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK2(                                  \
-            uma_type, (histogram_name ".Put", ##__VA_ARGS__));               \
-        break;                                                               \
-      case CacheStorageSchedulerOp::kSize:                                   \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK2(                                  \
-            uma_type, (histogram_name ".Size", ##__VA_ARGS__));              \
-        break;                                                               \
-      case CacheStorageSchedulerOp::kSizeThenClose:                          \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK2(                                  \
-            uma_type, (histogram_name ".SizeThenClose", ##__VA_ARGS__));     \
-        break;                                                               \
-      case CacheStorageSchedulerOp::kTest:                                   \
-        /* do not record op-specific histograms for test ops */              \
-        break;                                                               \
-      case CacheStorageSchedulerOp::kWriteIndex:                             \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK2(                                  \
-            uma_type, (histogram_name ".WriteIndex", ##__VA_ARGS__));        \
-        break;                                                               \
-      case CacheStorageSchedulerOp::kWriteSideData:                          \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK2(                                  \
-            uma_type, (histogram_name ".WriteSideData", ##__VA_ARGS__));     \
-        break;                                                               \
-    }                                                                        \
-  } while (0)
+// The following functions are used to record UMA histograms for the
+// scheduler.  There are two functions to handle the different argument types
+// without triggering template code bloat.
+void RecordCacheStorageSchedulerUMA(CacheStorageSchedulerUMA uma_type,
+                                    CacheStorageSchedulerClient client_type,
+                                    CacheStorageSchedulerOp op_type,
+                                    int value);
 
-#define CACHE_STORAGE_SCHEDULER_UMA(uma_type, uma_name, client_type, op_type,  \
-                                    ...)                                       \
-  /* Only the Cache and CacheStorage clients should have specific op types */  \
-  DCHECK(client_type != CacheStorageSchedulerClient::kBackgroundSync ||        \
-         op_type == CacheStorageSchedulerOp::kBackgroundSync);                 \
-  do {                                                                         \
-    switch (client_type) {                                                     \
-      case CacheStorageSchedulerClient::kStorage:                              \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK(                                     \
-            uma_type, op_type,                                                 \
-            "ServiceWorkerCache.CacheStorage.Scheduler." uma_name,             \
-            ##__VA_ARGS__);                                                    \
-        break;                                                                 \
-      case CacheStorageSchedulerClient::kCache:                                \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK(                                     \
-            uma_type, op_type, "ServiceWorkerCache.Cache.Scheduler." uma_name, \
-            ##__VA_ARGS__);                                                    \
-        break;                                                                 \
-      case CacheStorageSchedulerClient::kBackgroundSync:                       \
-        CACHE_STORAGE_SCHEDULER_UMA_THUNK(                                     \
-            uma_type, op_type,                                                 \
-            "ServiceWorkerCache.BackgroundSyncManager.Scheduler." uma_name,    \
-            ##__VA_ARGS__);                                                    \
-        break;                                                                 \
-    }                                                                          \
-  } while (0)
+void RecordCacheStorageSchedulerUMA(CacheStorageSchedulerUMA uma_type,
+                                    CacheStorageSchedulerClient client_type,
+                                    CacheStorageSchedulerOp op_type,
+                                    base::TimeDelta value);
 
 }  // namespace content
 

content/browser/cache_storage/cache_storage_operation.cc

@@ -25,11 +25,13 @@ CacheStorageOperation::CacheStorageOperation(
       weak_ptr_factory_(this) {}
 
 CacheStorageOperation::~CacheStorageOperation() {
-  CACHE_STORAGE_SCHEDULER_UMA(LONG_TIMES, "OperationDuration2", client_type_,
-                              op_type_, base::TimeTicks::Now() - start_ticks_);
+  RecordCacheStorageSchedulerUMA(CacheStorageSchedulerUMA::kOperationDuration,
+                                 client_type_, op_type_,
+                                 base::TimeTicks::Now() - start_ticks_);
 
   if (!was_slow_)
-    RecordOperationSlowness();
+    RecordCacheStorageSchedulerUMA(CacheStorageSchedulerUMA::kIsOperationSlow,
+                                   client_type_, op_type_, was_slow_);
 }
 
 void CacheStorageOperation::Run() {
@@ -45,13 +47,8 @@ void CacheStorageOperation::Run() {
 
 void CacheStorageOperation::NotifyOperationSlow() {
   was_slow_ = true;
-  RecordOperationSlowness();
-}
-
-void CacheStorageOperation::RecordOperationSlowness() {
-  // Wrap the UMA macro in a method to reduce code bloat.
-  CACHE_STORAGE_SCHEDULER_UMA(BOOLEAN, "IsOperationSlow", client_type_,
-                              op_type_, was_slow_);
+  RecordCacheStorageSchedulerUMA(CacheStorageSchedulerUMA::kIsOperationSlow,
+                                 client_type_, op_type_, was_slow_);
 }
 
 }  // namespace content

content/browser/cache_storage/cache_storage_operation.h

@@ -39,7 +39,6 @@ class CONTENT_EXPORT CacheStorageOperation {
 
  private:
   void NotifyOperationSlow();
-  void RecordOperationSlowness();
 
   // The operation's closure to run.
   base::OnceClosure closure_;

content/browser/cache_storage/cache_storage_scheduler.cc

@@ -25,8 +25,9 @@ CacheStorageScheduler::~CacheStorageScheduler() {}
 
 void CacheStorageScheduler::ScheduleOperation(CacheStorageSchedulerOp op_type,
                                               base::OnceClosure closure) {
-  CACHE_STORAGE_SCHEDULER_UMA(COUNTS_10000, "QueueLength", client_type_,
-                              op_type, pending_operations_.size());
+  RecordCacheStorageSchedulerUMA(CacheStorageSchedulerUMA::kQueueLength,
+                                 client_type_, op_type,
+                                 pending_operations_.size());
 
   pending_operations_.push_back(std::make_unique<CacheStorageOperation>(
       std::move(closure), client_type_, op_type,
@@ -51,8 +52,8 @@ void CacheStorageScheduler::RunOperationIfIdle() {
     running_operation_ = std::move(pending_operations_.front());
     pending_operations_.pop_front();
 
-    CACHE_STORAGE_SCHEDULER_UMA(
-        LONG_TIMES, "QueueDuration2", client_type_,
+    RecordCacheStorageSchedulerUMA(
+        CacheStorageSchedulerUMA::kQueueDuration, client_type_,
         running_operation_->op_type(),
         base::TimeTicks::Now() - running_operation_->creation_ticks());