Bug 1386751 - Split CollectCodeLocation into CollectWasmFrame and CollectPseudoEntry, r=njn

MozReview-Commit-ID: 4LNuJQtm95K

tools/profiler/core/ProfileBuffer.cpp

@@ -7,6 +7,9 @@
 #include "ProfileBuffer.h"
 
 #include "ProfilerMarker.h"
+#include "jsfriendapi.h"
+#include "nsScriptSecurityManager.h"
+#include "nsJSPrincipals.h"
 
 using namespace mozilla;
 
@@ -64,18 +67,6 @@ ProfileBuffer::AddStoredMarker(ProfilerMarker *aStoredMarker)
   mStoredMarkers.insert(aStoredMarker);
 }
 
-void
-ProfileBuffer::CollectNativeLeafAddr(void* aAddr)
-{
-  AddEntry(ProfileBufferEntry::NativeLeafAddr(aAddr));
-}
-
-void
-ProfileBuffer::CollectJitReturnAddr(void* aAddr)
-{
-  AddEntry(ProfileBufferEntry::JitReturnAddr(aAddr));
-}
-
 void
 ProfileBuffer::CollectCodeLocation(
   const char* aLabel, const char* aStr, int aLineNumber,
@@ -142,3 +133,95 @@ ProfileBuffer::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
   return n;
 }
 
+/* ProfileBufferCollector */
+
+static bool
+IsChromeJSScript(JSScript* aScript)
+{
+  // WARNING: this function runs within the profiler's "critical section".
+
+  nsIScriptSecurityManager* const secman =
+    nsScriptSecurityManager::GetScriptSecurityManager();
+  NS_ENSURE_TRUE(secman, false);
+
+  JSPrincipals* const principals = JS_GetScriptPrincipals(aScript);
+  return secman->IsSystemPrincipal(nsJSPrincipals::get(principals));
+}
+
+Maybe<uint32_t>
+ProfileBufferCollector::Generation()
+{
+  return Some(mBuf.mGeneration);
+}
+
+void
+ProfileBufferCollector::CollectNativeLeafAddr(void* aAddr)
+{
+  mBuf.AddEntry(ProfileBufferEntry::NativeLeafAddr(aAddr));
+}
+
+void
+ProfileBufferCollector::CollectJitReturnAddr(void* aAddr)
+{
+  mBuf.AddEntry(ProfileBufferEntry::JitReturnAddr(aAddr));
+}
+
+void
+ProfileBufferCollector::CollectWasmFrame(const char* aLabel)
+{
+  mBuf.CollectCodeLocation("", aLabel, -1, Nothing());
+}
+
+void
+ProfileBufferCollector::CollectPseudoEntry(const js::ProfileEntry& aEntry)
+{
+  // WARNING: this function runs within the profiler's "critical section".
+
+  MOZ_ASSERT(aEntry.kind() == js::ProfileEntry::Kind::CPP_NORMAL ||
+             aEntry.kind() == js::ProfileEntry::Kind::JS_NORMAL);
+
+  const char* label = aEntry.label();
+  const char* dynamicString = aEntry.dynamicString();
+  bool isChromeJSEntry = false;
+  int lineno = -1;
+
+  if (aEntry.isJs()) {
+    // There are two kinds of JS frames that get pushed onto the PseudoStack.
+    //
+    // - label = "", dynamic string = <something>
+    // - label = "js::RunScript", dynamic string = nullptr
+    //
+    // The line number is only interesting in the first case.
+
+    if (label[0] == '\0') {
+      MOZ_ASSERT(dynamicString);
+
+      // We call aEntry.script() repeatedly -- rather than storing the result in
+      // a local variable in order -- to avoid rooting hazards.
+      if (aEntry.script()) {
+        isChromeJSEntry = IsChromeJSScript(aEntry.script());
+        if (aEntry.pc()) {
+          lineno = JS_PCToLineNumber(aEntry.script(), aEntry.pc());
+        }
+      }
+
+    } else {
+      MOZ_ASSERT(strcmp(label, "js::RunScript") == 0 && !dynamicString);
+    }
+  } else {
+    MOZ_ASSERT(aEntry.isCpp());
+    lineno = aEntry.line();
+  }
+
+  if (dynamicString) {
+    // Adjust the dynamic string as necessary.
+    if (ProfilerFeature::HasPrivacy(mFeatures) && !isChromeJSEntry) {
+      dynamicString = "(private)";
+    } else if (strlen(dynamicString) >= ProfileBuffer::kMaxFrameKeyLength) {
+      dynamicString = "(too long)";
+    }
+  }
+
+  mBuf.CollectCodeLocation(label, dynamicString, lineno,
+                           Some(aEntry.category()));
+}

tools/profiler/core/ProfileBuffer.h

@@ -13,7 +13,7 @@
 #include "mozilla/RefPtr.h"
 #include "mozilla/RefCounted.h"
 
-class ProfileBuffer final : public ProfilerStackCollector
+class ProfileBuffer final
 {
 public:
   explicit ProfileBuffer(int aEntrySize);
@@ -42,16 +42,9 @@ public:
   // record the resulting generation and index in |aLS| if it's non-null.
   void AddThreadIdEntry(int aThreadId, LastSample* aLS = nullptr);
 
-  virtual mozilla::Maybe<uint32_t> Generation() override
-  {
-    return mozilla::Some(mGeneration);
-  }
-
-  virtual void CollectNativeLeafAddr(void* aAddr) override;
-  virtual void CollectJitReturnAddr(void* aAddr) override;
-  virtual void CollectCodeLocation(
+  void CollectCodeLocation(
     const char* aLabel, const char* aStr, int aLineNumber,
-    const mozilla::Maybe<js::ProfileEntry::Category>& aCategory) override;
+    const mozilla::Maybe<js::ProfileEntry::Category>& aCategory);
 
   // Maximum size of a frameKey string that we'll handle.
   static const size_t kMaxFrameKeyLength = 512;
@@ -105,4 +98,29 @@ public:
   ProfilerMarkerLinkedList mStoredMarkers;
 };
 
+/**
+ * Helper type used to implement ProfilerStackCollector. This type is used as
+ * the collector for MergeStacks by ProfileBuffer. It holds a reference to the
+ * buffer, as well as additional feature flags which are needed to control the
+ * data collection strategy
+ */
+class ProfileBufferCollector final : public ProfilerStackCollector
+{
+public:
+  ProfileBufferCollector(ProfileBuffer& aBuf, uint32_t aFeatures)
+    : mBuf(aBuf)
+    , mFeatures(aFeatures)
+  {}
+
+  virtual mozilla::Maybe<uint32_t> Generation() override;
+  virtual void CollectNativeLeafAddr(void* aAddr) override;
+  virtual void CollectJitReturnAddr(void* aAddr) override;
+  virtual void CollectWasmFrame(const char* aLabel) override;
+  virtual void CollectPseudoEntry(const js::ProfileEntry& aEntry) override;
+
+private:
+  ProfileBuffer& mBuf;
+  uint32_t mFeatures;
+};
+
 #endif

tools/profiler/core/platform.cpp

@@ -716,80 +716,6 @@ public:
 #endif
 };
 
-static bool
-IsChromeJSScript(JSScript* aScript)
-{
-  // WARNING: this function runs within the profiler's "critical section".
-
-  nsIScriptSecurityManager* const secman =
-    nsScriptSecurityManager::GetScriptSecurityManager();
-  NS_ENSURE_TRUE(secman, false);
-
-  JSPrincipals* const principals = JS_GetScriptPrincipals(aScript);
-  return secman->IsSystemPrincipal(nsJSPrincipals::get(principals));
-}
-
-static void
-AddPseudoEntry(uint32_t aFeatures, NotNull<RacyThreadInfo*> aRacyInfo,
-               const js::ProfileEntry& entry,
-               ProfilerStackCollector& aCollector)
-{
-  // WARNING: this function runs within the profiler's "critical section".
-  // WARNING: this function might be called while the profiler is inactive, and
-  //          cannot rely on ActivePS.
-
-  MOZ_ASSERT(entry.kind() == js::ProfileEntry::Kind::CPP_NORMAL ||
-             entry.kind() == js::ProfileEntry::Kind::JS_NORMAL);
-
-  const char* label = entry.label();
-  const char* dynamicString = entry.dynamicString();
-  bool isChromeJSEntry = false;
-  int lineno = -1;
-
-  if (entry.isJs()) {
-    // There are two kinds of JS frames that get pushed onto the PseudoStack.
-    //
-    // - label = "", dynamic string = <something>
-    // - label = "js::RunScript", dynamic string = nullptr
-    //
-    // The line number is only interesting for the first case.
-    if (label[0] == '\0') {
-      MOZ_ASSERT(dynamicString);
-
-      // We call entry.script() repeatedly -- rather than storing the result in
-      // a local variable in order -- to avoid rooting hazards.
-      if (entry.script()) {
-        isChromeJSEntry = IsChromeJSScript(entry.script());
-        if (entry.pc()) {
-          lineno = JS_PCToLineNumber(entry.script(), entry.pc());
-        } else {
-          // The JIT only allows the top-most entry to have a nullptr pc.
-          MOZ_ASSERT(&entry == &aRacyInfo->entries[aRacyInfo->stackSize() - 1]);
-        }
-      }
-
-    } else {
-      MOZ_ASSERT(strcmp(label, "js::RunScript") == 0 && !dynamicString);
-    }
-
-  } else {
-    MOZ_ASSERT(entry.isCpp());
-    lineno = entry.line();
-  }
-
-  if (dynamicString) {
-    // Adjust the dynamic string as necessary.
-    if (ProfilerFeature::HasPrivacy(aFeatures) && !isChromeJSEntry) {
-      dynamicString = "(private)";
-    } else if (strlen(dynamicString) >= ProfileBuffer::kMaxFrameKeyLength) {
-      dynamicString = "(too long)";
-    }
-  }
-
-  aCollector.CollectCodeLocation(label, dynamicString, lineno,
-                                 Some(entry.category()));
-}
-
 // Setting MAX_NATIVE_FRAMES too high risks the unwinder wasting a lot of time
 // looping on corrupted stacks.
 //
@@ -965,7 +891,10 @@ MergeStacks(uint32_t aFeatures, bool aIsSynchronous,
       // Pseudo-frames with the CPP_MARKER_FOR_JS kind are just annotations and
       // should not be recorded in the profile.
       if (pseudoEntry.kind() != js::ProfileEntry::Kind::CPP_MARKER_FOR_JS) {
-        AddPseudoEntry(aFeatures, racyInfo, pseudoEntry, aCollector);
+        // The JIT only allows the top-most entry to have a nullptr pc.
+        MOZ_ASSERT_IF(pseudoEntry.isJs() && pseudoEntry.script() && !pseudoEntry.pc(),
+                      &pseudoEntry == &racyInfo->entries[racyInfo->stackSize() - 1]);
+        aCollector.CollectPseudoEntry(pseudoEntry);
       }
       pseudoIndex++;
       continue;
@@ -991,7 +920,7 @@ MergeStacks(uint32_t aFeatures, bool aIsSynchronous,
       // with stale JIT code return addresses.
       if (aIsSynchronous ||
           jsFrame.kind == JS::ProfilingFrameIterator::Frame_Wasm) {
-        aCollector.CollectCodeLocation("", jsFrame.label, -1, Nothing());
+        aCollector.CollectWasmFrame(jsFrame.label);
       } else {
         MOZ_ASSERT(jsFrame.kind == JS::ProfilingFrameIterator::Frame_Ion ||
                    jsFrame.kind == JS::ProfilingFrameIterator::Frame_Baseline);
@@ -1346,18 +1275,19 @@ DoSharedSample(PSLockRef aLock, bool aIsSynchronous,
   TimeDuration delta = aNow - CorePS::ProcessStartTime();
   aBuffer.AddEntry(ProfileBufferEntry::Time(delta.ToMilliseconds()));
 
+  ProfileBufferCollector collector(aBuffer, ActivePS::Features(aLock));
   NativeStack nativeStack;
 #if defined(HAVE_NATIVE_UNWIND)
   if (ActivePS::FeatureStackWalk(aLock)) {
     DoNativeBacktrace(aLock, aThreadInfo, aRegs, nativeStack);
 
     MergeStacks(ActivePS::Features(aLock), aIsSynchronous, aThreadInfo, aRegs,
-                nativeStack, aBuffer);
+                nativeStack, collector);
   } else
 #endif
   {
     MergeStacks(ActivePS::Features(aLock), aIsSynchronous, aThreadInfo, aRegs,
-                nativeStack, aBuffer);
+                nativeStack, collector);
 
     // We can't walk the whole native stack, but we can record the top frame.
     if (ActivePS::FeatureLeaf(aLock)) {

tools/profiler/public/GeckoProfiler.h

@@ -309,10 +309,9 @@ public:
 
   virtual void CollectJitReturnAddr(void* aAddr) = 0;
 
-  // aLabel is static and never null. aStr may be null. aLineNumber may be -1.
-  virtual void CollectCodeLocation(
-    const char* aLabel, const char* aStr, int aLineNumber,
-    const mozilla::Maybe<js::ProfileEntry::Category>& aCategory) = 0;
+  virtual void CollectWasmFrame(const char* aLabel) = 0;
+
+  virtual void CollectPseudoEntry(const js::ProfileEntry& aEntry) = 0;
 };
 
 // This method suspends the thread identified by aThreadId, samples its

tools/profiler/tests/gtest/GeckoProfiler.cpp

@@ -750,9 +750,8 @@ public:
 
   virtual void CollectNativeLeafAddr(void* aAddr) { mFrames++; }
   virtual void CollectJitReturnAddr(void* aAddr) { mFrames++; }
-  virtual void CollectCodeLocation(
-    const char* aLabel, const char* aStr, int aLineNumber,
-    const mozilla::Maybe<js::ProfileEntry::Category>& aCategory) { mFrames++; }
+  virtual void CollectWasmFrame(const char* aLabel) { mFrames++; }
+  virtual void CollectPseudoEntry(const js::ProfileEntry& aEntry) { mFrames++; }
 
   int mSetIsMainThread;
   int mFrames;