Bug 1151597 - Step 2: Don't start child process memory reports until parent is finished. r=erahm
authorJed Davis <jld@mozilla.com>
Mon, 27 Apr 2015 15:46:00 -0400
changeset 271288 338486ce551e2c00125229c1eca5b577d6f9edb5
parent 271287 57ffca487e0add9ccbba416758f8206cc8ae398e
child 271289 e654d5d726e9a108d7d70afc02983a18cb6994f8
push id4830
push userjlund@mozilla.com
push dateMon, 29 Jun 2015 20:18:48 +0000
treeherdermozilla-beta@4c2175bb0420 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerserahm
bugs1151597
milestone40.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 1151597 - Step 2: Don't start child process memory reports until parent is finished. r=erahm
xpcom/base/nsMemoryReporterManager.cpp
xpcom/base/nsMemoryReporterManager.h
--- a/xpcom/base/nsMemoryReporterManager.cpp
+++ b/xpcom/base/nsMemoryReporterManager.cpp
@@ -1335,92 +1335,99 @@ nsMemoryReporterManager::GetReportsExten
     MEMORY_REPORTING_LOG("GetReports (gen=%u, s->gen=%u): abort\n",
                          generation, mGetReportsState->mGeneration);
     return NS_OK;
   }
 
   MEMORY_REPORTING_LOG("GetReports (gen=%u, %d child(ren) present)\n",
                        generation, mNumChildProcesses);
 
-  if (mNumChildProcesses > 0) {
-    // Request memory reports from child processes.  We do this *before*
-    // collecting reports for this process so each process can collect
-    // reports in parallel.
-    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
-    NS_ENSURE_STATE(obs);
-
-    nsPrintfCString genStr("generation=%x anonymize=%d minimize=%d DMDident=",
-                           generation, aAnonymize ? 1 : 0, aMinimize ? 1 : 0);
-    nsAutoString msg = NS_ConvertUTF8toUTF16(genStr);
-    msg += aDMDDumpIdent;
-
-    obs->NotifyObservers(nullptr, "child-memory-reporter-request",
-                         msg.get());
-
-    nsCOMPtr<nsITimer> timer = do_CreateInstance(NS_TIMER_CONTRACTID);
-    NS_ENSURE_TRUE(timer, NS_ERROR_FAILURE);
-    rv = timer->InitWithFuncCallback(TimeoutCallback,
-                                     this, kTimeoutLengthMS,
-                                     nsITimer::TYPE_ONE_SHOT);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    mGetReportsState = new GetReportsState(generation,
-                                           aAnonymize,
-                                           timer,
-                                           mNumChildProcesses,
-                                           aHandleReport,
-                                           aHandleReportData,
-                                           aFinishReporting,
-                                           aFinishReportingData,
-                                           aDMDDumpIdent);
-  } else {
-    mGetReportsState = new GetReportsState(generation,
-                                           aAnonymize,
-                                           nullptr,
-                                           /* mNumChildProcesses = */ 0,
-                                           aHandleReport,
-                                           aHandleReportData,
-                                           aFinishReporting,
-                                           aFinishReportingData,
-                                           aDMDDumpIdent);
-  }
+  mGetReportsState = new GetReportsState(generation,
+                                         aAnonymize,
+                                         aMinimize,
+                                         mNumChildProcesses,
+                                         aHandleReport,
+                                         aHandleReportData,
+                                         aFinishReporting,
+                                         aFinishReportingData,
+                                         aDMDDumpIdent);
 
   if (aMinimize) {
     rv = MinimizeMemoryUsage(NS_NewRunnableMethod(
       this, &nsMemoryReporterManager::StartGettingReports));
   } else {
     rv = StartGettingReports();
   }
   return rv;
 }
 
 nsresult
 nsMemoryReporterManager::StartGettingReports()
 {
   GetReportsState* s = mGetReportsState;
+  nsresult rv;
 
   // Get reports for this process.
   FILE* parentDMDFile = nullptr;
 #ifdef MOZ_DMD
   if (!s->mDMDDumpIdent.IsEmpty()) {
-    nsresult rv = nsMemoryInfoDumper::OpenDMDFile(s->mDMDDumpIdent, getpid(),
+    rv = nsMemoryInfoDumper::OpenDMDFile(s->mDMDDumpIdent, getpid(),
                                                   &parentDMDFile);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       // Proceed with the memory report as if DMD were disabled.
       parentDMDFile = nullptr;
     }
   }
 #endif
   GetReportsForThisProcessExtended(s->mHandleReport, s->mHandleReportData,
                                    s->mAnonymize, parentDMDFile);
-  s->mParentDone = true;
+
+  MOZ_ASSERT(s->mNumChildProcessesCompleted == 0);
+  if (s->mNumChildProcesses > 0) {
+    // Request memory reports from child processes.  This happens
+    // after the parent report so that the parent's main thread will
+    // be free to process the child reports, instead of causing them
+    // to be buffered and consume (possibly scarce) memory.
+    nsCOMPtr<nsITimer> timer = do_CreateInstance(NS_TIMER_CONTRACTID);
+    // Don't use NS_ENSURE_* here; can't return until the report is finished.
+    if (NS_WARN_IF(!timer)) {
+      FinishReporting();
+      return NS_ERROR_FAILURE;
+    }
+    rv = timer->InitWithFuncCallback(TimeoutCallback,
+                                     this, kTimeoutLengthMS,
+                                     nsITimer::TYPE_ONE_SHOT);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      FinishReporting();
+      return rv;
+    }
 
-  // If there are no remaining child processes, we can finish up immediately.
-  return (s->mNumChildProcessesCompleted >= s->mNumChildProcesses) ?
-    FinishReporting() : NS_OK;
+    MOZ_ASSERT(!s->mTimer);
+    s->mTimer.swap(timer);
+
+    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
+    if (NS_WARN_IF(!obs)) {
+      FinishReporting();
+      return NS_ERROR_UNEXPECTED;
+    }
+
+    nsPrintfCString genStr("generation=%x anonymize=%d minimize=%d DMDident=",
+                           s->mGeneration, s->mAnonymize ? 1 : 0,
+                           s->mMinimize ? 1 : 0);
+    nsAutoString msg = NS_ConvertUTF8toUTF16(genStr);
+    msg += s->mDMDDumpIdent;
+
+    obs->NotifyObservers(nullptr, "child-memory-reporter-request",
+                         msg.get());
+
+    return NS_OK;
+  } else {
+    // If there are no child processes, we can finish up immediately.
+    return FinishReporting();
+  }
 }
 
 typedef nsCOMArray<nsIMemoryReporter> MemoryReporterArray;
 
 static PLDHashOperator
 StrongEnumerator(nsRefPtrHashKey<nsIMemoryReporter>* aElem, void* aData)
 {
   MemoryReporterArray* allReporters = static_cast<MemoryReporterArray*>(aData);
@@ -1570,45 +1577,38 @@ nsMemoryReporterManager::EndChildReport(
     // (Also, we don't have the child's identifier at this point.)
     MEMORY_REPORTING_LOG("HandleChildReports (aGen=%u): child %d exited"
                          " during report\n",
                          aGeneration, s->mNumChildProcessesCompleted);
   }
 
   // If all the child processes have reported, we can cancel the timer and
   // finish up.  Otherwise, just return.
-  if (s->mNumChildProcessesCompleted >= s->mNumChildProcesses &&
-      s->mParentDone) {
+  if (s->mNumChildProcessesCompleted >= s->mNumChildProcesses) {
     s->mTimer->Cancel();
     FinishReporting();
   }
 }
 
 /* static */ void
 nsMemoryReporterManager::TimeoutCallback(nsITimer* aTimer, void* aData)
 {
   nsMemoryReporterManager* mgr = static_cast<nsMemoryReporterManager*>(aData);
   GetReportsState* s = mgr->mGetReportsState;
 
-  MOZ_ASSERT(mgr->mGetReportsState);
+  // Release assert because: if the pointer is null we're about to
+  // crash regardless of DEBUG, and this way the compiler doesn't
+  // complain about unused variables.
+  MOZ_RELEASE_ASSERT(s, "mgr->mGetReportsState");
   MEMORY_REPORTING_LOG("TimeoutCallback (s->gen=%u)\n",
                        s->mGeneration);
 
   // We don't bother sending any kind of cancellation message to the child
   // processes that haven't reported back.
-
-  if (s->mParentDone) {
-    mgr->FinishReporting();
-  } else {
-    // This is unlikely -- the timeout expired during MinimizeMemoryUsage.
-    MEMORY_REPORTING_LOG("Timeout expired before parent report started!");
-    // Let the parent continue with its report, but ensure that
-    // StartGettingReports gives up immediately after that.
-    s->mNumChildProcesses = s->mNumChildProcessesCompleted;
-  }
+  mgr->FinishReporting();
 }
 
 nsresult
 nsMemoryReporterManager::FinishReporting()
 {
   // Memory reporting only happens on the main thread.
   if (!NS_IsMainThread()) {
     MOZ_CRASH();
--- a/xpcom/base/nsMemoryReporterManager.h
+++ b/xpcom/base/nsMemoryReporterManager.h
@@ -201,39 +201,38 @@ private:
 
   uint32_t mNumChildProcesses;
   uint32_t mNextGeneration;
 
   struct GetReportsState
   {
     uint32_t                             mGeneration;
     bool                                 mAnonymize;
+    bool                                 mMinimize;
     nsCOMPtr<nsITimer>                   mTimer;
     uint32_t                             mNumChildProcesses;
     uint32_t                             mNumChildProcessesCompleted;
-    bool                                 mParentDone;
     nsCOMPtr<nsIHandleReportCallback>    mHandleReport;
     nsCOMPtr<nsISupports>                mHandleReportData;
     nsCOMPtr<nsIFinishReportingCallback> mFinishReporting;
     nsCOMPtr<nsISupports>                mFinishReportingData;
     nsString                             mDMDDumpIdent;
 
-    GetReportsState(uint32_t aGeneration, bool aAnonymize, nsITimer* aTimer,
+    GetReportsState(uint32_t aGeneration, bool aAnonymize, bool aMinimize,
                     uint32_t aNumChildProcesses,
                     nsIHandleReportCallback* aHandleReport,
                     nsISupports* aHandleReportData,
                     nsIFinishReportingCallback* aFinishReporting,
                     nsISupports* aFinishReportingData,
                     const nsAString& aDMDDumpIdent)
       : mGeneration(aGeneration)
       , mAnonymize(aAnonymize)
-      , mTimer(aTimer)
+      , mMinimize(aMinimize)
       , mNumChildProcesses(aNumChildProcesses)
       , mNumChildProcessesCompleted(0)
-      , mParentDone(false)
       , mHandleReport(aHandleReport)
       , mHandleReportData(aHandleReportData)
       , mFinishReporting(aFinishReporting)
       , mFinishReportingData(aFinishReportingData)
       , mDMDDumpIdent(aDMDDumpIdent)
     {
     }
   };