Bug 1576555 - Don't store temporary ProfilerMarkerPayloads in UniquePtrs - r=gregtatum

Since payloads are not kept alive long after they have been serialized, we can
just create them on the stack and pass a reference to their base (or pointer,
`nullptr` representing "no payloads") to `profiler_add_marker()`.

Differential Revision: https://phabricator.services.mozilla.com/D43430

--HG--
extra : moz-landing-system : lando

mozglue/baseprofiler/core/platform.cpp

@@ -3198,17 +3198,19 @@ void ProfilerBacktraceDestructor::operator()(ProfilerBacktrace* aBacktrace) {
   delete aBacktrace;
 }
 
-static void racy_profiler_add_marker(
-    const char* aMarkerName, ProfilingCategoryPair aCategoryPair,
-    UniquePtr<ProfilerMarkerPayload> aPayload) {
+static void racy_profiler_add_marker(const char* aMarkerName,
+                                     ProfilingCategoryPair aCategoryPair,
+                                     const ProfilerMarkerPayload* aPayload) {
   MOZ_RELEASE_ASSERT(CorePS::Exists());
 
-  // We don't assert that RacyFeatures::IsActiveWithoutPrivacy() or
-  // RacyRegisteredThread::IsBeingProfiled() is true here, because it's
-  // possible that the result has changed since we tested it in the caller.
-  //
-  // Because of this imprecision it's possible to miss a marker or record one
-  // we shouldn't. Either way is not a big deal.
+  // This function is hot enough that we use RacyFeatures, not ActivePS.
+  if (!RacyFeatures::IsActiveWithoutPrivacy()) {
+    return;
+  }
+
+  // Note that it's possible that the above test would change again before we
+  // actually record the marker. Because of this imprecision it's possible to
+  // miss a marker or record one we shouldn't. Either way is not a big deal.
 
   RacyRegisteredThread* racyRegisteredThread =
       TLSRegisteredThread::RacyRegisteredThread();
@@ -3228,27 +3230,20 @@ static void racy_profiler_add_marker(
 
 void profiler_add_marker(const char* aMarkerName,
                          ProfilingCategoryPair aCategoryPair,
-                         UniquePtr<ProfilerMarkerPayload> aPayload) {
-  MOZ_RELEASE_ASSERT(CorePS::Exists());
-
-  // This function is hot enough that we use RacyFeatures, not ActivePS.
-  if (!RacyFeatures::IsActiveWithoutPrivacy()) {
-    return;
-  }
-
-  racy_profiler_add_marker(aMarkerName, aCategoryPair, std::move(aPayload));
+                         const ProfilerMarkerPayload& aPayload) {
+  racy_profiler_add_marker(aMarkerName, aCategoryPair, &aPayload);
 }
 
 void profiler_add_marker(const char* aMarkerName,
                          ProfilingCategoryPair aCategoryPair) {
-  profiler_add_marker(aMarkerName, aCategoryPair, nullptr);
+  racy_profiler_add_marker(aMarkerName, aCategoryPair, nullptr);
 }
 
 // This is a simplified version of profiler_add_marker that can be easily passed
 // into the JS engine.
 void profiler_add_js_marker(const char* aMarkerName) {
   AUTO_PROFILER_STATS(base_add_marker);
-  profiler_add_marker(aMarkerName, ProfilingCategoryPair::JS, nullptr);
+  profiler_add_marker(aMarkerName, ProfilingCategoryPair::JS);
 }
 
 // This logic needs to add a marker for a different thread, so we actually need
@@ -3306,9 +3301,9 @@ void profiler_tracing(const char* aCategoryString, const char* aMarkerName,
   }
 
   AUTO_PROFILER_STATS(base_add_marker_with_TracingMarkerPayload);
-  auto payload = MakeUnique<TracingMarkerPayload>(
-      aCategoryString, aKind, aDocShellId, aDocShellHistoryId);
-  racy_profiler_add_marker(aMarkerName, aCategoryPair, std::move(payload));
+  profiler_add_marker(aMarkerName, aCategoryPair,
+                      TracingMarkerPayload(aCategoryString, aKind, aDocShellId,
+                                           aDocShellHistoryId));
 }
 
 void profiler_tracing(const char* aCategoryString, const char* aMarkerName,
@@ -3326,10 +3321,10 @@ void profiler_tracing(const char* aCategoryString, const char* aMarkerName,
   }
 
   AUTO_PROFILER_STATS(base_add_marker_with_TracingMarkerPayload);
-  auto payload =
-      MakeUnique<TracingMarkerPayload>(aCategoryString, aKind, aDocShellId,
-                                       aDocShellHistoryId, std::move(aCause));
-  racy_profiler_add_marker(aMarkerName, aCategoryPair, std::move(payload));
+  profiler_add_marker(
+      aMarkerName, aCategoryPair,
+      TracingMarkerPayload(aCategoryString, aKind, aDocShellId,
+                           aDocShellHistoryId, std::move(aCause)));
 }
 
 void profiler_add_text_marker(const char* aMarkerName, const std::string& aText,
@@ -3342,7 +3337,7 @@ void profiler_add_text_marker(const char* aMarkerName, const std::string& aText,
   AUTO_PROFILER_STATS(base_add_marker_with_TextMarkerPayload);
   profiler_add_marker(
       aMarkerName, aCategoryPair,
-      MakeUnique<TextMarkerPayload>(aText, aStartTime, aEndTime, aDocShellId,
+      TextMarkerPayload(aText, aStartTime, aEndTime, aDocShellId,
                         aDocShellHistoryId, std::move(aCause)));
 }
 

mozglue/baseprofiler/public/BaseProfiler.h

@@ -712,12 +712,13 @@ MFBT_API void profiler_add_marker(const char* aMarkerName,
       ::mozilla::baseprofiler::profiler_add_marker(                     \
           markerName,                                                   \
           ::mozilla::baseprofiler::ProfilingCategoryPair::categoryPair, \
-          ::mozilla::MakeUnique<PayloadType> parenthesizedPayloadArgs); \
+          PayloadType parenthesizedPayloadArgs);                        \
     } while (false)
 
 MFBT_API void profiler_add_marker(const char* aMarkerName,
                                   ProfilingCategoryPair aCategoryPair,
-                                  UniquePtr<ProfilerMarkerPayload> aPayload);
+                                  const ProfilerMarkerPayload& aPayload);
+
 MFBT_API void profiler_add_js_marker(const char* aMarkerName);
 
 // Insert a marker in the profile timeline for a specified thread.

mozglue/baseprofiler/public/BaseProfilerMarkerPayload.h

@@ -333,6 +333,21 @@ class LogMarkerPayload : public ProfilerMarkerPayload {
 // Serialize a pointed-at ProfilerMarkerPayload, may be null when there are no
 // payloads.
 template <>
+struct BlocksRingBuffer::Serializer<
+    const baseprofiler::ProfilerMarkerPayload*> {
+  static Length Bytes(const baseprofiler::ProfilerMarkerPayload* aPayload) {
+    return baseprofiler::ProfilerMarkerPayload::TagAndSerializationBytes(
+        aPayload);
+  }
+
+  static void Write(EntryWriter& aEW,
+                    const baseprofiler::ProfilerMarkerPayload* aPayload) {
+    baseprofiler::ProfilerMarkerPayload::TagAndSerialize(aPayload, aEW);
+  }
+};
+
+// Serialize a pointed-at ProfilerMarkerPayload, may be null for no payloads.
+template <>
 struct BlocksRingBuffer::Serializer<
     UniquePtr<baseprofiler::ProfilerMarkerPayload>> {
   static Length Bytes(

mozglue/tests/TestBaseProfiler.cpp

@@ -1475,10 +1475,9 @@ void TestProfilerMarkerSerialization() {
 
   constexpr int data = 42;
   {
-    UniquePtr<baseprofiler::ProfilerMarkerPayload> testPayload{
-        new BaseTestMarkerPayload(data)};
-
-    rb.PutObject(testPayload);
+    BaseTestMarkerPayload payload(data);
+    rb.PutObject(
+        static_cast<const baseprofiler::ProfilerMarkerPayload*>(&payload));
   }
 
   int read = 0;
@@ -1644,8 +1643,8 @@ void TestProfiler() {
     // Just making sure all payloads know how to (de)serialize and stream.
     baseprofiler::profiler_add_marker(
         "TracingMarkerPayload", baseprofiler::ProfilingCategoryPair::OTHER,
-        MakeUnique<baseprofiler::TracingMarkerPayload>(
-            "category", baseprofiler::TRACING_EVENT));
+        baseprofiler::TracingMarkerPayload("category",
+                                           baseprofiler::TRACING_EVENT));
 
     auto cause =
 #  if defined(__linux__) || defined(__ANDROID__)
@@ -1656,36 +1655,36 @@ void TestProfiler() {
 #  endif
     baseprofiler::profiler_add_marker(
         "FileIOMarkerPayload", baseprofiler::ProfilingCategoryPair::OTHER,
-        MakeUnique<baseprofiler::FileIOMarkerPayload>(
+        baseprofiler::FileIOMarkerPayload(
             "operation", "source", "filename", TimeStamp::NowUnfuzzed(),
             TimeStamp::NowUnfuzzed(), std::move(cause)));
 
     baseprofiler::profiler_add_marker(
         "UserTimingMarkerPayload", baseprofiler::ProfilingCategoryPair::OTHER,
-        MakeUnique<baseprofiler::UserTimingMarkerPayload>(
-            "name", TimeStamp::NowUnfuzzed(), Nothing{}, Nothing{}));
+        baseprofiler::UserTimingMarkerPayload("name", TimeStamp::NowUnfuzzed(),
+                                              Nothing{}, Nothing{}));
 
     baseprofiler::profiler_add_marker(
         "HangMarkerPayload", baseprofiler::ProfilingCategoryPair::OTHER,
-        MakeUnique<baseprofiler::HangMarkerPayload>(TimeStamp::NowUnfuzzed(),
+        baseprofiler::HangMarkerPayload(TimeStamp::NowUnfuzzed(),
                                         TimeStamp::NowUnfuzzed()));
 
     baseprofiler::profiler_add_marker(
         "LongTaskMarkerPayload", baseprofiler::ProfilingCategoryPair::OTHER,
-        MakeUnique<baseprofiler::LongTaskMarkerPayload>(
-            TimeStamp::NowUnfuzzed(), TimeStamp::NowUnfuzzed()));
+        baseprofiler::LongTaskMarkerPayload(TimeStamp::NowUnfuzzed(),
+                                            TimeStamp::NowUnfuzzed()));
 
     {
       std::string s = "text payload";
       baseprofiler::profiler_add_marker(
           "TextMarkerPayload", baseprofiler::ProfilingCategoryPair::OTHER,
-          MakeUnique<baseprofiler::TextMarkerPayload>(
-              s, TimeStamp::NowUnfuzzed(), TimeStamp::NowUnfuzzed()));
+          baseprofiler::TextMarkerPayload(s, TimeStamp::NowUnfuzzed(),
+                                          TimeStamp::NowUnfuzzed()));
     }
 
     baseprofiler::profiler_add_marker(
         "LogMarkerPayload", baseprofiler::ProfilingCategoryPair::OTHER,
-        MakeUnique<baseprofiler::LogMarkerPayload>("module", "text",
+        baseprofiler::LogMarkerPayload("module", "text",
                                        TimeStamp::NowUnfuzzed()));
 
     printf("Sleep 1s...\n");

tools/profiler/core/platform.cpp

@@ -4067,17 +4067,19 @@ void ProfilerBacktraceDestructor::operator()(ProfilerBacktrace* aBacktrace) {
   delete aBacktrace;
 }
 
-static void racy_profiler_add_marker(
-    const char* aMarkerName, JS::ProfilingCategoryPair aCategoryPair,
-    UniquePtr<ProfilerMarkerPayload> aPayload) {
+static void racy_profiler_add_marker(const char* aMarkerName,
+                                     JS::ProfilingCategoryPair aCategoryPair,
+                                     const ProfilerMarkerPayload* aPayload) {
   MOZ_RELEASE_ASSERT(CorePS::Exists());
 
-  // We don't assert that RacyFeatures::IsActiveWithoutPrivacy() or
-  // RacyRegisteredThread::IsBeingProfiled() is true here, because it's
-  // possible that the result has changed since we tested it in the caller.
-  //
-  // Because of this imprecision it's possible to miss a marker or record one
-  // we shouldn't. Either way is not a big deal.
+  // This function is hot enough that we use RacyFeatures, not ActivePS.
+  if (!RacyFeatures::IsActiveWithoutPrivacy()) {
+    return;
+  }
+
+  // Note that it's possible that the above test would change again before we
+  // actually record the marker. Because of this imprecision it's possible to
+  // miss a marker or record one we shouldn't. Either way is not a big deal.
 
   RacyRegisteredThread* racyRegisteredThread =
       TLSRegisteredThread::RacyRegisteredThread();
@@ -4097,34 +4099,27 @@ static void racy_profiler_add_marker(
 
 void profiler_add_marker(const char* aMarkerName,
                          JS::ProfilingCategoryPair aCategoryPair,
-                         UniquePtr<ProfilerMarkerPayload> aPayload) {
-  MOZ_RELEASE_ASSERT(CorePS::Exists());
-
-  // This function is hot enough that we use RacyFeatures, not ActivePS.
-  if (!RacyFeatures::IsActiveWithoutPrivacy()) {
-    return;
-  }
-
-  racy_profiler_add_marker(aMarkerName, aCategoryPair, std::move(aPayload));
+                         const ProfilerMarkerPayload& aPayload) {
+  racy_profiler_add_marker(aMarkerName, aCategoryPair, &aPayload);
 }
 
 void profiler_add_marker(const char* aMarkerName,
                          JS::ProfilingCategoryPair aCategoryPair) {
-  profiler_add_marker(aMarkerName, aCategoryPair, nullptr);
+  racy_profiler_add_marker(aMarkerName, aCategoryPair, nullptr);
 }
 
 // This is a simplified version of profiler_add_marker that can be easily passed
 // into the JS engine.
 void profiler_add_js_marker(const char* aMarkerName) {
   AUTO_PROFILER_STATS(add_marker);
-  profiler_add_marker(aMarkerName, JS::ProfilingCategoryPair::JS, nullptr);
+  profiler_add_marker(aMarkerName, JS::ProfilingCategoryPair::JS);
 }
 
 void profiler_add_js_allocation_marker(JS::RecordAllocationInfo&& info) {
   AUTO_PROFILER_STATS(add_marker_with_JsAllocationMarkerPayload);
   profiler_add_marker(
       "JS allocation", JS::ProfilingCategoryPair::JS,
-      MakeUnique<JsAllocationMarkerPayload>(TimeStamp::Now(), std::move(info),
+      JsAllocationMarkerPayload(TimeStamp::Now(), std::move(info),
                                 profiler_get_backtrace()));
 }
 
@@ -4153,7 +4148,7 @@ void profiler_add_network_marker(
   AUTO_PROFILER_STATS(add_marker_with_NetworkMarkerPayload);
   profiler_add_marker(
       name, JS::ProfilingCategoryPair::NETWORK,
-      MakeUnique<NetworkMarkerPayload>(
+      NetworkMarkerPayload(
           static_cast<int64_t>(aChannelId), PromiseFlatCString(spec).get(),
           aType, aStart, aEnd, aPriority, aCount, aCacheDisposition, aTimings,
           PromiseFlatCString(redirect_spec).get(), std::move(aSource)));
@@ -4211,9 +4206,9 @@ void profiler_tracing(const char* aCategoryString, const char* aMarkerName,
   }
 
   AUTO_PROFILER_STATS(add_marker_with_TracingMarkerPayload);
-  auto payload = MakeUnique<TracingMarkerPayload>(
-      aCategoryString, aKind, aDocShellId, aDocShellHistoryId);
-  racy_profiler_add_marker(aMarkerName, aCategoryPair, std::move(payload));
+  profiler_add_marker(aMarkerName, aCategoryPair,
+                      TracingMarkerPayload(aCategoryString, aKind, aDocShellId,
+                                           aDocShellHistoryId));
 }
 
 void profiler_tracing(const char* aCategoryString, const char* aMarkerName,
@@ -4231,10 +4226,10 @@ void profiler_tracing(const char* aCategoryString, const char* aMarkerName,
   }
 
   AUTO_PROFILER_STATS(add_marker_with_TracingMarkerPayload);
-  auto payload =
-      MakeUnique<TracingMarkerPayload>(aCategoryString, aKind, aDocShellId,
-                                       aDocShellHistoryId, std::move(aCause));
-  racy_profiler_add_marker(aMarkerName, aCategoryPair, std::move(payload));
+  profiler_add_marker(
+      aMarkerName, aCategoryPair,
+      TracingMarkerPayload(aCategoryString, aKind, aDocShellId,
+                           aDocShellHistoryId, std::move(aCause)));
 }
 
 void profiler_add_text_marker(
@@ -4247,7 +4242,7 @@ void profiler_add_text_marker(
   AUTO_PROFILER_STATS(add_marker_with_TextMarkerPayload);
   profiler_add_marker(
       aMarkerName, aCategoryPair,
-      MakeUnique<TextMarkerPayload>(aText, aStartTime, aEndTime, aDocShellId,
+      TextMarkerPayload(aText, aStartTime, aEndTime, aDocShellId,
                         aDocShellHistoryId, std::move(aCause)));
 }
 

tools/profiler/public/GeckoProfiler.h

@@ -695,14 +695,15 @@ void profiler_add_marker(const char* aMarkerName,
       markerName, categoryPair, PayloadType, parenthesizedPayloadArgs) \
     do {                                                               \
       AUTO_PROFILER_STATS(add_marker_with_##PayloadType);              \
-      ::profiler_add_marker(                                            \
-          markerName, ::JS::ProfilingCategoryPair::categoryPair,        \
-          ::mozilla::MakeUnique<PayloadType> parenthesizedPayloadArgs); \
+      ::profiler_add_marker(markerName,                                \
+                            ::JS::ProfilingCategoryPair::categoryPair, \
+                            PayloadType parenthesizedPayloadArgs);     \
     } while (false)
 
 void profiler_add_marker(const char* aMarkerName,
                          JS::ProfilingCategoryPair aCategoryPair,
-                         mozilla::UniquePtr<ProfilerMarkerPayload> aPayload);
+                         const ProfilerMarkerPayload& aPayload);
+
 void profiler_add_js_marker(const char* aMarkerName);
 void profiler_add_js_allocation_marker(JS::RecordAllocationInfo&& info);
 

tools/profiler/public/ProfilerMarkerPayload.h

@@ -651,7 +651,7 @@ class LogMarkerPayload : public ProfilerMarkerPayload {
 class JsAllocationMarkerPayload : public ProfilerMarkerPayload {
  public:
   JsAllocationMarkerPayload(const mozilla::TimeStamp& aStartTime,
-                            const JS::RecordAllocationInfo& aInfo,
+                            JS::RecordAllocationInfo&& aInfo,
                             UniqueProfilerBacktrace aStack)
       : ProfilerMarkerPayload(aStartTime, aStartTime, mozilla::Nothing(),
                               mozilla::Nothing(), std::move(aStack)),
@@ -699,6 +699,19 @@ class JsAllocationMarkerPayload : public ProfilerMarkerPayload {
 
 namespace mozilla {
 
+// Serialize a pointed-at ProfilerMarkerPayload, may be null when there are no
+// payloads.
+template <>
+struct BlocksRingBuffer::Serializer<const ProfilerMarkerPayload*> {
+  static Length Bytes(const ProfilerMarkerPayload* aPayload) {
+    return ProfilerMarkerPayload::TagAndSerializationBytes(aPayload);
+  }
+
+  static void Write(EntryWriter& aEW, const ProfilerMarkerPayload* aPayload) {
+    ProfilerMarkerPayload::TagAndSerialize(aPayload, aEW);
+  }
+};
+
 // Serialize a pointed-at ProfilerMarkerPayload, may be null when there are no
 // payloads.
 template <>