refactor: well-defined behaviour (facebook#52187)

Summary:

# Changelog: [Internal]

- `tracing_` is now `std::atomic<bool>`.
- `performanceMeasureCount_` is now`std::atomic<uint32_t>`.
- `mutex_` -> `bufferMutex_`.


The biggest change is that we no longer read from `tracing_` directly for an early-return when not tracing. This should not significantly regress performance of the subsystem, see the test plan for numbers.

Differential Revision: D77053030

Author: hoxyq

packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.cpp

@@ -27,19 +27,14 @@ PerformanceTracer::PerformanceTracer()
     : processId_(oscompat::getCurrentProcessId()) {}
 
 bool PerformanceTracer::startTracing() {
-  {
-    std::lock_guard lock(mutex_);
-    if (tracing_) {
-      return false;
-    }
-
-    tracing_ = true;
+  if (tracing_.exchange(true, std::memory_order_acq_rel)) {
+    return false;
   }
 
   reportProcess(processId_, "React Native");
 
   {
-    std::lock_guard lock(mutex_);
+    std::lock_guard lock(bufferMutex_);
     buffer_.push_back(TraceEvent{
         .name = "TracingStartedInPage",
         .cat = "disabled-by-default-devtools.timeline",
@@ -49,42 +44,44 @@ bool PerformanceTracer::startTracing() {
         .tid = oscompat::getCurrentThreadId(),
         .args = folly::dynamic::object("data", folly::dynamic::object()),
     });
-
-    return true;
   }
+
+  return true;
 }
 
 bool PerformanceTracer::stopTracing() {
-  std::lock_guard lock(mutex_);
-  if (!tracing_) {
+  if (!tracing_.exchange(false, std::memory_order_acq_rel)) {
     return false;
   }
 
-  // This is synthetic Trace Event, which should not be represented on a
-  // timeline. CDT is not using Profile or ProfileChunk events for determining
-  // trace timeline window, this is why trace that only contains JavaScript
-  // samples will be displayed as empty. We use this event to avoid that.
-  // This could happen for non-bridgeless apps, where Performance interface is
-  // not supported and no spec-compliant Event Loop implementation.
-  buffer_.push_back(TraceEvent{
-      .name = "ReactNative-TracingStopped",
-      .cat = "disabled-by-default-devtools.timeline",
-      .ph = 'I',
-      .ts = HighResTimeStamp::now(),
-      .pid = processId_,
-      .tid = oscompat::getCurrentThreadId(),
-  });
+  {
+    std::lock_guard lock(bufferMutex_);
+    // This is synthetic Trace Event, which should not be represented on a
+    // timeline. CDT is not using Profile or ProfileChunk events for determining
+    // trace timeline window, this is why trace that only contains JavaScript
+    // samples will be displayed as empty. We use this event to avoid that.
+    // This could happen for non-bridgeless apps, where Performance interface is
+    // not supported and no spec-compliant Event Loop implementation.
+    buffer_.push_back(TraceEvent{
+        .name = "ReactNative-TracingStopped",
+        .cat = "disabled-by-default-devtools.timeline",
+        .ph = 'I',
+        .ts = HighResTimeStamp::now(),
+        .pid = processId_,
+        .tid = oscompat::getCurrentThreadId(),
+    });
+  }
 
-  performanceMeasureCount_ = 0;
-  tracing_ = false;
+  // Potential increments of this counter are covered by tracing_ atomic flag.
+  performanceMeasureCount_.store(0, std::memory_order_relaxed);
   return true;
 }
 
 void PerformanceTracer::collectEvents(
     const std::function<void(const folly::dynamic& eventsChunk)>&
         resultCallback,
     uint16_t chunkSize) {
-  std::lock_guard lock(mutex_);
+  std::lock_guard lock(bufferMutex_);
 
   if (buffer_.empty()) {
     return;
@@ -110,15 +107,11 @@ void PerformanceTracer::collectEvents(
 void PerformanceTracer::reportMark(
     const std::string_view& name,
     HighResTimeStamp start) {
-  if (!tracing_) {
-    return;
-  }
-
-  std::lock_guard<std::mutex> lock(mutex_);
-  if (!tracing_) {
+  if (!isTracing()) {
     return;
   }
 
+  std::lock_guard<std::mutex> lock(bufferMutex_);
   buffer_.push_back(TraceEvent{
       .name = std::string(name),
       .cat = "blink.user_timing",
@@ -134,12 +127,7 @@ void PerformanceTracer::reportMeasure(
     HighResTimeStamp start,
     HighResDuration duration,
     const std::optional<DevToolsTrackEntryPayload>& trackMetadata) {
-  if (!tracing_) {
-    return;
-  }
-
-  std::lock_guard<std::mutex> lock(mutex_);
-  if (!tracing_) {
+  if (!isTracing()) {
     return;
   }
 
@@ -152,10 +140,12 @@ void PerformanceTracer::reportMeasure(
         folly::dynamic::object("detail", folly::toJson(devtoolsObject));
   }
 
-  ++performanceMeasureCount_;
+  auto eventId = ++performanceMeasureCount_;
   auto currentThreadId = oscompat::getCurrentThreadId();
+
+  std::lock_guard<std::mutex> lock(bufferMutex_);
   buffer_.push_back(TraceEvent{
-      .id = performanceMeasureCount_,
+      .id = eventId,
       .name = std::string(name),
       .cat = "blink.user_timing",
       .ph = 'b',
@@ -165,7 +155,7 @@ void PerformanceTracer::reportMeasure(
       .args = beginEventArgs,
   });
   buffer_.push_back(TraceEvent{
-      .id = performanceMeasureCount_,
+      .id = eventId,
       .name = std::string(name),
       .cat = "blink.user_timing",
       .ph = 'e',
@@ -176,15 +166,11 @@ void PerformanceTracer::reportMeasure(
 }
 
 void PerformanceTracer::reportProcess(uint64_t id, const std::string& name) {
-  if (!tracing_) {
-    return;
-  }
-
-  std::lock_guard<std::mutex> lock(mutex_);
-  if (!tracing_) {
+  if (!isTracing()) {
     return;
   }
 
+  std::lock_guard<std::mutex> lock(bufferMutex_);
   buffer_.push_back(TraceEvent{
       .name = "process_name",
       .cat = "__metadata",
@@ -201,15 +187,11 @@ void PerformanceTracer::reportJavaScriptThread() {
 }
 
 void PerformanceTracer::reportThread(uint64_t id, const std::string& name) {
-  if (!tracing_) {
-    return;
-  }
-
-  std::lock_guard<std::mutex> lock(mutex_);
-  if (!tracing_) {
+  if (!isTracing()) {
     return;
   }
 
+  std::lock_guard<std::mutex> lock(bufferMutex_);
   buffer_.push_back(TraceEvent{
       .name = "thread_name",
       .cat = "__metadata",
@@ -238,15 +220,11 @@ void PerformanceTracer::reportThread(uint64_t id, const std::string& name) {
 void PerformanceTracer::reportEventLoopTask(
     HighResTimeStamp start,
     HighResTimeStamp end) {
-  if (!tracing_) {
-    return;
-  }
-
-  std::lock_guard lock(mutex_);
-  if (!tracing_) {
+  if (!isTracing()) {
     return;
   }
 
+  std::lock_guard lock(bufferMutex_);
   buffer_.push_back(TraceEvent{
       .name = "RunTask",
       .cat = "disabled-by-default-devtools.timeline",
@@ -261,15 +239,11 @@ void PerformanceTracer::reportEventLoopTask(
 void PerformanceTracer::reportEventLoopMicrotasks(
     HighResTimeStamp start,
     HighResTimeStamp end) {
-  if (!tracing_) {
-    return;
-  }
-
-  std::lock_guard lock(mutex_);
-  if (!tracing_) {
+  if (!isTracing()) {
     return;
   }
 
+  std::lock_guard lock(bufferMutex_);
   buffer_.push_back(TraceEvent{
       .name = "RunMicrotasks",
       .cat = "v8.execute",

packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.h

@@ -14,6 +14,7 @@
 #include <react/timing/primitives.h>
 
 #include <folly/dynamic.h>
+#include <atomic>
 #include <functional>
 #include <mutex>
 #include <optional>
@@ -47,10 +48,8 @@ class PerformanceTracer {
    * avoid doing expensive work (like formatting strings) if tracing is not
    * enabled.
    */
-  bool isTracing() const {
-    // This is not thread safe but it's only a performance optimization. The
-    // call to report marks and measures is already thread safe.
-    return tracing_;
+  inline bool isTracing() const {
+    return tracing_.load(std::memory_order_relaxed);
   }
 
   /**
@@ -135,12 +134,13 @@ class PerformanceTracer {
 
   folly::dynamic serializeTraceEvent(const TraceEvent& event) const;
 
-  bool tracing_{false};
-
   uint64_t processId_;
-  uint32_t performanceMeasureCount_{0};
+
+  std::atomic<bool> tracing_{false};
+  std::atomic<uint32_t> performanceMeasureCount_{0};
+
   std::vector<TraceEvent> buffer_;
-  std::mutex mutex_;
+  std::mutex bufferMutex_;
 };
 
 } // namespace facebook::react::jsinspector_modern::tracing