From 2fcc403b11d008a61babfb1091cfa2dbdc329885 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 25 Jun 2025 04:03:48 -0700 Subject: [PATCH 1/4] refactor: well-defined behaviour (#52187) Summary: # Changelog: [Internal] - `bool tracing_` -> `std::atomic tracingAtomic_`. - More doc-comments to explain the usage of mutex and atomics. - `PerformanceTracer::isTracing()` -> `inline PerformanceTracer::isTracing()`. - `uint64_t processId_` -> `const uint64_t processId_`. The main change is that the boolean flag that controls "if we are tracing" is now atomic, which should eliminate potential data races. To avoid "logic" races, we are still going to lock mutex, and then check again. The use of `std::atomic` allows us to perform cheaper check first to avoid potentially unnecessary serializations from other systems that report events into `PerformanceTracer`. Reviewed By: rubennorte Differential Revision: D77053030 --- .../tracing/PerformanceTracer.cpp | 72 ++++++++++--------- .../tracing/PerformanceTracer.h | 15 ++-- 2 files changed, 46 insertions(+), 41 deletions(-) diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.cpp index eb67ae7fb672ce..b77ab41dab0ad9 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.cpp +++ b/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,8 +78,8 @@ bool PerformanceTracer::stopTracing() { .tid = oscompat::getCurrentThreadId(), }); + // Potential increments of this counter are covered by tracing_ atomic flag. performanceMeasureCount_ = 0; - tracing_ = false; return true; } @@ -84,7 +87,7 @@ void PerformanceTracer::collectEvents( const std::function& 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 lock(mutex_); - if (!tracing_) { + std::lock_guard lock(tracingMutex_); + if (!isTracing()) { return; } @@ -134,12 +137,7 @@ void PerformanceTracer::reportMeasure( HighResTimeStamp start, HighResDuration duration, const std::optional& trackMetadata) { - if (!tracing_) { - return; - } - - std::lock_guard 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 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 lock(mutex_); - if (!tracing_) { + std::lock_guard 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 lock(mutex_); - if (!tracing_) { + std::lock_guard 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 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 lock(tracingMutex_); + if (!isTracing()) { return; } diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.h b/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.h index 9acc978dfc759a..890b9b1f1406bb 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.h @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -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 tracing_{false}; + std::atomic performanceMeasureCount_{0}; + std::vector buffer_; - std::mutex mutex_; + // Protects buffer_ operations and tracing_ modifications. + std::mutex tracingMutex_; }; } // namespace facebook::react::jsinspector_modern::tracing From 95ffba48dff953e6befbf2a0ed4177ccb76757d2 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 25 Jun 2025 04:03:48 -0700 Subject: [PATCH 2/4] Avoid potential copies of TraceEvent when buffering (#52188) Summary: # Changelog: [Internal] `buffer_.push_back` -> `buffer_.emplace_back` I didn't measure if there were any runtime wins from this, because I don't expect there would be. Let's avoid potential copies, if possible. Reviewed By: rubennorte Differential Revision: D77053032 --- .../tracing/PerformanceTracer.cpp | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.cpp index b77ab41dab0ad9..0dc058f8967c51 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.cpp @@ -42,7 +42,7 @@ bool PerformanceTracer::startTracing() { if (!isTracing()) { return false; } - buffer_.push_back(TraceEvent{ + buffer_.emplace_back(TraceEvent{ .name = "TracingStartedInPage", .cat = "disabled-by-default-devtools.timeline", .ph = 'I', @@ -69,7 +69,7 @@ bool PerformanceTracer::stopTracing() { // 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{ + buffer_.emplace_back(TraceEvent{ .name = "ReactNative-TracingStopped", .cat = "disabled-by-default-devtools.timeline", .ph = 'I', @@ -122,7 +122,7 @@ void PerformanceTracer::reportMark( return; } - buffer_.push_back(TraceEvent{ + buffer_.emplace_back(TraceEvent{ .name = std::string(name), .cat = "blink.user_timing", .ph = 'I', @@ -158,7 +158,7 @@ void PerformanceTracer::reportMeasure( } auto eventId = ++performanceMeasureCount_; - buffer_.push_back(TraceEvent{ + buffer_.emplace_back(TraceEvent{ .id = eventId, .name = std::string(name), .cat = "blink.user_timing", @@ -168,7 +168,7 @@ void PerformanceTracer::reportMeasure( .tid = currentThreadId, .args = beginEventArgs, }); - buffer_.push_back(TraceEvent{ + buffer_.emplace_back(TraceEvent{ .id = eventId, .name = std::string(name), .cat = "blink.user_timing", @@ -189,7 +189,7 @@ void PerformanceTracer::reportProcess(uint64_t id, const std::string& name) { return; } - buffer_.push_back(TraceEvent{ + buffer_.emplace_back(TraceEvent{ .name = "process_name", .cat = "__metadata", .ph = 'M', @@ -214,7 +214,7 @@ void PerformanceTracer::reportThread(uint64_t id, const std::string& name) { return; } - buffer_.push_back(TraceEvent{ + buffer_.emplace_back(TraceEvent{ .name = "thread_name", .cat = "__metadata", .ph = 'M', @@ -229,7 +229,7 @@ void PerformanceTracer::reportThread(uint64_t id, const std::string& name) { // no timeline events or user timings. 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{ + buffer_.emplace_back(TraceEvent{ .name = "ReactNative-ThreadRegistered", .cat = "disabled-by-default-devtools.timeline", .ph = 'I', @@ -251,7 +251,7 @@ void PerformanceTracer::reportEventLoopTask( return; } - buffer_.push_back(TraceEvent{ + buffer_.emplace_back(TraceEvent{ .name = "RunTask", .cat = "disabled-by-default-devtools.timeline", .ph = 'X', @@ -274,7 +274,7 @@ void PerformanceTracer::reportEventLoopMicrotasks( return; } - buffer_.push_back(TraceEvent{ + buffer_.emplace_back(TraceEvent{ .name = "RunMicrotasks", .cat = "v8.execute", .ph = 'X', From 76f3e81e9c8dd6c73a5ac6cf1ee3b732a7576bb9 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 25 Jun 2025 04:03:48 -0700 Subject: [PATCH 3/4] Avoid potential copies of TraceEvent before serialization (#52196) Summary: # Changelog: [Internal] Probably been overlooked for quite some time, but shouldn't be a bottleneck. Reviewed By: motiz88 Differential Revision: D77148271 --- .../jsinspector-modern/tracing/PerformanceTracer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.cpp index 0dc058f8967c51..07dd7b2624f8b5 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.cpp @@ -94,7 +94,7 @@ void PerformanceTracer::collectEvents( } auto traceEvents = folly::dynamic::array(); - for (auto event : buffer_) { + for (const auto& event : buffer_) { // Emit trace events traceEvents.push_back(serializeTraceEvent(event)); From 925ec209a59487a1890c917262b1c6ff5726a6a4 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 25 Jun 2025 04:03:48 -0700 Subject: [PATCH 4/4] Avoid copying strings when serializing TraceEvent / lock only on buffer operations (#52220) Summary: # Changelog: [Internal] Mainly, 2 changes: 1. `PerformanceTracer::serializeTraceEvent(const TraceEvent& event)` -> `PerformanceTracer::serializeTraceEvent(TraceEvent&& event)` for less copies, actually move strings from the `TraceEvent` into the serialized `folly:object`. 2. When collecting events from the buffer, only lock when accessing buffer, not when serializing. Reviewed By: rubennorte Differential Revision: D77164969 --- .../tracing/PerformanceTracer.cpp | 34 ++++++++++--------- .../tracing/PerformanceTracer.h | 8 ++++- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.cpp index 07dd7b2624f8b5..3112d0e7f70687 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.cpp @@ -87,27 +87,29 @@ void PerformanceTracer::collectEvents( const std::function& resultCallback, uint16_t chunkSize) { - std::lock_guard lock(tracingMutex_); + std::vector localBuffer; + { + std::lock_guard lock(tracingMutex_); + buffer_.swap(localBuffer); + } - if (buffer_.empty()) { + if (localBuffer.empty()) { return; } - auto traceEvents = folly::dynamic::array(); - for (const auto& event : buffer_) { + auto serializedTraceEvents = folly::dynamic::array(); + for (auto&& event : localBuffer) { // Emit trace events - traceEvents.push_back(serializeTraceEvent(event)); + serializedTraceEvents.push_back(serializeTraceEvent(std::move(event))); - if (traceEvents.size() == chunkSize) { - resultCallback(traceEvents); - traceEvents = folly::dynamic::array(); + if (serializedTraceEvents.size() == chunkSize) { + resultCallback(serializedTraceEvents); + serializedTraceEvents = folly::dynamic::array(); } } - if (!traceEvents.empty()) { - resultCallback(traceEvents); + if (!serializedTraceEvents.empty()) { + resultCallback(serializedTraceEvents); } - - buffer_.clear(); } void PerformanceTracer::reportMark( @@ -326,7 +328,7 @@ folly::dynamic PerformanceTracer::getSerializedRuntimeProfileChunkTraceEvent( } folly::dynamic PerformanceTracer::serializeTraceEvent( - const TraceEvent& event) const { + TraceEvent&& event) const { folly::dynamic result = folly::dynamic::object; if (event.id.has_value()) { @@ -334,13 +336,13 @@ folly::dynamic PerformanceTracer::serializeTraceEvent( snprintf(buffer.data(), buffer.size(), "0x%x", event.id.value()); result["id"] = buffer.data(); } - result["name"] = event.name; - result["cat"] = event.cat; + result["name"] = std::move(event.name); + result["cat"] = std::move(event.cat); result["ph"] = std::string(1, event.ph); result["ts"] = highResTimeStampToTracingClockTimeStamp(event.ts); result["pid"] = event.pid; result["tid"] = event.tid; - result["args"] = event.args; + result["args"] = std::move(event.args); if (event.dur.has_value()) { result["dur"] = highResDurationToTracingClockDuration(event.dur.value()); } diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.h b/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.h index 890b9b1f1406bb..5cacb3971afbc8 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/tracing/PerformanceTracer.h @@ -132,7 +132,13 @@ class PerformanceTracer { PerformanceTracer& operator=(const PerformanceTracer&) = delete; ~PerformanceTracer() = default; - folly::dynamic serializeTraceEvent(const TraceEvent& event) const; + /** + * Serialize a TraceEvent into a folly::dynamic object. + * \param event rvalue reference to the TraceEvent object. + * \return folly::dynamic object that represents a serialized into JSON Trace + * Event for CDP. + */ + folly::dynamic serializeTraceEvent(TraceEvent&& event) const; uint64_t processId_;