From 20598949c86cd5d4b09c0acdc2de16c8f7634f46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Tue, 2 Apr 2024 06:27:05 -0700 Subject: [PATCH] Fix measure not using the last reported mark with a given name (#43703) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/43703 Changelog: [internal] (internal because this API isn't available in OSS yet) I found a bug in the current implementation of `performance.measure` where the API would use the first `mark` reported under a specific name instead of the last one (found it in the new example in RNTester in D55477746 that re-logs the marks every time we click on a button). The root cause for this problem is that we were using `insert` from `std::unordered_set` to update the value, but `insert` doesn't modify the value if it's already present. This fixes the issue by doing a lookup and removing the value prior to inserting it. Reviewed By: rshest Differential Revision: D55477743 fbshipit-source-id: e72aa784a936828db64b572988fe0acb2ad78214 --- .../NativePerformanceObserver.h | 15 +++++---- .../PerformanceEntryReporter.cpp | 9 +++++- .../tests/PerformanceEntryReporterTest.cpp | 32 +++++++++++++++++-- 3 files changed, 45 insertions(+), 11 deletions(-) diff --git a/packages/react-native/ReactCommon/react/nativemodule/webperformance/NativePerformanceObserver.h b/packages/react-native/ReactCommon/react/nativemodule/webperformance/NativePerformanceObserver.h index f33d4586ebd..8271bde666e 100644 --- a/packages/react-native/ReactCommon/react/nativemodule/webperformance/NativePerformanceObserver.h +++ b/packages/react-native/ReactCommon/react/nativemodule/webperformance/NativePerformanceObserver.h @@ -21,14 +21,15 @@ class PerformanceEntryReporter; using RawPerformanceEntryType = int32_t; using RawPerformanceEntry = NativePerformanceObserverCxxRawPerformanceEntry< - std::string, - RawPerformanceEntryType, - double, - double, + /* name */ std::string, + /* type */ RawPerformanceEntryType, + /* startTime */ double, + /* duration */ double, + // For "event" entries only: - std::optional, - std::optional, - std::optional>; + /* processingStart */ std::optional, + /* processingEnd */ std::optional, + /* interactionId */ std::optional>; template <> struct Bridging diff --git a/packages/react-native/ReactCommon/react/nativemodule/webperformance/PerformanceEntryReporter.cpp b/packages/react-native/ReactCommon/react/nativemodule/webperformance/PerformanceEntryReporter.cpp index e9fc0e03b25..1b1b376ebce 100644 --- a/packages/react-native/ReactCommon/react/nativemodule/webperformance/PerformanceEntryReporter.cpp +++ b/packages/react-native/ReactCommon/react/nativemodule/webperformance/PerformanceEntryReporter.cpp @@ -113,6 +113,8 @@ void PerformanceEntryReporter::logEntry(const RawPerformanceEntry& entry) { } if (buffer.hasNameLookup) { + // If we need to remove an entry because the buffer is null, + // we also need to remove it from the name lookup. auto overwriteCandidate = buffer.entries.getNextOverwriteCandidate(); if (overwriteCandidate != nullptr) { std::lock_guard lock2(nameLookupMutex_); @@ -134,7 +136,12 @@ void PerformanceEntryReporter::logEntry(const RawPerformanceEntry& entry) { if (buffer.hasNameLookup) { std::lock_guard lock2(nameLookupMutex_); - buffer.nameLookup.insert(&buffer.entries.back()); + auto currentEntry = &buffer.entries.back(); + auto it = buffer.nameLookup.find(currentEntry); + if (it != buffer.nameLookup.end()) { + buffer.nameLookup.erase(it); + } + buffer.nameLookup.insert(currentEntry); } if (buffer.entries.getNumToConsume() == 1) { diff --git a/packages/react-native/ReactCommon/react/nativemodule/webperformance/tests/PerformanceEntryReporterTest.cpp b/packages/react-native/ReactCommon/react/nativemodule/webperformance/tests/PerformanceEntryReporterTest.cpp index c04e97995f3..05ac75203d9 100644 --- a/packages/react-native/ReactCommon/react/nativemodule/webperformance/tests/PerformanceEntryReporterTest.cpp +++ b/packages/react-native/ReactCommon/react/nativemodule/webperformance/tests/PerformanceEntryReporterTest.cpp @@ -94,12 +94,14 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestReportMarks) { reporter.mark("mark0", 0.0); reporter.mark("mark1", 1.0); reporter.mark("mark2", 2.0); + // Report mark0 again + reporter.mark("mark0", 3.0); auto res = reporter.popPendingEntries(); const auto& entries = res.entries; ASSERT_EQ(0, res.droppedEntriesCount); - ASSERT_EQ(3, entries.size()); + ASSERT_EQ(4, entries.size()); const std::vector expected = { {"mark0", @@ -122,7 +124,15 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestReportMarks) { 0.0, std::nullopt, std::nullopt, - std::nullopt}}; + std::nullopt}, + {"mark0", + static_cast(PerformanceEntryType::MARK), + 3.0, + 0.0, + std::nullopt, + std::nullopt, + std::nullopt}, + }; ASSERT_EQ(expected, entries); } @@ -152,6 +162,9 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestReportMeasures) { reporter.mark("mark3", 2.0); reporter.measure("measure6", 2.0, 2.0); reporter.mark("mark4", 2.0); + reporter.mark("mark4", 3.0); + // Uses the last reported time for mark4 + reporter.measure("measure7", 0.0, 0.0, std::nullopt, "mark1", "mark4"); auto res = reporter.popPendingEntries(); const auto& entries = res.entries; @@ -194,6 +207,13 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestReportMeasures) { std::nullopt, std::nullopt, std::nullopt}, + {"measure7", + static_cast(PerformanceEntryType::MEASURE), + 1.0, + 2.0, + std::nullopt, + std::nullopt, + std::nullopt}, {"measure3", static_cast(PerformanceEntryType::MEASURE), 1.0, @@ -243,7 +263,13 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestReportMeasures) { std::nullopt, std::nullopt, std::nullopt}, - }; + {"mark4", + static_cast(PerformanceEntryType::MARK), + 3.0, + 0.0, + std::nullopt, + std::nullopt, + std::nullopt}}; ASSERT_EQ(expected, entries); }