remove memory allocation in signal handler

Summary:
Context: `ModuleIdManager` is introduced in SamplingProfiler to hold RuntimeModule alive with reference counting. With the new domain design, it is not necessary, holding strong references to Domain is enough. However, pushing a Domain into vector may cause memory allocation in signal handler causing potential deadlock/crash.
To solve this, two new methods `increaseDomainCount/decreaseDomainCount` are added to perform `domains_` storage management outside of signal handler.

To summarize, this diff does:
* Remove `ModuleIdManager` and directly use RuntimeModule pointers in stack frame.
* Introduce `increaseDomainCount/decreaseDomainCount` to manage `domains_` storage to avoid memory allocation in signal handler.
* Added a new testcase

Reviewed By: davedets

Differential Revision: D15002859

fbshipit-source-id: 39460898a5acaa36cca772019c38d6871507b30c

include/hermes/VM/Profiler/ChromeTraceSerializerPosix.h

@@ -193,16 +193,13 @@ class ChromeTraceSerializer {
   // Emit "sampled" events for captured stack traces.
   void serializeSampledEvents(JSONEmitter &json) const;
   // Emit "stackFrames" entries.
-  void serializeStackFrames(
-      JSONEmitter &json,
-      const ModuleIdManager &moduleIdManager) const;
+  void serializeStackFrames(JSONEmitter &json) const;
 
  public:
   explicit ChromeTraceSerializer(ChromeTraceFormat &&chromeTrace);
 
   /// Serialize chrome trace to \p OS.
-  void serialize(const ModuleIdManager &moduleIdManager, llvm::raw_ostream &OS)
-      const;
+  void serialize(llvm::raw_ostream &OS) const;
 };
 
 } // namespace vm

include/hermes/VM/Profiler/SamplingProfilerPosix.h

@@ -9,7 +9,6 @@
 
 #include "hermes/Support/Semaphore.h"
 #include "hermes/Support/ThreadLocal.h"
-#include "hermes/VM/Profiler/ModuleIdManager.h"
 #include "hermes/VM/Runtime.h"
 
 #include "llvm/ADT/DenseMap.h"
@@ -42,8 +41,8 @@ class SamplingProfiler {
   // TODO: consolidate the stack frame struct with other function/extern
   // profilers.
   struct JSFunctionFrameInfo {
-    // Id of the module this function is associated with.
-    ModuleIdManager::ModuleId runtimeModuleId;
+    // RuntimeModule this function is associated with.
+    RuntimeModule *module;
     // Function id associated with current frame.
     uint32_t functionId;
     // IP offset within the function.
@@ -100,8 +99,9 @@ class SamplingProfiler {
   /// signal handler is unsafe.
   static volatile std::atomic<SamplingProfiler *> sProfilerInstance_;
 
-  /// Lock for profiler operations.
+  /// Lock for profiler operations and access to member fields.
   std::mutex profilerLock_;
+
   /// Stores a list of active <thread, runtime> pair.
   /// Protected by profilerLock_.
   llvm::DenseMap<Runtime *, pthread_t> activeRuntimeThreads_;
@@ -134,8 +134,16 @@ class SamplingProfiler {
   /// Prellocated map that contains thread names mapping.
   ThreadNamesMap threadNames_;
 
-  /// Manages the module id for symbolication.
-  ModuleIdManager moduleIdManager_;
+  /// Domains to be kept alive for sampled RuntimeModules.
+  /// Its storage size is increased/decreased by
+  /// increaseDomainCount/decreaseDomainCount outside signal handler.
+  /// New storage is initialized with null pointers.
+  /// This prevents any memory allocation to domains_ inside
+  /// signal handler.
+  /// domains_.size() >= number of constructed but not destructed Domain
+  /// objects.
+  /// registerDomain() keeps a Domain from being destructed.
+  std::vector<Domain *> domains_;
 
  private:
   SamplingProfiler();
@@ -151,6 +159,11 @@ class SamplingProfiler {
   /// Unregister sampling signal handler.
   bool unregisterSignalHandler();
 
+  /// Hold \p domain so that the RuntimeModule(s) used by profiler are not
+  /// released during symbolication.
+  /// Refer to Domain.h for relationship between Domain and RuntimeModule.
+  void registerDomain(Domain *domain);
+
   /// Signal handler to walk the stack frames.
   static void profilingSignalHandler(int signo);
 
@@ -175,12 +188,7 @@ class SamplingProfiler {
 
   /// Clear previous stored samples.
   /// Note: caller should take the lock before calling.
-  void clear() {
-    sampledStacks_.clear();
-    moduleIdManager_.clear();
-    // TODO: keep thread names that are still in use.
-    threadNames_.clear();
-  }
+  void clear();
 
  public:
   /// Return the singleton profiler instance.
@@ -193,9 +201,16 @@ class SamplingProfiler {
   /// Unregister an active \p runtime and current thread with profiler.
   void unregisterRuntime(Runtime *runtime);
 
+  /// Reserve domain slots to avoid memory allocation in signal handler.
+  void increaseDomainCount();
+  /// Shrink domain storage to fit domains alive.
+  void decreaseDomainCount();
+
   /// Mark roots that are kept alive by the SamplingProfiler.
   void markRoots(SlotAcceptorWithNames &acceptor) {
-    moduleIdManager_.markRoots(acceptor);
+    for (Domain *&domain : domains_) {
+      acceptor.acceptPtr(domain);
+    }
   }
 
   /// Dump sampled stack to \p OS.

include/hermes/VM/Profiler/SamplingProfilerWindows.h

@@ -28,6 +28,12 @@ class SamplingProfiler {
   /// Unregister an active \p runtime and current thread with profiler.
   void unregisterRuntime(Runtime *runtime) {}
 
+  /// Reserve domain slots to avoid memory allocation in signal handler.
+  void increaseDomainCount() {}
+
+  /// Shrink domain storage to fit domains alive.
+  void decreaseDomainCount() {}
+
   /// Mark roots that are kept alive by the SamplingProfiler.
   void markRoots(SlotAcceptorWithNames &acceptor) {}
 

lib/VM/CMakeLists.txt

@@ -36,7 +36,6 @@ set(source_files
   Runtime.cpp Runtime-profilers.cpp
   RuntimeModule.cpp
   Profiler/ChromeTraceSerializerPosix.cpp
-  Profiler/ModuleIdManager.cpp
   Profiler/SamplingProfilerWindows.cpp
   Profiler/SamplingProfilerPosix.cpp
   SegmentedArray.cpp

lib/VM/Domain.cpp

@@ -9,6 +9,7 @@
 #include "hermes/VM/Callable.h"
 #include "hermes/VM/GCPointer-inline.h"
 #include "hermes/VM/JSLib.h"
+#include "hermes/VM/Profiler/SamplingProfiler.h"
 
 namespace hermes {
 namespace vm {
@@ -29,12 +30,16 @@ PseudoHandle<Domain> Domain::create(Runtime *runtime) {
   void *mem =
       runtime->alloc</*fixedSize*/ true, HasFinalizer::Yes>(sizeof(Domain));
   auto self = createPseudoHandle(new (mem) Domain(runtime));
+  auto &samplingProfiler = SamplingProfiler::getInstance();
+  samplingProfiler->increaseDomainCount();
   return self;
 }
 
 void Domain::_finalizeImpl(GCCell *cell, GC *gc) {
   auto *self = vmcast<Domain>(cell);
   self->~Domain();
+  auto &samplingProfiler = SamplingProfiler::getInstance();
+  samplingProfiler->decreaseDomainCount();
 }
 
 Domain::~Domain() {

lib/VM/Profiler/ChromeTraceSerializerPosix.cpp

@@ -177,29 +177,23 @@ void ChromeTraceSerializer::serializeSampledEvents(JSONEmitter &json) const {
   }
 }
 
-void ChromeTraceSerializer::serializeStackFrames(
-    JSONEmitter &json,
-    const ModuleIdManager &moduleIdManager) const {
-  const ModuleIdManager::BCProviderMap bcProviderMap =
-      moduleIdManager.generateBCProviderMap();
+void ChromeTraceSerializer::serializeStackFrames(JSONEmitter &json) const {
   for (const auto &tree : trace_.getCallTree()) {
-    tree->dfsWalk([&json, &bcProviderMap](
+    tree->dfsWalk([&json](
                       const ChromeStackFrameNode &node,
                       const ChromeStackFrameNode *parent) {
       json.emitKey(oscompat::to_string(node.getId()));
       json.openDict();
 
-      auto getFunctionName =
-          [](const std::shared_ptr<hbc::BCProvider> &bcProvider,
-             uint32_t funcId) {
-            hbc::RuntimeFunctionHeader functionHeader =
-                bcProvider->getFunctionHeader(funcId);
-            return bcProvider->getStringRefFromID(functionHeader.functionName())
-                .str();
-          };
+      auto getFunctionName = [](hbc::BCProvider *bcProvider, uint32_t funcId) {
+        hbc::RuntimeFunctionHeader functionHeader =
+            bcProvider->getFunctionHeader(funcId);
+        return bcProvider->getStringRefFromID(functionHeader.functionName())
+            .str();
+      };
 
       auto getSourceLocation =
-          [](const std::shared_ptr<hbc::BCProvider> &bcProvider,
+          [](hbc::BCProvider *bcProvider,
              uint32_t funcId,
              uint32_t opcodeOffset) -> OptValue<hbc::DebugSourceLocation> {
         const hbc::DebugOffsets *debugOffsets =
@@ -215,9 +209,8 @@ void ChromeTraceSerializer::serializeStackFrames(
       std::string frameName, categoryName;
       const auto &frame = node.getFrameInfo();
       if (frame.kind == SamplingProfiler::StackFrame::FrameKind::JSFunction) {
-        ModuleIdManager::ModuleId moduleId = frame.jsFrame.runtimeModuleId;
-        assert(bcProviderMap.count(moduleId) == 1 && "Unknown module id");
-        const auto &bcProvider = bcProviderMap.lookup(moduleId);
+        RuntimeModule *module = frame.jsFrame.module;
+        hbc::BCProvider *bcProvider = module->getBytecode();
 
         llvm::raw_string_ostream os(frameName);
         os << getFunctionName(bcProvider, frame.jsFrame.functionId);
@@ -281,9 +274,7 @@ void ChromeTraceSerializer::serializeStackFrames(
   }
 }
 
-void ChromeTraceSerializer::serialize(
-    const ModuleIdManager &moduleIdManager,
-    llvm::raw_ostream &OS) const {
+void ChromeTraceSerializer::serialize(llvm::raw_ostream &OS) const {
   JSONEmitter json(OS);
 
   // The format of the chrome trace is a bit vague. Here are the essential
@@ -316,7 +307,7 @@ void ChromeTraceSerializer::serialize(
   // Emit "stackFrames" entries.
   json.emitKey("stackFrames");
   json.openDict();
-  serializeStackFrames(json, moduleIdManager);
+  serializeStackFrames(json);
   json.closeDict(); // stackFrames.
 
   json.closeDict(); // Whole trace.