refactor: well-defined behaviour (facebook#52187)

Summary:
Pull Request resolved: facebook#52187

# Changelog: [Internal]

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

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

@@ -28,18 +28,20 @@ PerformanceTracer::PerformanceTracer()
 
 bool PerformanceTracer::startTracing() {
   {
-    std::lock_guard lock(mutex_);
-    if (tracing_) {
+    std::lock_guard lock(tracingMutex_);
+    if (isTracing()) {
       return false;
     }
-
     tracing_ = true;
   }
 
   reportProcess(processId_, "React Native");
 
   {
-    std::lock_guard lock(mutex_);
+    std::lock_guard lock(tracingMutex_);
+    if (!isTracing()) {
+      return false;
+    }
     buffer_.push_back(TraceEvent{
         .name = "TracingStartedInPage",
         .cat = "disabled-by-default-devtools.timeline",
@@ -49,16 +51,17 @@ 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_) {
+  std::lock_guard lock(tracingMutex_);
+  if (!isTracing()) {
     return false;
   }
+  tracing_ = false;
 
   // This is synthetic Trace Event, which should not be represented on a
   // timeline. CDT is not using Profile or ProfileChunk events for determining
@@ -75,16 +78,16 @@ bool PerformanceTracer::stopTracing() {
       .tid = oscompat::getCurrentThreadId(),
   });
 
+  // Potential increments of this counter are covered by tracing_ atomic flag.
   performanceMeasureCount_ = 0;
-  tracing_ = false;
   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(tracingMutex_);
 
   if (buffer_.empty()) {
     return;
@@ -110,12 +113,12 @@ void PerformanceTracer::collectEvents(
 void PerformanceTracer::reportMark(
     const std::string_view& name,
     HighResTimeStamp start) {
-  if (!tracing_) {
+  if (!isTracing()) {
     return;
   }
 
-  std::lock_guard<std::mutex> lock(mutex_);
-  if (!tracing_) {
+  std::lock_guard<std::mutex> lock(tracingMutex_);
+  if (!isTracing()) {
     return;
   }
 
@@ -134,12 +137,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 +150,16 @@ void PerformanceTracer::reportMeasure(
         folly::dynamic::object("detail", folly::toJson(devtoolsObject));
   }
 
-  ++performanceMeasureCount_;
   auto currentThreadId = oscompat::getCurrentThreadId();
+
+  std::lock_guard<std::mutex> lock(tracingMutex_);
+  if (!isTracing()) {
+    return;
+  }
+  auto eventId = ++performanceMeasureCount_;
+
   buffer_.push_back(TraceEvent{
-      .id = performanceMeasureCount_,
+      .id = eventId,
       .name = std::string(name),
       .cat = "blink.user_timing",
       .ph = 'b',
@@ -165,7 +169,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,12 +180,12 @@ void PerformanceTracer::reportMeasure(
 }
 
 void PerformanceTracer::reportProcess(uint64_t id, const std::string& name) {
-  if (!tracing_) {
+  if (!isTracing()) {
     return;
   }
 
-  std::lock_guard<std::mutex> lock(mutex_);
-  if (!tracing_) {
+  std::lock_guard<std::mutex> lock(tracingMutex_);
+  if (!isTracing()) {
     return;
   }
 
@@ -201,12 +205,12 @@ void PerformanceTracer::reportJavaScriptThread() {
 }
 
 void PerformanceTracer::reportThread(uint64_t id, const std::string& name) {
-  if (!tracing_) {
+  if (!isTracing()) {
     return;
   }
 
-  std::lock_guard<std::mutex> lock(mutex_);
-  if (!tracing_) {
+  std::lock_guard<std::mutex> lock(tracingMutex_);
+  if (!isTracing()) {
     return;
   }
 
@@ -238,12 +242,12 @@ void PerformanceTracer::reportThread(uint64_t id, const std::string& name) {
 void PerformanceTracer::reportEventLoopTask(
     HighResTimeStamp start,
     HighResTimeStamp end) {
-  if (!tracing_) {
+  if (!isTracing()) {
     return;
   }
 
-  std::lock_guard lock(mutex_);
-  if (!tracing_) {
+  std::lock_guard<std::mutex> lock(tracingMutex_);
+  if (!isTracing()) {
     return;
   }
 
@@ -261,12 +265,12 @@ void PerformanceTracer::reportEventLoopTask(
 void PerformanceTracer::reportEventLoopMicrotasks(
     HighResTimeStamp start,
     HighResTimeStamp end) {
-  if (!tracing_) {
+  if (!isTracing()) {
     return;
   }
 
-  std::lock_guard lock(mutex_);
-  if (!tracing_) {
+  std::lock_guard<std::mutex> lock(tracingMutex_);
+  if (!isTracing()) {
     return;
   }
 

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,9 +48,7 @@ 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.
+  inline bool isTracing() const {
     return tracing_;
   }
 
@@ -135,12 +134,14 @@ 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_;
+  // Protects buffer_ operations and tracing_ modifications.
+  std::mutex tracingMutex_;
 };
 
 } // namespace facebook::react::jsinspector_modern::tracing