Bug 1369612 - Remove ProfilerStackFrameDynamicRAII and profiler_call_{enter,exit}. r=mstange.
authorNicholas Nethercote <nnethercote@mozilla.com>
Fri, 02 Jun 2017 15:38:20 +1000
changeset 412942 e3cf285f484a04e665f1f42d9df50de0c65dc1ea
parent 412941 a325d838180b07692bbb0e04094ad5e1dc06cca9
child 412943 cac26305ac53144a8fad330cc27a1a0e66931c6f
push id1490
push usermtabara@mozilla.com
push dateMon, 31 Jul 2017 14:08:16 +0000
treeherdermozilla-release@70e32e6bf15e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmstange
bugs1369612
milestone55.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 1369612 - Remove ProfilerStackFrameDynamicRAII and profiler_call_{enter,exit}. r=mstange. ProfilerStackFrameRAII and ProfilerStackFrameDynamicRAII are very similar; the latter lets a dynamic string be specified as well (and lacks the MOZ_GUARD_OBJECT stuff, for no good reason). This patch does the following. - Removes ProfilerStackFrameDynamicRAII, and adds a dynamic string to ProfilerStackFrameRAII. It also reorders the constructor's arguments to match the field ordering of ProfileEntry. There aren't many usage sites so these changes don't affect many places. - With that done, there is only a single callsite for each of profiler_call_enter() and profiler_call_exit(), so the patch also inlines and removes them.
dom/base/nsJSUtils.cpp
tools/profiler/core/platform.cpp
tools/profiler/public/GeckoProfiler.h
tools/profiler/tests/gtest/GeckoProfiler.cpp
--- a/dom/base/nsJSUtils.cpp
+++ b/dom/base/nsJSUtils.cpp
@@ -131,18 +131,18 @@ EvaluationExceptionToNSResult(JSContext*
   }
   return NS_SUCCESS_DOM_SCRIPT_EVALUATION_THREW_UNCATCHABLE;
 }
 
 nsJSUtils::ExecutionContext::ExecutionContext(JSContext* aCx,
                                               JS::Handle<JSObject*> aGlobal)
   :
 #ifdef MOZ_GECKO_PROFILER
-    mProfilerRAII("nsJSUtils::ExecutionContext", /* PROFILER_LABEL */
-                  js::ProfileEntry::Category::JS, __LINE__),
+    mProfilerRAII("nsJSUtils::ExecutionContext", /* dynamicStr */ nullptr,
+                  __LINE__, js::ProfileEntry::Category::JS),
 #endif
     mCx(aCx)
   , mCompartment(aCx, aGlobal)
   , mRetValue(aCx)
   , mScopeChain(aCx)
   , mRv(NS_OK)
   , mSkip(false)
   , mCoerceToString(false)
--- a/tools/profiler/core/platform.cpp
+++ b/tools/profiler/core/platform.cpp
@@ -581,26 +581,26 @@ private:
 };
 
 MOZ_THREAD_LOCAL(ThreadInfo*) TLSInfo::sThreadInfo;
 
 // Although you can access a thread's PseudoStack via TLSInfo::sThreadInfo, we
 // also have a second TLS pointer directly to the PseudoStack. Here's why.
 //
 // - We need to be able to push to and pop from the PseudoStack in
-//   profiler_call_{enter,exit}.
+//   ProfilerStackFrameRAII.
 //
 // - Those two functions are hot and must be defined in GeckoProfiler.h so they
 //   can be inlined.
 //
 // - We don't want to expose TLSInfo (and ThreadInfo) in GeckoProfiler.h.
 //
 // This second pointer isn't ideal, but does provide a way to satisfy those
 // constraints. TLSInfo manages it, except for the uses in
-// profiler_call_{enter,exit}.
+// ProfilerStackFrameRAII.
 MOZ_THREAD_LOCAL(PseudoStack*) sPseudoStack;
 
 // The name of the main thread.
 static const char* const kMainThreadName = "GeckoMain";
 
 ////////////////////////////////////////////////////////////////////////
 // BEGIN tick/unwinding code
 
--- a/tools/profiler/public/GeckoProfiler.h
+++ b/tools/profiler/public/GeckoProfiler.h
@@ -93,17 +93,18 @@ using UniqueProfilerBacktrace =
 // profile buffer. So there's one string copy operation, and it happens at
 // sample time.
 // Compare this to the plain PROFILER_LABEL macro, which only accepts literal
 // strings: When the pseudo stack frames generated by PROFILER_LABEL are
 // sampled, no string copy needs to be made because the profile buffer can
 // just store the raw pointers to the literal strings. Consequently,
 // PROFILER_LABEL frames take up considerably less space in the profile buffer
 // than PROFILER_LABEL_DYNAMIC frames.
-#define PROFILER_LABEL_DYNAMIC(name_space, info, category, str) do {} while (0)
+#define PROFILER_LABEL_DYNAMIC(name_space, info, category, dynamicStr) \
+  do {} while (0)
 
 // Insert a marker in the profile timeline. This is useful to delimit something
 // important happening such as the first paint. Unlike profiler_label that are
 // only recorded if a sample is collected while it is active, marker will always
 // be collected.
 #define PROFILER_MARKER(marker_name) do {} while (0)
 #define PROFILER_MARKER_PAYLOAD(marker_name, payload) \
   do { \
@@ -123,30 +124,30 @@ using UniqueProfilerBacktrace =
 #define PROFILER_FUNC_VOID(decl) void decl;
 
 // we want the class and function name but can't easily get that using preprocessor macros
 // __func__ doesn't have the class name and __PRETTY_FUNCTION__ has the parameters
 
 #define PROFILER_LABEL(name_space, info, category) \
   PROFILER_PLATFORM_TRACING(name_space "::" info) \
   mozilla::ProfilerStackFrameRAII \
-  PROFILER_APPEND_LINE_NUMBER(profiler_raii)(name_space "::" info, category, \
-                                             __LINE__)
+  PROFILER_APPEND_LINE_NUMBER(profiler_raii)(name_space "::" info, nullptr, \
+                                             __LINE__, category)
 
 #define PROFILER_LABEL_FUNC(category) \
   PROFILER_PLATFORM_TRACING(PROFILER_FUNCTION_NAME) \
   mozilla::ProfilerStackFrameRAII \
-  PROFILER_APPEND_LINE_NUMBER(profiler_raii)(PROFILER_FUNCTION_NAME, category, \
-                                             __LINE__)
+  PROFILER_APPEND_LINE_NUMBER(profiler_raii)(PROFILER_FUNCTION_NAME, nullptr, \
+                                             __LINE__, category)
 
-#define PROFILER_LABEL_DYNAMIC(name_space, info, category, str) \
+#define PROFILER_LABEL_DYNAMIC(name_space, info, category, dynamicStr) \
   PROFILER_PLATFORM_TRACING(name_space "::" info) \
-  mozilla::ProfilerStackFrameDynamicRAII \
-  PROFILER_APPEND_LINE_NUMBER(profiler_raii)(name_space "::" info, category, \
-                                             __LINE__, str)
+  mozilla::ProfilerStackFrameRAII \
+  PROFILER_APPEND_LINE_NUMBER(profiler_raii)(name_space "::" info, dynamicStr, \
+                                             __LINE__, category)
 
 #define PROFILER_MARKER(marker_name) profiler_add_marker(marker_name)
 #define PROFILER_MARKER_PAYLOAD(marker_name, payload) \
   profiler_add_marker(marker_name, payload)
 
 #endif  // defined(MOZ_GECKO_PROFILER)
 
 // Higher-order macro containing all the feature info in one place. Define
@@ -400,61 +401,20 @@ PROFILER_FUNC(void* profiler_get_stack_t
 // Make sure that we can use std::min here without the Windows headers messing with us.
 #ifdef min
 # undef min
 #endif
 
 class nsISupports;
 class ProfilerMarkerPayload;
 
-// This exists purely for profiler_call_{enter,exit}. See the comment on the
+// This exists purely for ProfilerStackFrameRAII. See the comment on the
 // definition in platform.cpp for details.
 extern MOZ_THREAD_LOCAL(PseudoStack*) sPseudoStack;
 
-// Returns a handle to pass on exit. This can check that we are popping the
-// correct callstack. Operates the same whether the profiler is active or not.
-//
-// A short-lived, non-owning PseudoStack reference is created between each
-// profiler_call_enter() / profiler_call_exit() call pair. RAII objects (e.g.
-// ProfilerStackFrameRAII) ensure that these calls are balanced. Furthermore,
-// the RAII objects exist within the thread itself, which means they are
-// necessarily bounded by the lifetime of the thread, which ensures that the
-// references held can't be used after the PseudoStack is destroyed.
-inline void*
-profiler_call_enter(const char* aLabel, js::ProfileEntry::Category aCategory,
-                    void* aFrameAddress, uint32_t aLine,
-                    const char* aDynamicString = nullptr)
-{
-  // This function runs both on and off the main thread.
-
-  PseudoStack* pseudoStack = sPseudoStack.get();
-  if (!pseudoStack) {
-    return pseudoStack;
-  }
-  pseudoStack->pushCppFrame(aLabel, aDynamicString, aFrameAddress, aLine,
-                            js::ProfileEntry::Kind::CPP_NORMAL, aCategory);
-
-  // The handle is meant to support future changes but for now it is simply
-  // used to avoid having to call TLSInfo::RacyInfo() in profiler_call_exit().
-  return pseudoStack;
-}
-
-inline void
-profiler_call_exit(void* aHandle)
-{
-  // This function runs both on and off the main thread.
-
-  if (!aHandle) {
-    return;
-  }
-
-  PseudoStack* pseudoStack = static_cast<PseudoStack*>(aHandle);
-  pseudoStack->pop();
-}
-
 // Adds a marker to the PseudoStack. A no-op if the profiler is inactive or in
 // privacy mode.
 void profiler_add_marker(const char* aMarkerName,
                          ProfilerMarkerPayload* aPayload = nullptr);
 
 #define PROFILER_APPEND_LINE_NUMBER_PASTE(id, line) id ## line
 #define PROFILER_APPEND_LINE_NUMBER_EXPAND(id, line) \
   PROFILER_APPEND_LINE_NUMBER_PASTE(id, line)
@@ -502,51 +462,51 @@ void profiler_add_marker(const char* aMa
 // In the case of profiler_get_backtrace we know that we only need enough space
 // for a single backtrace.
 #define PROFILER_GET_BACKTRACE_ENTRIES 1000
 
 #define PROFILER_DEFAULT_INTERVAL 1
 
 namespace mozilla {
 
+// This class creates a non-owning PseudoStack reference. Objects of this class
+// are stack-allocated, and so exist within a thread, and are thus bounded by
+// the lifetime of the thread, which ensures that the references held can't be
+// used after the PseudoStack is destroyed.
 class MOZ_RAII ProfilerStackFrameRAII {
 public:
-  // We only copy the strings at save time, so to take multiple parameters we'd
-  // need to copy them then.
-  ProfilerStackFrameRAII(const char* aLabel,
-    js::ProfileEntry::Category aCategory, uint32_t aLine
-    MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
+  ProfilerStackFrameRAII(const char* aLabel, const char* aDynamicString,
+                         uint32_t aLine, js::ProfileEntry::Category aCategory
+                         MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
   {
     MOZ_GUARD_OBJECT_NOTIFIER_INIT;
-    mHandle = profiler_call_enter(aLabel, aCategory, this, aLine);
-  }
-  ~ProfilerStackFrameRAII() {
-    profiler_call_exit(mHandle);
-  }
-private:
-  MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
-  void* mHandle;
-};
+
+    // This function runs both on and off the main thread.
 
-class MOZ_RAII ProfilerStackFrameDynamicRAII {
-public:
-  ProfilerStackFrameDynamicRAII(const char* aLabel,
-    js::ProfileEntry::Category aCategory, uint32_t aLine,
-    const char* aDynamicString)
-  {
-    mHandle = profiler_call_enter(aLabel, aCategory, this, aLine,
-                                  aDynamicString);
+    mPseudoStack = sPseudoStack.get();
+    if (mPseudoStack) {
+      mPseudoStack->pushCppFrame(aLabel, aDynamicString, this, aLine,
+                                js::ProfileEntry::Kind::CPP_NORMAL, aCategory);
+    }
   }
 
-  ~ProfilerStackFrameDynamicRAII() {
-    profiler_call_exit(mHandle);
+  ~ProfilerStackFrameRAII()
+  {
+    // This function runs both on and off the main thread.
+
+    if (mPseudoStack) {
+      mPseudoStack->pop();
+    }
   }
 
 private:
-  void* mHandle;
+  MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
+  // We save a PseudoStack pointer in the ctor so we don't have to redo the TLS
+  // lookup in the dtor.
+  PseudoStack* mPseudoStack;
 };
 
 } // namespace mozilla
 
 PseudoStack* profiler_get_pseudo_stack();
 
 void profiler_set_js_context(JSContext* aCx);
 void profiler_clear_js_context();
--- a/tools/profiler/tests/gtest/GeckoProfiler.cpp
+++ b/tools/profiler/tests/gtest/GeckoProfiler.cpp
@@ -578,25 +578,21 @@ TEST(GeckoProfiler, PseudoStack)
 
     profiler_start(PROFILER_DEFAULT_ENTRIES, PROFILER_DEFAULT_INTERVAL,
                    features, filters, MOZ_ARRAY_LENGTH(filters));
 
     ASSERT_TRUE(profiler_get_backtrace());
   }
 
 #if defined(MOZ_GECKO_PROFILER)
-  ProfilerStackFrameRAII raii1("A", js::ProfileEntry::Category::STORAGE, 888);
-  ProfilerStackFrameDynamicRAII raii2("A", js::ProfileEntry::Category::STORAGE,
-                                      888, dynamic.get());
-  void* handle = profiler_call_enter("A", js::ProfileEntry::Category::NETWORK,
-                                     this, 999);
+  ProfilerStackFrameRAII raii1("A", nullptr, 888,
+                               js::ProfileEntry::Category::STORAGE);
+  ProfilerStackFrameRAII raii2("A", dynamic.get(), 888,
+                               js::ProfileEntry::Category::NETWORK);
   ASSERT_TRUE(profiler_get_backtrace());
-  profiler_call_exit(handle);
-
-  profiler_call_exit(nullptr);  // a no-op
 #endif
 
   profiler_stop();
 
   ASSERT_TRUE(!profiler_get_profile());
 }
 
 TEST(GeckoProfiler, Bug1355807)