Bug 1894582 - Update the conditions for when to gather memory telemetry r=mccr8

* Rename DelayedInit() to Poke() to communicate that it can be called
   multiple times.  The new DelayedInit() is much smaller and only relevant
   to postpone gathering telemetry until after process startup.

 * Remove the observer callbacks that responded to cycle collection, replace
   them with a timer that is created after a task is executed.

 * Setup the memory telemetry timer regardless of extended telemetry

   Previously the memory telemetry was collected periodically only when
   extended telemetry was enabled, and if it wasn't enabled it was collected
   once (or more if DelayedInit() was called more than once, and it is).
   This could lead to different biases in the data between release and
   nightly.

 * Don't collect telemetry directly in Poke(), always use the
   timer.

 * Don't setup the timer if it would do nothing.

   If release telemetry isn't enabled then the only thing collected is
   MEMORY_TOTAL and that's only collected on the parent process.  So don't
   setup the timer on content processes.

Note that this is not a change in whether data is collected or not, since
GatherReports() always ran at least once before this change.  Furthermore
different settings change whether the telemetry is actually sent or not.

Differential Revision: https://phabricator.services.mozilla.com/D209201

js/xpconnect/src/XPCJSContext.cpp

@@ -25,6 +25,7 @@
 #include "nsPrintfCString.h"
 #include "mozilla/Preferences.h"
 #include "mozilla/Telemetry.h"
+#include "mozilla/MemoryTelemetry.h"
 #include "mozilla/Services.h"
 #ifdef FUZZING
 #  include "mozilla/StaticPrefs_fuzzing.h"
@@ -1433,6 +1434,11 @@ void XPCJSContext::AfterProcessTask(uint32_t aNewRecursionDepth) {
   nsJSContext::MaybePokeCC();
   CycleCollectedJSContext::AfterProcessTask(aNewRecursionDepth);
 
+  // Poke the memory telemetry reporter
+  if (AppShutdown::GetCurrentShutdownPhase() == ShutdownPhase::NotInShutdown) {
+    MemoryTelemetry::Get().Poke();
+  }
+
   // This exception might have been set if we called an XPCWrappedJS that threw,
   // but now we're returning to the event loop, so nothing is going to look at
   // this value again. Clear it to prevent leaks.

xpcom/base/MemoryTelemetry.cpp

@@ -42,10 +42,12 @@ using mozilla::dom::AutoJSAPI;
 using mozilla::dom::ContentParent;
 
 // Do not gather data more than once a minute (ms)
-static constexpr uint32_t kTelemetryInterval = 60 * 1000;
+static constexpr uint32_t kTelemetryIntervalMS = 60 * 1000;
+
+// Do not create a timer for telemetry this many seconds after the previous one
+// fires.  This exists so that we don't respond to our own timer.
+static constexpr uint32_t kTelemetryCooldownS = 10;
 
-static constexpr const char* kTopicCycleCollectorBegin =
-    "cycle-collector-begin";
 static constexpr const char* kTopicShutdown = "content-child-shutdown";
 
 namespace {
@@ -119,24 +121,65 @@ void MemoryTelemetry::Init() {
   return *sInstance;
 }
 
-nsresult MemoryTelemetry::DelayedInit() {
-  if (Telemetry::CanRecordExtended()) {
-    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
-    MOZ_RELEASE_ASSERT(obs);
+void MemoryTelemetry::DelayedInit() {
+  mCanRun = true;
+  Poke();
+}
 
-    obs->AddObserver(this, kTopicCycleCollectorBegin, true);
+void MemoryTelemetry::Poke() {
+  // Don't do anything that might delay process startup
+  if (!mCanRun) {
+    return;
   }
 
-  GatherReports();
+  if (XRE_IsContentProcess() && !Telemetry::CanRecordReleaseData()) {
+    // All memory telemetry produced by content processes is release data, so if
+    // we're not recording release data then don't setup the timers on content
+    // processes.
+    return;
+  }
 
-  return NS_OK;
+  TimeStamp now = TimeStamp::Now();
+
+  if (mLastRun && mLastRun + TimeDuration::FromSeconds(10) < now) {
+    // If we last gathered telemetry less than ten seconds ago then Poke() does
+    // nothing.  This is to prevent our own timer waking us up.
+    return;
+  }
+
+  mLastPoke = now;
+  if (!mTimer) {
+    uint32_t delay = kTelemetryIntervalMS;
+    if (mLastRun) {
+      delay = uint32_t(
+          std::min(
+              TimeDuration::FromMilliseconds(kTelemetryIntervalMS),
+              std::max(TimeDuration::FromSeconds(kTelemetryCooldownS),
+                       TimeDuration::FromMilliseconds(kTelemetryIntervalMS) -
+                           (now - mLastRun)))
+              .ToMilliseconds());
+    }
+    RefPtr<MemoryTelemetry> self(this);
+    auto res = NS_NewTimerWithCallback(
+        [self](nsITimer* aTimer) { self->GatherReports(); }, delay,
+        nsITimer::TYPE_ONE_SHOT_LOW_PRIORITY, "MemoryTelemetry::GatherReports");
+
+    if (res.isOk()) {
+      // Errors are ignored, if there was an error then we just don't get
+      // telemetry.
+      mTimer = res.unwrap();
+    }
+  }
 }
 
 nsresult MemoryTelemetry::Shutdown() {
+  if (mTimer) {
+    mTimer->Cancel();
+  }
+
   nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
   MOZ_RELEASE_ASSERT(obs);
 
-  obs->RemoveObserver(this, kTopicCycleCollectorBegin);
   obs->RemoveObserver(this, kTopicShutdown);
 
   return NS_OK;
@@ -201,6 +244,9 @@ nsresult MemoryTelemetry::GatherReports(
     }
   });
 
+  mLastRun = TimeStamp::Now();
+  mTimer = nullptr;
+
   RefPtr<nsMemoryReporterManager> mgr = nsMemoryReporterManager::GetOrCreate();
   MOZ_DIAGNOSTIC_ASSERT(mgr);
   NS_ENSURE_TRUE(mgr, NS_ERROR_FAILURE);
@@ -504,21 +550,7 @@ nsresult MemoryTelemetry::FinishGatheringTotalMemory(
 
 nsresult MemoryTelemetry::Observe(nsISupports* aSubject, const char* aTopic,
                                   const char16_t* aData) {
-  if (strcmp(aTopic, kTopicCycleCollectorBegin) == 0) {
-    auto now = TimeStamp::Now();
-    if (!mLastPoll.IsNull() &&
-        (now - mLastPoll).ToMilliseconds() < kTelemetryInterval) {
-      return NS_OK;
-    }
-
-    mLastPoll = now;
-
-    NS_DispatchToCurrentThreadQueue(
-        NewRunnableMethod<std::function<void()>>(
-            "MemoryTelemetry::GatherReports", this,
-            &MemoryTelemetry::GatherReports, nullptr),
-        EventQueuePriority::Idle);
-  } else if (strcmp(aTopic, kTopicShutdown) == 0) {
+  if (strcmp(aTopic, kTopicShutdown) == 0) {
     if (nsCOMPtr<nsITelemetry> telemetry =
             do_GetService("@mozilla.org/base/telemetry;1")) {
       telemetry->FlushBatchedChildTelemetry();

xpcom/base/MemoryTelemetry.h

@@ -40,10 +40,14 @@ class MemoryTelemetry final : public nsIObserver,
       const std::function<void()>& aCompletionCallback = nullptr);
 
   /**
-   * Does expensive initialization, which should happen only after startup has
-   * completed, and the event loop is idle.
+   * Called to signal that we can begin collecting telemetry.
    */
-  nsresult DelayedInit();
+  void DelayedInit();
+
+  /**
+   * Notify that the browser is active and telemetry should be recorded soon.
+   */
+  void Poke();
 
   nsresult Shutdown();
 
@@ -64,7 +68,12 @@ class MemoryTelemetry final : public nsIObserver,
 
   bool mGatheringTotalMemory = false;
 
-  TimeStamp mLastPoll{};
+  TimeStamp mLastRun{};
+  TimeStamp mLastPoke{};
+  nsCOMPtr<nsITimer> mTimer;
+
+  // True if startup is finished and it's okay to start gathering telemetry.
+  bool mCanRun = false;
 };
 
 }  // namespace mozilla