Use push model for JS trace provider

Summary:
Right now the JS trace provider uses a pull model. Specifically, every 50ms, it asks the Hermes profiler to collect the stack trace from the JS thread and write them into loom entries. The problem is that this process needs to interupt the JS thread by holding a lock, and that causes race conditions with garbage collection.

This diff changes it to use pull model. Specifically, the Hermes profiler itself already have a timerLoop which proves to work well, that timerLoop collects the stack traces every N milliseconds, we can just write the stack traces into loom after that. This model makes more sense.

Here is a summary for each diff on the stack
1. Add the new API where the sampling profiler will call
2. Remove the timer loop in FBLoomJSTraceProvider, and instead set a callback for write stack to loom.
3. Let sampling profiler call the API added in step 1
4. Clean up the old code path

Reviewed By: jpporto

Differential Revision: D40944677

fbshipit-source-id: 5bab67a45797d673fc5403e73cfca83e66594b69

include/hermes/VM/Profiler/SamplingProfilerPosix.h

@@ -188,12 +188,6 @@ class SamplingProfiler {
     /// Whether signal handler is registered or not. Protected by profilerLock_.
     bool isSigHandlerRegistered_{false};
 
-#if defined(__APPLE__) && defined(HERMES_FACEBOOK_BUILD)
-    /// Indicating whether or not we are in the middle of collecting stack for
-    /// loom.
-    bool collectingStack_{false};
-#endif
-
     /// Semaphore to indicate all signal handlers have finished the sampling.
     Semaphore samplingDoneSem_;
 
@@ -216,12 +210,6 @@ class SamplingProfiler {
     /// member variable.
     std::condition_variable enabledCondVar_;
 
-#if defined(__APPLE__) && defined(HERMES_FACEBOOK_BUILD)
-    /// This condition variable is used when we disable profiler for loom
-    /// collection, in the disableForLoomCollection() function.
-    std::condition_variable disableForLoomCondVar_;
-#endif
-
     /// invoke sigaction() posix API to register \p handler.
     /// \return what sigaction() returns: 0 to indicate success.
     static int invokeSignalAction(void (*handler)(int));
@@ -249,13 +237,6 @@ class SamplingProfiler {
     bool enable();
     bool disable();
 
-#if defined(__APPLE__) && defined(HERMES_FACEBOOK_BUILD)
-    /// Modified version of enable/disable, designed to be called by
-    /// SamplingProfiler::collectStackForLoom.
-    bool enableForLoomCollection();
-    bool disableForLoomCollection();
-#endif
-
     /// \return true if the sampling profiler is enabled, false otherwise.
     bool enabled();
 
@@ -357,19 +338,6 @@ class SamplingProfiler {
       uint16_t max_depth);
 #endif
 
-#if defined(__APPLE__) && defined(HERMES_FACEBOOK_BUILD)
-  /// Registered loom callback for collecting stack frames.
-  static FBLoomStackCollectionRetcode collectStackForLoom(
-      int64_t *frames,
-      uint16_t *depth,
-      uint16_t max_depth,
-      void *profiler);
-  /// Modified version of enable/disable, designed to be called by
-  /// SamplingProfiler::collectStackForLoom.
-  static bool enableForLoom();
-  static bool disableForLoom();
-#endif
-
   /// Clear previous stored samples.
   /// Note: caller should take the lock before calling.
   void clear();

lib/VM/Profiler/SamplingProfilerPosix.cpp

@@ -470,82 +470,13 @@ void SamplingProfiler::collectStackForLoomCommon(
 }
 #endif
 
-#if defined(__APPLE__) && defined(HERMES_FACEBOOK_BUILD)
-/*static*/ FBLoomStackCollectionRetcode SamplingProfiler::collectStackForLoom(
-    int64_t *frames,
-    uint16_t *depth,
-    uint16_t max_depth,
-    void *profiler) {
-  auto profilerInstance = GlobalProfiler::get();
-  {
-    std::unique_lock<std::mutex> lock(profilerInstance->profilerLock_);
-    if (!profilerInstance->enabled_) {
-      return FBLoomStackCollectionRetcode::NO_STACK_FOR_THREAD;
-    }
-    profilerInstance->collectingStack_ = true;
-  }
-  auto *localProfiler = reinterpret_cast<SamplingProfiler *>(profiler);
-  std::lock_guard<std::mutex> lk(localProfiler->runtimeDataLock_);
-
-  *depth = 0;
-  // Sampling stack will touch GC objects(like closure) so
-  // only do so if heap is valid.
-  if (LLVM_LIKELY(localProfiler->suspendCount_ == 0)) {
-    // Do not register domains for Loom profiling, since we don't use them for
-    // symbolication.
-    if (!profilerInstance->sampleStack(localProfiler)) {
-      profilerInstance->collectingStack_ = false;
-      profilerInstance->disableForLoomCondVar_.notify_one();
-      return FBLoomStackCollectionRetcode::NO_STACK_FOR_THREAD;
-    }
-  } else {
-    profilerInstance->collectingStack_ = false;
-    profilerInstance->disableForLoomCondVar_.notify_one();
-    return FBLoomStackCollectionRetcode::EMPTY_STACK;
-  }
-
-  uint32_t index = 0;
-  for (unsigned i = 0; i < localProfiler->sampledStacks_.size(); ++i) {
-    auto &sample = localProfiler->sampledStacks_[i];
-    for (auto iter = sample.stack.rbegin(); iter != sample.stack.rend();
-         ++iter) {
-      const StackFrame &frame = *iter;
-      localProfiler->collectStackForLoomCommon(frame, frames, index);
-      (*depth)++;
-      index++;
-    }
-  }
-  localProfiler->clear();
-  if (*depth == 0) {
-    profilerInstance->collectingStack_ = false;
-    profilerInstance->disableForLoomCondVar_.notify_one();
-    return FBLoomStackCollectionRetcode::EMPTY_STACK;
-  }
-  profilerInstance->collectingStack_ = false;
-  profilerInstance->disableForLoomCondVar_.notify_one();
-  return FBLoomStackCollectionRetcode::SUCCESS;
-}
-
-/*static*/ bool SamplingProfiler::enableForLoom() {
-  auto profilerInstance = GlobalProfiler::get();
-  return profilerInstance->enableForLoomCollection();
-}
-
-/*static*/ bool SamplingProfiler::disableForLoom() {
-  auto profilerInstance = GlobalProfiler::get();
-  return profilerInstance->disableForLoomCollection();
-}
-#endif
-
 SamplingProfiler::SamplingProfiler(Runtime &runtime)
     : currentThread_{pthread_self()}, runtime_{runtime} {
   threadNames_[oscompat::thread_id()] = oscompat::thread_name();
   GlobalProfiler::get()->registerRuntime(this);
 #if defined(__APPLE__) && defined(HERMES_FACEBOOK_BUILD)
   // TODO(xidachen): do a refactor to use the enum in ExternalTracer.h
   const int32_t tracerTypeJavascript = 1;
-  fbloom_profilo_api()->fbloom_register_external_tracer_callback(
-      1, this, collectStackForLoom);
   fbloom_profilo_api()->fbloom_register_enable_for_loom_callback(
       tracerTypeJavascript, enable);
   fbloom_profilo_api()->fbloom_register_disable_for_loom_callback(
@@ -659,23 +590,6 @@ bool SamplingProfiler::GlobalProfiler::enable() {
   return true;
 }
 
-#if defined(__APPLE__) && defined(HERMES_FACEBOOK_BUILD)
-bool SamplingProfiler::GlobalProfiler::enableForLoomCollection() {
-  std::lock_guard<std::mutex> lockGuard(profilerLock_);
-  if (enabled_) {
-    return true;
-  }
-  if (!samplingDoneSem_.open(kSamplingDoneSemaphoreName)) {
-    return false;
-  }
-  if (!registerSignalHandlers()) {
-    return false;
-  }
-  enabled_ = true;
-  return true;
-}
-#endif
-
 bool SamplingProfiler::disable() {
   return GlobalProfiler::get()->disable();
 }
@@ -706,32 +620,6 @@ bool SamplingProfiler::GlobalProfiler::disable() {
   return true;
 }
 
-#if defined(__APPLE__) && defined(HERMES_FACEBOOK_BUILD)
-bool SamplingProfiler::GlobalProfiler::disableForLoomCollection() {
-  {
-    std::unique_lock lock(profilerLock_);
-    while (collectingStack_) {
-      disableForLoomCondVar_.wait(lock);
-    }
-
-    if (!enabled_) {
-      // Already disabled.
-      return true;
-    }
-    if (!samplingDoneSem_.close()) {
-      return false;
-    }
-    // Unregister handlers before shutdown.
-    if (!unregisterSignalHandler()) {
-      return false;
-    }
-    // Telling timer thread to exit.
-    enabled_ = false;
-  }
-  return true;
-}
-#endif
-
 void SamplingProfiler::clear() {
   sampledStacks_.clear();
   // Release all strong roots.