bug 1373240 - Use somewhat-more-dynamically-allocated histogram storage r?gfritzsche draft
authorChris H-C <chutten@mozilla.com>
Tue, 29 Aug 2017 15:05:35 -0400
changeset 661503 ab2b4a8fb3e97680cf694403c39335b176de9b39
parent 660208 93dd2e456c0ecca00fb4d28744e88078a77deaf7
child 730598 1427cdc1f3b924243e0c07ba1ad006255d61dfb0
push id78789
push userbmo:chutten@mozilla.com
push dateFri, 08 Sep 2017 15:37:06 +0000
reviewersgfritzsche
bugs1373240
milestone57.0a1
bug 1373240 - Use somewhat-more-dynamically-allocated histogram storage r?gfritzsche Switch from static multi-dimensional arrays to dynamic one-dimensional arrays that are only allocated in the parent process. MozReview-Commit-ID: tyGEFhU2Fq
toolkit/components/telemetry/TelemetryHistogram.cpp
--- a/toolkit/components/telemetry/TelemetryHistogram.cpp
+++ b/toolkit/components/telemetry/TelemetryHistogram.cpp
@@ -24,16 +24,18 @@
 
 #include "TelemetryCommon.h"
 #include "TelemetryHistogram.h"
 #include "TelemetryScalar.h"
 #include "ipc/TelemetryIPCAccumulator.h"
 
 #include "base/histogram.h"
 
+#include <limits>
+
 using base::Histogram;
 using base::BooleanHistogram;
 using base::CountHistogram;
 using base::FlagHistogram;
 using base::LinearHistogram;
 using mozilla::StaticMutex;
 using mozilla::StaticMutexAutoLock;
 using mozilla::Telemetry::Accumulation;
@@ -191,20 +193,20 @@ bool gInitDone = false;
 
 // Whether we are collecting the base, opt-out, Histogram data.
 bool gCanRecordBase = false;
 // Whether we are collecting the extended, opt-in, Histogram data.
 bool gCanRecordExtended = false;
 
 // The storage for actual Histogram instances.
 // We use separate ones for plain and keyed histograms.
-Histogram* gHistogramStorage[HistogramCount][uint32_t(ProcessID::Count)][uint32_t(SessionType::Count)] = {};
+Histogram** gHistogramStorage;
 // Keyed histograms internally map string keys to individual Histogram instances.
 // KeyedHistogram keeps track of session & subsession histograms internally.
-KeyedHistogram* gKeyedHistogramStorage[HistogramCount][uint32_t(ProcessID::Count)] = {};
+KeyedHistogram** gKeyedHistogramStorage;
 
 // Cache of histogram name to a histogram id.
 StringToHistogramIdMap gNameToHistogramIDMap(HistogramCount);
 
 // To simplify logic below we use a single histogram instance for all expired histograms.
 Histogram* gExpiredHistogram = nullptr;
 
 // This tracks whether recording is enabled for specific histograms.
@@ -238,16 +240,79 @@ const HistogramID kRecordingInitiallyDis
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
 //
 // The core storage access functions.
 // They wrap access to the histogram storage and lookup caches.
 
 namespace {
 
+size_t internal_KeyedHistogramStorageIndex(HistogramID aHistogramId,
+                                           ProcessID aProcessId)
+{
+  return aHistogramId * size_t(ProcessID::Count) + size_t(aProcessId);
+}
+
+size_t internal_HistogramStorageIndex(HistogramID aHistogramId,
+                                      ProcessID aProcessId,
+                                      SessionType aSessionType)
+{
+  static_assert(
+    HistogramCount <
+      std::numeric_limits<size_t>::max() / size_t(ProcessID::Count) / size_t(SessionType::Count),
+        "Too many histograms, processes, and session types to store in a 1D "
+        "array.");
+
+  return aHistogramId * size_t(ProcessID::Count) * size_t(SessionType::Count) +
+         size_t(aProcessId) * size_t(SessionType::Count) +
+         size_t(aSessionType);
+}
+
+Histogram* internal_GetHistogramFromStorage(HistogramID aHistogramId,
+                                            ProcessID aProcessId,
+                                            SessionType aSessionType)
+{
+  size_t index = internal_HistogramStorageIndex(aHistogramId, aProcessId, aSessionType);
+  return gHistogramStorage[index];
+}
+
+void internal_SetHistogramInStorage(HistogramID aHistogramId,
+                                    ProcessID aProcessId,
+                                    SessionType aSessionType,
+                                    Histogram* aHistogram)
+{
+  MOZ_ASSERT(XRE_IsParentProcess(),
+    "Histograms are stored only in the parent process.");
+
+  size_t index = internal_HistogramStorageIndex(aHistogramId, aProcessId, aSessionType);
+  MOZ_ASSERT(!gHistogramStorage[index],
+    "Mustn't overwrite storage without clearing it first.");
+  gHistogramStorage[index] = aHistogram;
+}
+
+KeyedHistogram* internal_GetKeyedHistogramFromStorage(HistogramID aHistogramId,
+                                                      ProcessID aProcessId)
+{
+  size_t index = internal_KeyedHistogramStorageIndex(aHistogramId, aProcessId);
+  return gKeyedHistogramStorage[index];
+}
+
+void internal_SetKeyedHistogramInStorage(HistogramID aHistogramId,
+                                         ProcessID aProcessId,
+                                         KeyedHistogram* aKeyedHistogram)
+{
+  MOZ_ASSERT(XRE_IsParentProcess(),
+    "Keyed Histograms are stored only in the parent process.");
+
+  size_t index = internal_KeyedHistogramStorageIndex(aHistogramId, aProcessId);
+  MOZ_ASSERT(!gKeyedHistogramStorage[index],
+    "Mustn't overwrite storage without clearing it first");
+  gKeyedHistogramStorage[index] = aKeyedHistogram;
+}
+
 // Factory function for histogram instances.
 Histogram*
 internal_CreateHistogramInstance(const HistogramInfo& info, int bucketsOffset);
 
 bool
 internal_IsHistogramEnumId(HistogramID aID)
 {
   static_assert(((HistogramID)-1 > 0), "ID should be unsigned.");
@@ -259,46 +324,49 @@ Histogram*
 internal_GetHistogramById(HistogramID histogramId, ProcessID processId, SessionType sessionType,
                           bool instantiate = true)
 {
   MOZ_ASSERT(internal_IsHistogramEnumId(histogramId));
   MOZ_ASSERT(!gHistogramInfos[histogramId].keyed);
   MOZ_ASSERT(processId < ProcessID::Count);
   MOZ_ASSERT(sessionType < SessionType::Count);
 
-  Histogram* h = gHistogramStorage[histogramId][uint32_t(processId)][uint32_t(sessionType)];
+  Histogram* h = internal_GetHistogramFromStorage(histogramId,
+                                                  processId,
+                                                  sessionType);
   if (h || !instantiate) {
     return h;
   }
 
   const HistogramInfo& info = gHistogramInfos[histogramId];
   const int bucketsOffset = gExponentialBucketLowerBoundIndex[histogramId];
   h = internal_CreateHistogramInstance(info, bucketsOffset);
   MOZ_ASSERT(h);
-  gHistogramStorage[histogramId][uint32_t(processId)][uint32_t(sessionType)] = h;
+  internal_SetHistogramInStorage(histogramId, processId, sessionType, h);
   return h;
 }
 
 // Look up a keyed histogram by id.
 KeyedHistogram*
 internal_GetKeyedHistogramById(HistogramID histogramId, ProcessID processId,
                                bool instantiate = true)
 {
   MOZ_ASSERT(internal_IsHistogramEnumId(histogramId));
   MOZ_ASSERT(gHistogramInfos[histogramId].keyed);
   MOZ_ASSERT(processId < ProcessID::Count);
 
-  KeyedHistogram* kh = gKeyedHistogramStorage[histogramId][uint32_t(processId)];
+  KeyedHistogram* kh = internal_GetKeyedHistogramFromStorage(histogramId,
+                                                             processId);
   if (kh || !instantiate) {
     return kh;
   }
 
   const HistogramInfo& info = gHistogramInfos[histogramId];
   kh = new KeyedHistogram(histogramId, info);
-  gKeyedHistogramStorage[histogramId][uint32_t(processId)] = kh;
+  internal_SetKeyedHistogramInStorage(histogramId, processId, kh);
 
   return kh;
 }
 
 // Look up a histogram id from a histogram name.
 nsresult
 internal_GetHistogramIdByName(const nsACString& name, HistogramID* id)
 {
@@ -309,18 +377,19 @@ internal_GetHistogramIdByName(const nsAC
 
   return NS_OK;
 }
 
 // Clear a histogram from storage.
 void
 internal_ClearHistogramById(HistogramID histogramId, ProcessID processId, SessionType sessionType)
 {
-  delete gHistogramStorage[histogramId][uint32_t(processId)][uint32_t(sessionType)];
-  gHistogramStorage[histogramId][uint32_t(processId)][uint32_t(sessionType)] = nullptr;
+  size_t index = internal_HistogramStorageIndex(histogramId, processId, sessionType);
+  delete gHistogramStorage[index];
+  gHistogramStorage[index] = nullptr;
 }
 
 }
 
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
 //
 // PRIVATE: Misc small helpers
@@ -1626,16 +1695,23 @@ void TelemetryHistogram::InitializeGloba
 {
   StaticMutexAutoLock locker(gTelemetryHistogramMutex);
   MOZ_ASSERT(!gInitDone, "TelemetryHistogram::InitializeGlobalState "
              "may only be called once");
 
   gCanRecordBase = canRecordBase;
   gCanRecordExtended = canRecordExtended;
 
+  if (XRE_IsParentProcess()) {
+    gHistogramStorage =
+      new Histogram*[HistogramCount * size_t(ProcessID::Count) * size_t(SessionType::Count)] {};
+    gKeyedHistogramStorage =
+      new KeyedHistogram*[HistogramCount * size_t(ProcessID::Count)] {};
+  }
+
   // gNameToHistogramIDMap should have been pre-sized correctly at the
   // declaration point further up in this file.
 
   // Populate the static histogram name->id cache.
   // Note that the histogram names are statically allocated.
   for (uint32_t i = 0; i < HistogramCount; i++) {
     gNameToHistogramIDMap.Put(nsDependentCString(gHistogramInfos[i].name()), HistogramID(i));
   }
@@ -1672,29 +1748,27 @@ void TelemetryHistogram::DeInitializeGlo
 {
   StaticMutexAutoLock locker(gTelemetryHistogramMutex);
   gCanRecordBase = false;
   gCanRecordExtended = false;
   gNameToHistogramIDMap.Clear();
   gInitDone = false;
 
   // FactoryGet `new`s Histograms for us, but requires us to manually delete.
-  for (size_t i = 0; i < HistogramCount; ++i) {
-    for (uint32_t process = 0; process < static_cast<uint32_t>(ProcessID::Count); ++process) {
-      delete gKeyedHistogramStorage[i][process];
-      gKeyedHistogramStorage[i][process] = nullptr;
-      for (uint32_t session = 0; session <
-        static_cast<uint32_t>(SessionType::Count); ++session) {
-        if (gHistogramStorage[i][process][session] == gExpiredHistogram) {
-          continue;
-        }
-        delete gHistogramStorage[i][process][session];
-        gHistogramStorage[i][process][session] = nullptr;
+  if (XRE_IsParentProcess()) {
+    for (size_t i = 0; i < HistogramCount * size_t(ProcessID::Count) * size_t(SessionType::Count); ++i) {
+      if (i < HistogramCount * size_t(ProcessID::Count)) {
+        delete gKeyedHistogramStorage[i];
+      }
+      if (gHistogramStorage[i] != gExpiredHistogram) {
+        delete gHistogramStorage[i];
       }
     }
+    delete[] gHistogramStorage;
+    delete[] gKeyedHistogramStorage;
   }
   delete gExpiredHistogram;
   gExpiredHistogram = nullptr;
 }
 
 #ifdef DEBUG
 bool TelemetryHistogram::GlobalStateHasBeenInitialized() {
   StaticMutexAutoLock locker(gTelemetryHistogramMutex);