From dec8562494effb0e2b11878b40ce72e91db5445f Mon Sep 17 00:00:00 2001 From: Ruslan Shestopalyuk Date: Thu, 20 Apr 2023 06:31:27 -0700 Subject: [PATCH] Clean up uneeded "duration" parameter from the Performance.mark API (#36997) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/36997 I've noticed that `Performance.mark`, for some reason, has been using an explicit `duration` parameter throughout, whereas it doesn't really make sense - neither from the web standard perspective, nor in general. Changelog: [Internal] Reviewed By: rubennorte Differential Revision: D45141978 fbshipit-source-id: ce3d149401249882f673c4bb6727eb1560469fa3 --- .../WebPerformance/NativePerformance.cpp | 5 +- .../WebPerformance/NativePerformance.h | 3 +- .../WebPerformance/NativePerformance.js | 2 +- .../Libraries/WebPerformance/Performance.js | 2 +- .../PerformanceEntryReporter.cpp | 17 +++--- .../WebPerformance/PerformanceEntryReporter.h | 4 +- .../__mocks__/NativePerformance.js | 4 +- .../PerformanceEntryReporterTest.cpp | 56 +++++++++---------- 8 files changed, 48 insertions(+), 45 deletions(-) diff --git a/packages/react-native/Libraries/WebPerformance/NativePerformance.cpp b/packages/react-native/Libraries/WebPerformance/NativePerformance.cpp index d08810da47c..e9ccdf332b6 100644 --- a/packages/react-native/Libraries/WebPerformance/NativePerformance.cpp +++ b/packages/react-native/Libraries/WebPerformance/NativePerformance.cpp @@ -28,9 +28,8 @@ NativePerformance::NativePerformance(std::shared_ptr jsInvoker) void NativePerformance::mark( jsi::Runtime &rt, std::string name, - double startTime, - double duration) { - PerformanceEntryReporter::getInstance().mark(name, startTime, duration); + double startTime) { + PerformanceEntryReporter::getInstance().mark(name, startTime); } void NativePerformance::measure( diff --git a/packages/react-native/Libraries/WebPerformance/NativePerformance.h b/packages/react-native/Libraries/WebPerformance/NativePerformance.h index 4b334d7339b..1726bb663f3 100644 --- a/packages/react-native/Libraries/WebPerformance/NativePerformance.h +++ b/packages/react-native/Libraries/WebPerformance/NativePerformance.h @@ -41,8 +41,7 @@ class NativePerformance : public NativePerformanceCxxSpec, public: NativePerformance(std::shared_ptr jsInvoker); - void - mark(jsi::Runtime &rt, std::string name, double startTime, double duration); + void mark(jsi::Runtime &rt, std::string name, double startTime); void measure( jsi::Runtime &rt, diff --git a/packages/react-native/Libraries/WebPerformance/NativePerformance.js b/packages/react-native/Libraries/WebPerformance/NativePerformance.js index 9a34b231299..092b4ff6875 100644 --- a/packages/react-native/Libraries/WebPerformance/NativePerformance.js +++ b/packages/react-native/Libraries/WebPerformance/NativePerformance.js @@ -22,7 +22,7 @@ export type ReactNativeStartupTiming = {| |}; export interface Spec extends TurboModule { - +mark: (name: string, startTime: number, duration: number) => void; + +mark: (name: string, startTime: number) => void; +measure: ( name: string, startTime: number, diff --git a/packages/react-native/Libraries/WebPerformance/Performance.js b/packages/react-native/Libraries/WebPerformance/Performance.js index 8e2ce32bd82..5b6fc03a183 100644 --- a/packages/react-native/Libraries/WebPerformance/Performance.js +++ b/packages/react-native/Libraries/WebPerformance/Performance.js @@ -157,7 +157,7 @@ export default class Performance { const mark = new PerformanceMark(markName, markOptions); if (NativePerformance?.mark) { - NativePerformance.mark(markName, mark.startTime, mark.duration); + NativePerformance.mark(markName, mark.startTime); } else { warnNoNativePerformance(); } diff --git a/packages/react-native/Libraries/WebPerformance/PerformanceEntryReporter.cpp b/packages/react-native/Libraries/WebPerformance/PerformanceEntryReporter.cpp index 024178f344a..8c82a5c2a9d 100644 --- a/packages/react-native/Libraries/WebPerformance/PerformanceEntryReporter.cpp +++ b/packages/react-native/Libraries/WebPerformance/PerformanceEntryReporter.cpp @@ -15,6 +15,10 @@ namespace facebook::react { EventTag PerformanceEntryReporter::sCurrentEventTag_{0}; +static inline double getCurrentTimeStamp() { + return JSExecutor::performanceNow(); +} + PerformanceEntryReporter &PerformanceEntryReporter::getInstance() { static PerformanceEntryReporter instance; return instance; @@ -134,13 +138,12 @@ void PerformanceEntryReporter::logEntry(const RawPerformanceEntry &entry) { void PerformanceEntryReporter::mark( const std::string &name, - double startTime, - double duration) { + const std::optional &startTime) { logEntry(RawPerformanceEntry{ name, static_cast(PerformanceEntryType::MARK), - startTime, - duration, + startTime ? *startTime : getCurrentTimeStamp(), + 0.0, std::nullopt, std::nullopt, std::nullopt}); @@ -352,7 +355,7 @@ EventTag PerformanceEntryReporter::onEventStart(const char *name) { sCurrentEventTag_ = 1; } - auto timeStamp = JSExecutor::performanceNow(); + auto timeStamp = getCurrentTimeStamp(); { std::lock_guard lock(eventsInFlightMutex_); eventsInFlight_.emplace(std::make_pair( @@ -365,7 +368,7 @@ void PerformanceEntryReporter::onEventDispatch(EventTag tag) { if (!isReporting(PerformanceEntryType::EVENT) || tag == 0) { return; } - auto timeStamp = JSExecutor::performanceNow(); + auto timeStamp = getCurrentTimeStamp(); { std::lock_guard lock(eventsInFlightMutex_); auto it = eventsInFlight_.find(tag); @@ -379,7 +382,7 @@ void PerformanceEntryReporter::onEventEnd(EventTag tag) { if (!isReporting(PerformanceEntryType::EVENT) || tag == 0) { return; } - auto timeStamp = JSExecutor::performanceNow(); + auto timeStamp = getCurrentTimeStamp(); { std::lock_guard lock(eventsInFlightMutex_); auto it = eventsInFlight_.find(tag); diff --git a/packages/react-native/Libraries/WebPerformance/PerformanceEntryReporter.h b/packages/react-native/Libraries/WebPerformance/PerformanceEntryReporter.h index 4ce7794078f..8ea7c820e01 100644 --- a/packages/react-native/Libraries/WebPerformance/PerformanceEntryReporter.h +++ b/packages/react-native/Libraries/WebPerformance/PerformanceEntryReporter.h @@ -111,7 +111,9 @@ class PerformanceEntryReporter : public EventLogger { return droppedEntryCount_; } - void mark(const std::string &name, double startTime, double duration); + void mark( + const std::string &name, + const std::optional &startTime = std::nullopt); void measure( const std::string &name, diff --git a/packages/react-native/Libraries/WebPerformance/__mocks__/NativePerformance.js b/packages/react-native/Libraries/WebPerformance/__mocks__/NativePerformance.js index 6b11a27a20a..b3335e0c774 100644 --- a/packages/react-native/Libraries/WebPerformance/__mocks__/NativePerformance.js +++ b/packages/react-native/Libraries/WebPerformance/__mocks__/NativePerformance.js @@ -20,12 +20,12 @@ import {RawPerformanceEntryTypeValues} from '../RawPerformanceEntry'; const marks: Map = new Map(); const NativePerformanceMock: NativePerformance = { - mark: (name: string, startTime: number, duration: number): void => { + mark: (name: string, startTime: number): void => { NativePerformanceObserver?.logRawEntry({ name, entryType: RawPerformanceEntryTypeValues.MARK, startTime, - duration, + duration: 0, }); marks.set(name, startTime); }, diff --git a/packages/react-native/Libraries/WebPerformance/__tests__/PerformanceEntryReporterTest.cpp b/packages/react-native/Libraries/WebPerformance/__tests__/PerformanceEntryReporterTest.cpp index 9a6842eec2c..fb2dabc0d96 100644 --- a/packages/react-native/Libraries/WebPerformance/__tests__/PerformanceEntryReporterTest.cpp +++ b/packages/react-native/Libraries/WebPerformance/__tests__/PerformanceEntryReporterTest.cpp @@ -54,9 +54,9 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestStopReporting) { reporter.startReporting(PerformanceEntryType::MARK); - reporter.mark("mark0", 0.0, 0.0); - reporter.mark("mark1", 0.0, 0.0); - reporter.mark("mark2", 0.0, 0.0); + reporter.mark("mark0", 0.0); + reporter.mark("mark1", 0.0); + reporter.mark("mark2", 0.0); reporter.measure("measure0", 0.0, 0.0); auto res = reporter.popPendingEntries(); @@ -73,7 +73,7 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestStopReporting) { reporter.stopReporting(PerformanceEntryType::MARK); reporter.startReporting(PerformanceEntryType::MEASURE); - reporter.mark("mark3", 0.0, 0.0); + reporter.mark("mark3"); reporter.measure("measure1", 0.0, 0.0); res = reporter.popPendingEntries(); @@ -91,9 +91,9 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestReportMarks) { reporter.startReporting(PerformanceEntryType::MARK); - reporter.mark("mark0", 0.0, 1.0); - reporter.mark("mark1", 1.0, 3.0); - reporter.mark("mark2", 2.0, 4.0); + reporter.mark("mark0", 0.0); + reporter.mark("mark1", 1.0); + reporter.mark("mark2", 2.0); auto res = reporter.popPendingEntries(); const auto &entries = res.entries; @@ -105,21 +105,21 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestReportMarks) { {"mark0", static_cast(PerformanceEntryType::MARK), 0.0, - 1.0, + 0.0, std::nullopt, std::nullopt, std::nullopt}, {"mark1", static_cast(PerformanceEntryType::MARK), 1.0, - 3.0, + 0.0, std::nullopt, std::nullopt, std::nullopt}, {"mark2", static_cast(PerformanceEntryType::MARK), 2.0, - 4.0, + 0.0, std::nullopt, std::nullopt, std::nullopt}}; @@ -136,9 +136,9 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestReportMeasures) { reporter.startReporting(PerformanceEntryType::MARK); reporter.startReporting(PerformanceEntryType::MEASURE); - reporter.mark("mark0", 0.0, 1.0); - reporter.mark("mark1", 1.0, 3.0); - reporter.mark("mark2", 2.0, 4.0); + reporter.mark("mark0", 0.0); + reporter.mark("mark1", 1.0); + reporter.mark("mark2", 2.0); reporter.measure("measure0", 0.0, 2.0); reporter.measure("measure1", 0.0, 2.0, 4.0); @@ -155,7 +155,7 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestReportMeasures) { {"mark0", static_cast(PerformanceEntryType::MARK), 0.0, - 1.0, + 0.0, std::nullopt, std::nullopt, std::nullopt}, @@ -173,6 +173,13 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestReportMeasures) { std::nullopt, std::nullopt, std::nullopt}, + {"mark1", + static_cast(PerformanceEntryType::MARK), + 1.0, + 0.0, + std::nullopt, + std::nullopt, + std::nullopt}, {"measure2", static_cast(PerformanceEntryType::MEASURE), 1.0, @@ -180,13 +187,6 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestReportMeasures) { std::nullopt, std::nullopt, std::nullopt}, - {"mark1", - static_cast(PerformanceEntryType::MARK), - 1.0, - 3.0, - std::nullopt, - std::nullopt, - std::nullopt}, {"measure3", static_cast(PerformanceEntryType::MEASURE), 1.0, @@ -204,7 +204,7 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestReportMeasures) { {"mark2", static_cast(PerformanceEntryType::MARK), 2.0, - 4.0, + 0.0, std::nullopt, std::nullopt, std::nullopt}}; @@ -249,9 +249,9 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestGetEntries) { reporter.startReporting(PerformanceEntryType::MARK); reporter.startReporting(PerformanceEntryType::MEASURE); - reporter.mark("common_name", 0.0, 1.0); - reporter.mark("mark1", 1.0, 3.0); - reporter.mark("mark2", 2.0, 4.0); + reporter.mark("common_name", 0.0); + reporter.mark("mark1", 1.0); + reporter.mark("mark2", 2.0); reporter.measure("common_name", 0.0, 2.0); reporter.measure("measure1", 0.0, 2.0, 4.0); @@ -296,9 +296,9 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestClearEntries) { reporter.startReporting(PerformanceEntryType::MARK); reporter.startReporting(PerformanceEntryType::MEASURE); - reporter.mark("common_name", 0.0, 1.0); - reporter.mark("mark1", 1.0, 3.0); - reporter.mark("mark2", 2.0, 4.0); + reporter.mark("common_name", 0.0); + reporter.mark("mark1", 1.0); + reporter.mark("mark2", 2.0); reporter.measure("common_name", 0.0, 2.0); reporter.measure("measure1", 0.0, 2.0, 4.0);