Bug 1598992 - Stop Base Profiler before starting Gecko Profiler - r=gregtatum
authorGerald Squelart <gsquelart@mozilla.com>
Tue, 26 Nov 2019 22:59:58 +0000
changeset 503929 909fd03696099bb3e5d16a6107cb56af7d01eaab
parent 503928 6aa8b2b48eb426d119bdf3ea85c189cc501c9c7c
child 503930 bf4148a5bc47be85cd64c34d88e3ed33c3f4b32a
push id36850
push userdvarga@mozilla.com
push dateWed, 27 Nov 2019 04:47:48 +0000
treeherdermozilla-central@07a4c42fecf6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgregtatum
bugs1598992
milestone72.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 1598992 - Stop Base Profiler before starting Gecko Profiler - r=gregtatum This ensures that no more Base Profiler (BP) activity can happen when Gecko Profiler (GP) starts. In particular on Linux this is needed because the BP sampler sends signals to stop threads, and the just-starting GP could receive this signal before it is fully ready to handle it with its own sampler. Differential Revision: https://phabricator.services.mozilla.com/D54443
tools/profiler/core/platform.cpp
--- a/tools/profiler/core/platform.cpp
+++ b/tools/profiler/core/platform.cpp
@@ -4020,16 +4020,43 @@ static void locked_profiler_start(PSLock
 
     for (uint32_t i = 0; i < aFilterCount; i++) {
       LOG("- threads  = %s", aFilters[i]);
     }
   }
 
   MOZ_RELEASE_ASSERT(CorePS::Exists() && !ActivePS::Exists(aLock));
 
+#ifdef MOZ_BASE_PROFILER
+  UniquePtr<char[]> baseprofile;
+  if (baseprofiler::profiler_is_active()) {
+    // Note that we still hold the lock, so the sampler cannot run yet and
+    // interact negatively with the still-active BaseProfiler sampler.
+    // Assume that Base Profiler is active because of MOZ_BASE_PROFILER_STARTUP.
+    // Capture the Base Profiler startup profile threads (if any).
+    baseprofile = baseprofiler::profiler_get_profile(
+        /* aSinceTime */ 0, /* aIsShuttingDown */ false,
+        /* aOnlyThreads */ true);
+
+    // Now stop Base Profiler (BP), as further recording will be ignored anyway,
+    // and so that it won't clash with Gecko Profiler (GP) sampling starting
+    // after the lock is dropped.
+    // On Linux this is especially important to do before creating the GP
+    // sampler, because the BP sampler may send a signal (to stop threads to be
+    // sampled), which the GP would intercept before its own initialization is
+    // complete and ready to handle such signals.
+    // Note that even though `profiler_stop()` doesn't immediately destroy and
+    // join the sampler thread, it safely deactivates it in such a way that the
+    // thread will soon exit without doing any actual work.
+    // TODO: Allow non-sampling profiling to continue.
+    // TODO: Re-start BP after GP shutdown, to capture post-XPCOM shutdown.
+    baseprofiler::profiler_stop();
+  }
+#endif
+
 #if defined(GP_PLAT_amd64_windows)
   InitializeWin64ProfilerHooks();
 #endif
 
   // Fall back to the default values if the passed-in values are unreasonable.
   // Less than 8192 entries (65536 bytes) may not be enough for the most complex
   // stack, so we should be able to store at least one full stack.
   // TODO: Review magic numbers.
@@ -4044,41 +4071,23 @@ static void locked_profiler_start(PSLock
 
   ActivePS::Create(aLock, capacity, interval, aFeatures, aFilters, aFilterCount,
                    duration);
 
   // ActivePS::Create can only succeed or crash.
   MOZ_ASSERT(ActivePS::Exists(aLock));
 
 #ifdef MOZ_BASE_PROFILER
-  if (baseprofiler::profiler_is_active()) {
-    // Note that we still hold the lock, so the sampler cannot run yet and
-    // interact negatively with the still-active BaseProfiler sampler.
-    // Assume that BaseProfiler is active because of MOZ_BASE_PROFILER_STARTUP.
-    // Capture the BaseProfiler startup profile threads (if any).
-    UniquePtr<char[]> baseprofile = baseprofiler::profiler_get_profile(
-        /* aSinceTime */ 0, /* aIsShuttingDown */ false,
-        /* aOnlyThreads */ true);
-
-    // Now stop BaseProfiler, as further recording will be ignored anyway, and
-    // so that it won't clash with Gecko Profiler sampling starting after the
-    // lock is dropped.
-    // TODO: Allow non-sampling profiling to continue.
-    // TODO: Re-start BaseProfiler after Gecko Profiler shutdown, to capture
-    // post-XPCOM shutdown.
-    baseprofiler::profiler_stop();
-
-    // An "empty" profile string may in fact contain 1 character (a newline),
-    // so we want at least 2 characters to register a profile.
-    if (HasMinimumLength(baseprofile.get(), 2)) {
-      // The BaseProfiler startup profile will be stored as a separate "process"
-      // in the Gecko Profiler profile, and shown as a new track under the
-      // corresponding Gecko Profiler thread.
-      ActivePS::AddBaseProfileThreads(aLock, std::move(baseprofile));
-    }
+  // An "empty" profile string may in fact contain 1 character (a newline), so
+  // we want at least 2 characters to register a profile.
+  if (HasMinimumLength(baseprofile.get(), 2)) {
+    // The BaseProfiler startup profile will be stored as a separate "process"
+    // in the Gecko Profiler profile, and shown as a new track under the
+    // corresponding Gecko Profiler thread.
+    ActivePS::AddBaseProfileThreads(aLock, std::move(baseprofile));
   }
 #endif
 
   // Set up profiling for each registered thread, if appropriate.
   Maybe<int> mainThreadId;
   int tid = profiler_current_thread_id();
   const Vector<UniquePtr<RegisteredThread>>& registeredThreads =
       CorePS::RegisteredThreads(aLock);