Identify profiler suspend reasons (#1608)

Summary:
Pull Request resolved: #1608

The sampling profiler tracks the type of each frame. A "suspend" frame
denotes that execution is suspended, with an arbitrary string providing
details.

We would like to identify GC frames for the sake of Chrome DevTools
visualizations/stats about GC time. This diff adds an enum to suspend
frames to allow GC frames to be identified without depending on
specific contents of the details string.

Another option would be to replace the "suspend" frame with multiple
different frame types for debugger, GC, etc.

Reviewed By: fbmal7

Differential Revision: D68442991

fbshipit-source-id: d16a3a1b87657e538e8c5471ea9533876f1c757d

Author: Matt Blagden

include/hermes/VM/Profiler/SamplingProfiler.h

@@ -56,8 +56,29 @@ class SamplingProfiler {
   /// been registered with the sampling profiler yet, and therefore can be moved
   /// by the GC.
   using NativeFunctionFrameInfo = size_t;
-  /// GC frame info. Pointing to string in suspendEventExtraInfoSet_.
-  using SuspendFrameInfo = const std::string *;
+
+  /// Details about a suspend frame.
+  struct SuspendFrameInfo {
+    /// GC frame info. Pointing to string in gcEventExtraInfoSet_.
+    using GCFrameInfo = const std::string *;
+
+    /// Cause of the suspension.
+    enum class Kind {
+      /// Suspended to perform GC actions. The gcFrame field contains additional
+      /// details specific to the GC.
+      GC,
+      /// Suspended to perform debugger actions.
+      Debugger,
+      /// Multiple suspensions have occurred.
+      Multiple,
+    } kind;
+    /// Extra information about GC suspensions. Only valid when \c kind is
+    /// \c GC. Pointing to string in gcEventExtraInfoSet_; it is optional
+    /// and can be null to indicate no extra info.
+    /// We can't directly use std::string here because it is
+    /// inside a union.
+    GCFrameInfo gcFrame;
+  };
 
   // This will break with more than one RuntimeModule(like FB4a, eval() call or
   // lazy compilation etc...). It is simply a temporary thing to get started.
@@ -80,11 +101,7 @@ class SamplingProfiler {
       LoomNativeFrameInfo nativeFunctionPtrForLoom;
       /// Native function frame info storage used for "regular" profiling.
       NativeFunctionFrameInfo nativeFrame;
-      /// Suspend frame info. Pointing to string
-      /// in suspendExtraInfoSet_; it is optional and
-      /// can be null to indicate no extra info.
-      /// We can't directly use std::string here because it is
-      /// inside a union.
+      /// Suspend frame info.
       SuspendFrameInfo suspendFrame;
     };
     FrameKind kind;
@@ -175,7 +192,7 @@ class SamplingProfiler {
   ThreadNamesMap threadNames_;
 
   /// Unique GC event extra info strings container.
-  std::unordered_set<std::string> suspendEventExtraInfoSet_;
+  std::unordered_set<std::string> gcEventExtraInfoSet_;
 
   /// Domains to be kept alive for sampled RuntimeModules. Protected by
   /// runtimeDataLock_.
@@ -223,7 +240,9 @@ class SamplingProfiler {
  private:
   /// Record JS stack at time of suspension, caller must hold
   /// runtimeDataLock_.
-  void recordPreSuspendStack(std::string_view extraInfo);
+  void recordPreSuspendStack(
+      SuspendFrameInfo::Kind reason,
+      llvh::StringRef gcFrame);
 
  protected:
   /// Clear previous stored samples.
@@ -269,8 +288,11 @@ class SamplingProfiler {
   static bool disable();
 
   /// Suspends the sample profiling. Every call to suspend must be matched by a
-  /// call to resume.
-  void suspend(std::string_view reason);
+  /// call to resume. \c reason indicates the cause of suspension. If \c reason
+  /// is GC, \c gcFrame can provide extra details, otherwise it is ignored.
+  void suspend(
+      SuspendFrameInfo::Kind reason,
+      llvh::StringRef gcSuspendDetails = "");
 
   /// Resumes the sample profiling. There must have been a previous call to
   /// suspend() that hansn't been resume()d yet.
@@ -290,7 +312,7 @@ class SuspendSamplingProfilerRAII {
  public:
   explicit SuspendSamplingProfilerRAII(
       Runtime &runtime,
-      std::string_view reason = "")
+      SamplingProfiler::SuspendFrameInfo::Kind reason)
       : profiler_(runtime.samplingProfiler.get()) {
     if (profiler_) {
       profiler_->suspend(reason);

lib/VM/Debugger/Debugger.cpp

@@ -422,7 +422,8 @@ ExecutionStatus Debugger::debuggerLoop(
   // Keep the evalResult alive, even if all other handles are flushed.
   static constexpr unsigned KEEP_HANDLES = 1;
 #if HERMESVM_SAMPLING_PROFILER_AVAILABLE
-  SuspendSamplingProfilerRAII ssp{runtime_, "debugger"};
+  SuspendSamplingProfilerRAII ssp{
+      runtime_, SamplingProfiler::SuspendFrameInfo::Kind::Debugger};
 #endif // HERMESVM_SAMPLING_PROFILER_AVAILABLE
   while (true) {
     GCScopeMarkerRAII marker{runtime_};

lib/VM/Profiler/ChromeTraceSerializer.cpp

@@ -16,6 +16,21 @@
 namespace hermes {
 namespace vm {
 
+static std::string getSuspendFrameName(
+    const SamplingProfiler::SuspendFrameInfo &info) {
+  const char *suspendFrameName;
+  if (info.kind == SamplingProfiler::SuspendFrameInfo::Kind::GC) {
+    suspendFrameName = info.gcFrame->c_str();
+  } else if (info.kind == SamplingProfiler::SuspendFrameInfo::Kind::Debugger) {
+    suspendFrameName = "debugger";
+  } else if (info.kind == SamplingProfiler::SuspendFrameInfo::Kind::Multiple) {
+    suspendFrameName = "multiple";
+  } else {
+    assert(false && "suspendFrame name should never be null");
+  }
+  return std::string("[") + suspendFrameName + "]";
+}
+
 std::shared_ptr<ChromeStackFrameNode> ChromeStackFrameNode::findOrAddNewChild(
     ChromeFrameIdGenerator &frameIdGen,
     const SamplingProfiler::StackFrame &target) {
@@ -249,8 +264,7 @@ void ChromeTraceSerializer::serializeStackFrames(JSONEmitter &json) const {
       }
 
       case SamplingProfiler::StackFrame::FrameKind::SuspendFrame: {
-        assert(frame.suspendFrame && "suspendFrame name should never be null");
-        frameName = "[" + *frame.suspendFrame + "]";
+        frameName = getSuspendFrameName(frame.suspendFrame);
         categoryName = "Metadata";
         break;
       }
@@ -446,8 +460,7 @@ void ProfilerProfileSerializer::processNode(
     }
 
     case SamplingProfiler::StackFrame::FrameKind::SuspendFrame: {
-      assert(frame.suspendFrame && "suspendFrame should never be nullptr");
-      name = "[" + *frame.suspendFrame + "]";
+      name = getSuspendFrameName(frame.suspendFrame);
       url = "[suspended]";
       break;
     }

lib/VM/Profiler/SamplingProfiler.cpp

@@ -224,22 +224,26 @@ void SamplingProfiler::clear() {
   threadNames_.clear();
 }
 
-void SamplingProfiler::suspend(std::string_view extraInfo) {
+void SamplingProfiler::suspend(
+    SuspendFrameInfo::Kind reason,
+    llvh::StringRef gcSuspendDetails) {
   // Need to check whether the profiler is enabled without holding the
   // runtimeDataLock_. Otherwise, we'd have a lock inversion.
   bool enabled = sampling_profiler::Sampler::get()->enabled();
 
   std::lock_guard<std::mutex> lk(runtimeDataLock_);
-  if (++suspendCount_ > 1 || extraInfo.empty()) {
-    // If there are multiple nested suspend calls use a default "suspended"
-    // label for the suspend entry in the call stack. Also use the default
-    // when no extra info is provided.
-    extraInfo = "suspended";
+  if (++suspendCount_ > 1) {
+    // If there are multiple nested suspend calls use a default "multiple" frame
+    // kind for the suspend entry in the call stack.
+    reason = SuspendFrameInfo::Kind::Multiple;
+  } else if (reason == SuspendFrameInfo::Kind::GC && gcSuspendDetails.empty()) {
+    // If no GC reason was provided, use a default "suspend" value.
+    gcSuspendDetails = "suspend";
   }
 
   // Only record the stack trace for the first suspend() call.
   if (LLVM_UNLIKELY(enabled && suspendCount_ == 1)) {
-    recordPreSuspendStack(extraInfo);
+    recordPreSuspendStack(reason, gcSuspendDetails);
   }
 }
 
@@ -251,10 +255,15 @@ void SamplingProfiler::resume() {
   }
 }
 
-void SamplingProfiler::recordPreSuspendStack(std::string_view extraInfo) {
-  std::pair<std::unordered_set<std::string>::iterator, bool> retPair =
-      suspendEventExtraInfoSet_.emplace(extraInfo);
-  SuspendFrameInfo suspendExtraInfo = &(*(retPair.first));
+void SamplingProfiler::recordPreSuspendStack(
+    SuspendFrameInfo::Kind reason,
+    llvh::StringRef gcFrame) {
+  SuspendFrameInfo suspendExtraInfo{reason, nullptr};
+  if (reason == SuspendFrameInfo::Kind::GC) {
+    std::pair<std::unordered_set<std::string>::iterator, bool> retPair =
+        gcEventExtraInfoSet_.emplace(gcFrame);
+    suspendExtraInfo.gcFrame = &(*(retPair.first));
+  }
 
   auto &leafFrame = preSuspendStackStorage_.stack[0];
   leafFrame.kind = StackFrame::FrameKind::SuspendFrame;
@@ -281,7 +290,8 @@ bool operator==(
       return left.nativeFrame == right.nativeFrame;
 
     case SamplingProfiler::StackFrame::FrameKind::SuspendFrame:
-      return left.suspendFrame == right.suspendFrame;
+      return left.suspendFrame.kind == right.suspendFrame.kind &&
+          left.suspendFrame.gcFrame == right.suspendFrame.gcFrame;
 
     default:
       llvm_unreachable("Unknown frame kind");

lib/VM/Runtime.cpp

@@ -2104,7 +2104,8 @@ void Runtime::onGCEvent(GCEventKind kind, const std::string &extraInfo) {
   if (samplingProfiler) {
     switch (kind) {
       case GCEventKind::CollectionStart:
-        samplingProfiler->suspend(extraInfo);
+        samplingProfiler->suspend(
+            SamplingProfiler::SuspendFrameInfo::Kind::GC, extraInfo);
         break;
       case GCEventKind::CollectionEnd:
         samplingProfiler->resume();