Fix measure not using the last reported mark with a given name (faceb…

…ook#43703)

Summary:

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.

Differential Revision: D55477743

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<double>,
-    std::optional<double>,
-    std::optional<uint32_t>>;
+    /* processingStart */ std::optional<double>,
+    /* processingEnd */ std::optional<double>,
+    /* interactionId */ std::optional<uint32_t>>;
 
 template <>
 struct Bridging<RawPerformanceEntry>

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) {

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<RawPerformanceEntry> expected = {
       {"mark0",
@@ -122,7 +124,15 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestReportMarks) {
        0.0,
        std::nullopt,
        std::nullopt,
-       std::nullopt}};
+       std::nullopt},
+      {"mark0",
+       static_cast<int>(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<int>(PerformanceEntryType::MEASURE),
+       1.0,
+       2.0,
+       std::nullopt,
+       std::nullopt,
+       std::nullopt},
       {"measure3",
        static_cast<int>(PerformanceEntryType::MEASURE),
        1.0,
@@ -243,7 +263,13 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestReportMeasures) {
        std::nullopt,
        std::nullopt,
        std::nullopt},
-  };
+      {"mark4",
+       static_cast<int>(PerformanceEntryType::MARK),
+       3.0,
+       0.0,
+       std::nullopt,
+       std::nullopt,
+       std::nullopt}};
 
   ASSERT_EQ(expected, entries);
 }