Bug 1340928 (part 7) - Factor out gIsActive handling in platform-*.cpp. r=mstange.
authorNicholas Nethercote <nnethercote@mozilla.com>
Thu, 16 Feb 2017 15:08:07 +1100
changeset 373479 6e4aa11ecb4d6873c113abf48f0694c9da8150d1
parent 373478 0a33bae98aee8fec29505ebc66d186e9b7331462
child 373480 8ac2f3a144257a917ac118499f54c6e18862bd21
push id10863
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 23:02:23 +0000
treeherdermozilla-aurora@0931190cd725 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmstange
bugs1340928
milestone54.0a1
Bug 1340928 (part 7) - Factor out gIsActive handling in platform-*.cpp. r=mstange. PlatformStart() and PlatformStop() are currently responsible for setting and clearing gIsActive, but it's better if we do it in profiler_{start,stop}(). The patch also does the following. - Adds some missing emacs/vim modelines. - Makes Platform{Start,Stop}() crash if they have failures. I'm not at all confident that ignoring the errors as is currently done will result in sensible behaviour, so brittleness is better.
tools/profiler/core/platform-linux-android.cpp
tools/profiler/core/platform-macos.cpp
tools/profiler/core/platform-win32.cpp
tools/profiler/core/platform.cpp
--- a/tools/profiler/core/platform-linux-android.cpp
+++ b/tools/profiler/core/platform-linux-android.cpp
@@ -1,8 +1,10 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
 // Copyright (c) 2006-2011 The Chromium Authors. All rights reserved.
 //
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions
 // are met:
 //  * Redistributions of source code must retain the above copyright
 //    notice, this list of conditions and the following disclaimer.
 //  * Redistributions in binary form must reproduce the above copyright
@@ -23,19 +25,16 @@
 // OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
 // AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
 // OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
 // OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
 // SUCH DAMAGE.
 
 // This file is used for both Linux and Android.
 
-/*
-# vim: sw=2
-*/
 #include <stdio.h>
 #include <math.h>
 
 #include <pthread.h>
 #include <semaphore.h>
 #include <signal.h>
 #include <sys/time.h>
 #include <sys/resource.h>
@@ -69,24 +68,22 @@
 // Memory profile
 #include "nsMemoryReporterManager.h"
 
 #include <string.h>
 #include <list>
 
 using namespace mozilla;
 
-// All accesses to these two variables are on the main thread, so no locking is
+// All accesses to this variable are on the main thread, so no locking is
 // needed.
-static bool gIsSigprofHandlerInstalled;
 static struct sigaction gOldSigprofHandler;
 
-// All accesses to these two variables are on the main thread, so no locking is
+// All accesses to this variable are on the main thread, so no locking is
 // needed.
-static bool gHasSigprofSenderLaunched;
 static pthread_t gSigprofSenderThread;
 
 #if defined(USE_LUL_STACKWALK)
 // A singleton instance of the library.  It is initialised at first
 // use.  Currently only the main thread can call PlatformStart(), so
 // there is no need for a mechanism to ensure that it is only
 // created once in a multi-thread-use situation.
 lul::LUL* gLUL = nullptr;
@@ -376,79 +373,64 @@ PlatformStart(double aInterval)
     gIntervalMicro = 1;
   }
 
   // Initialize signal handler communication
   gCurrentThreadInfo = nullptr;
   gRssMemory = 0;
   gUssMemory = 0;
   if (sem_init(&gSignalHandlingDone, /* pshared: */ 0, /* value: */ 0) != 0) {
-    LOG("Error initializing semaphore");
-    return;
+    MOZ_CRASH("Error initializing semaphore");
   }
 
   // Request profiling signals.
   LOG("Request signal");
   struct sigaction sa;
   sa.sa_sigaction = MOZ_SIGNAL_TRAMPOLINE(SigprofHandler);
   sigemptyset(&sa.sa_mask);
   sa.sa_flags = SA_RESTART | SA_SIGINFO;
   if (sigaction(SIGPROF, &sa, &gOldSigprofHandler) != 0) {
-    LOG("Error installing signal");
-    return;
+    MOZ_CRASH("Error installing signal");
   }
   LOG("Signal installed");
-  gIsSigprofHandlerInstalled = true;
 
 #if defined(USE_LUL_STACKWALK)
   // Switch into unwind mode.  After this point, we can't add or
   // remove any unwind info to/from this LUL instance.  The only thing
   // we can do with it is Unwind() calls.
   gLUL->EnableUnwinding();
 
   // Has a test been requested?
   if (PR_GetEnv("MOZ_PROFILER_LUL_TEST")) {
      int nTests = 0, nTestsPassed = 0;
      RunLulUnitTests(&nTests, &nTestsPassed, gLUL);
   }
 #endif
 
-  // Start a thread that sends SIGPROF signal to VM thread.
-  // Sending the signal ourselves instead of relying on itimer provides
-  // much better accuracy.
-  MOZ_ASSERT(!gIsActive);
-  gIsActive = true;
-  if (pthread_create(&gSigprofSenderThread, NULL, SigprofSender, NULL) == 0) {
-    gHasSigprofSenderLaunched = true;
+  // Start a thread that sends SIGPROF signal to VM thread. Sending the signal
+  // ourselves instead of relying on itimer provides much better accuracy.
+  if (pthread_create(&gSigprofSenderThread, NULL, SigprofSender, NULL) != 0) {
+    MOZ_CRASH("pthread_create failed");
   }
   LOG("Profiler thread started");
 }
 
 static void
 PlatformStop()
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
-  MOZ_ASSERT(gIsActive);
-  gIsActive = false;
-
   gIntervalMicro = 0;
 
-  // Wait for signal sender termination (it will exit after setting
-  // active_ to false).
-  if (gHasSigprofSenderLaunched) {
-    pthread_join(gSigprofSenderThread, NULL);
-    gHasSigprofSenderLaunched = false;
-  }
+  // Wait for SigprofSender() termination (SigprofSender() will exit because
+  // gIsActive has been set to false).
+  pthread_join(gSigprofSenderThread, NULL);
 
   // Restore old signal handler
-  if (gIsSigprofHandlerInstalled) {
-    sigaction(SIGPROF, &gOldSigprofHandler, 0);
-    gIsSigprofHandlerInstalled = false;
-  }
+  sigaction(SIGPROF, &gOldSigprofHandler, 0);
 }
 
 #if defined(GP_OS_android)
 static struct sigaction gOldSigstartHandler;
 const int SIGSTART = SIGUSR2;
 
 static void freeArray(const char** array, int size) {
   for (int i = 0; i < size; i++) {
--- a/tools/profiler/core/platform-macos.cpp
+++ b/tools/profiler/core/platform-macos.cpp
@@ -1,8 +1,10 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include <dlfcn.h>
 #include <unistd.h>
 #include <sys/mman.h>
 #include <mach/mach_init.h>
@@ -107,17 +109,19 @@ public:
     SetThreadName();
     MOZ_ASSERT(thread->mThread != kNoThread);
     thread->Run();
     return NULL;
   }
 
   void Start() {
     pthread_attr_t* attr_ptr = NULL;
-    pthread_create(&mThread, attr_ptr, ThreadEntry, this);
+    if (pthread_create(&mThread, attr_ptr, ThreadEntry, this) != 0) {
+      MOZ_CRASH("pthread_create failed");
+    }
     MOZ_ASSERT(mThread != kNoThread);
   }
 
   void Join() {
     pthread_join(mThread, NULL);
   }
 
   static void StartSampler(double aInterval) {
@@ -260,28 +264,24 @@ PlatformInit()
 {
 }
 
 static void
 PlatformStart(double aInterval)
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
-  MOZ_ASSERT(!gIsActive);
-  gIsActive = true;
   SamplerThread::StartSampler(aInterval);
 }
 
 static void
 PlatformStop()
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
-  MOZ_ASSERT(gIsActive);
-  gIsActive = false;
   SamplerThread::StopSampler();
 }
 
 /* static */ Thread::tid_t
 Thread::GetCurrentId()
 {
   return gettid();
 }
--- a/tools/profiler/core/platform-win32.cpp
+++ b/tools/profiler/core/platform-win32.cpp
@@ -1,8 +1,10 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
 // Copyright (c) 2006-2011 The Chromium Authors. All rights reserved.
 //
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions
 // are met:
 //  * Redistributions of source code must retain the above copyright
 //    notice, this list of conditions and the following disclaimer.
 //  * Redistributions in binary form must reproduce the above copyright
@@ -115,16 +117,19 @@ class SamplerThread
   void Start() {
     mThread = reinterpret_cast<HANDLE>(
         _beginthreadex(NULL,
                        /* stack_size */ 0,
                        ThreadEntry,
                        this,
                        /* initflag */ 0,
                        (unsigned int*) &mThreadId));
+    if (mThread == 0) {
+      MOZ_CRASH("_beginthreadex failed");
+    }
   }
 
   void Join() {
     if (mThreadId != Thread::GetCurrentId()) {
       WaitForSingleObject(mThread, INFINITE);
     }
   }
 
@@ -271,28 +276,24 @@ PlatformInit()
 {
 }
 
 static void
 PlatformStart(double aInterval)
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
-  MOZ_ASSERT(!gIsActive);
-  gIsActive = true;
   SamplerThread::StartSampler(aInterval);
 }
 
 static void
 PlatformStop()
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
-  MOZ_ASSERT(gIsActive);
-  gIsActive = false;
   SamplerThread::StopSampler();
 }
 
 /* static */ Thread::tid_t
 Thread::GetCurrentId()
 {
   return GetCurrentThreadId();
 }
--- a/tools/profiler/core/platform.cpp
+++ b/tools/profiler/core/platform.cpp
@@ -2030,19 +2030,19 @@ profiler_start(int aProfileEntries, doub
 #ifdef MOZ_TASK_TRACER
   if (gTaskTracer) {
     mozilla::tasktracer::StartLogging();
   }
 #endif
 
   gGatherer = new mozilla::ProfileGatherer();
 
-  MOZ_ASSERT(!gIsActive && !gIsPaused);
+  gIsActive = true;  // Must set this for PlatformStart() to work.
+  gIsPaused = false;
   PlatformStart(gInterval);
-  MOZ_ASSERT(gIsActive && !gIsPaused);  // PlatformStart() sets gIsActive.
 
   if (gProfileJS || privacyMode) {
     mozilla::StaticMutexAutoLock lock(gRegisteredThreadsMutex);
 
     for (uint32_t i = 0; i < gRegisteredThreads->size(); i++) {
       ThreadInfo* info = (*gRegisteredThreads)[i];
       if (info->IsPendingDelete() || !info->hasProfile()) {
         continue;
@@ -2122,18 +2122,19 @@ profiler_stop()
   bool disableJS = gProfileJS;
 
   {
     StaticMutexAutoLock lock(gThreadNameFiltersMutex);
     gThreadNameFilters.clear();
   }
   gFeatures.clear();
 
+  gIsActive = false;  // Must clear this for PlatformStop() to work.
+  gIsPaused = false;
   PlatformStop();
-  MOZ_ASSERT(!gIsActive && !gIsPaused);   // PlatformStop() clears these.
 
   gProfileJava      = false;
   gProfileJS        = false;
   gTaskTracer       = false;
 
   gAddLeafAddresses = false;
   gDisplayListDump  = false;
   gLayersDump       = false;