bug 1373240 - Use somewhat-more-dynamically-allocated histogram storage r=Dexter,gfritzsche
☠☠ backed out by a8d468af2cd3 ☠ ☠
authorChris H-C <chutten@mozilla.com>
Tue, 29 Aug 2017 15:05:35 -0400
changeset 429205 0181bf16af4f45ddab48bf2e7d3b0410a17c0d08
parent 429204 39a94e16a3694c443a985081ac1c6cd761363da2
child 429206 629ecdd561c4ac05052de7a0703dc282fbb411c6
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersDexter, gfritzsche
bugs1373240
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1373240 - Use somewhat-more-dynamically-allocated histogram storage r=Dexter,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
@@ -750,36 +819,22 @@ KeyedHistogram::Add(const nsCString& key
 void
 KeyedHistogram::Clear(bool onlySubsession)
 {
   MOZ_ASSERT(XRE_IsParentProcess());
   if (!XRE_IsParentProcess()) {
     return;
   }
 #if !defined(MOZ_WIDGET_ANDROID)
-  for (auto iter = mSubsessionMap.Iter(); !iter.Done(); iter.Next()) {
-    Histogram* h = iter.Get()->mData;
-    if (h == gExpiredHistogram) {
-      continue;
-    }
-    delete h;
-  }
   mSubsessionMap.Clear();
   if (onlySubsession) {
     return;
   }
 #endif
 
-  for (auto iter = mHistogramMap.Iter(); !iter.Done(); iter.Next()) {
-    Histogram* h = iter.Get()->mData;
-    if (h == gExpiredHistogram) {
-      continue;
-    }
-    delete h;
-  }
   mHistogramMap.Clear();
 }
 
 nsresult
 KeyedHistogram::GetJSKeys(JSContext* cx, JS::CallArgs& args)
 {
   JS::AutoValueVector keys(cx);
   if (!keys.reserve(mHistogramMap.Count())) {
@@ -1626,16 +1681,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 +1734,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);