Bug 1646266 - Marker option: MarkerTiming - r=gregtatum

This moves the existing MarkerTiming class introduced in bug 1640969 to the BaseProfilerMarkersPrerequesites.h header, and can be used as a marker option.

Some minor clarifying changes:
- `Instant()` is split into two functions: `InstantNow()` and `InstantAt(TimeStamp)`.
- `Interval(TimeStamp, TimeStamp)` must be given both start and end, otherwise `IntervalUntilNowFrom(TimeStamp)` takes the start only and ends "now".

Also the default construction is now reserved for internal marker usage, the private member function `IsUnspecified()` will be used by the add-marker code will replace it with `InstantNow()`.

The serialization contains the phase, and only one or two timestamps as needed, to save space for non-interval timings.

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

mozglue/baseprofiler/public/BaseProfilerMarkersDetail.h

@@ -131,7 +131,6 @@ struct ProfileBufferEntryReader::Deserializer<ProfilerStringView<CHAR>> {
   }
 };
 
-// ----------------------------------------------------------------------------
 // Serializer, Deserializer: MarkerCategory
 
 // The serialization contains both category numbers encoded as ULEB128.
@@ -167,6 +166,121 @@ struct ProfileBufferEntryReader::Deserializer<MarkerCategory> {
   }
 };
 
+// ----------------------------------------------------------------------------
+// Serializer, Deserializer: MarkerTiming
+
+// The serialization starts with the marker phase, followed by one or two
+// timestamps as needed.
+template <>
+struct ProfileBufferEntryWriter::Serializer<MarkerTiming> {
+  static Length Bytes(const MarkerTiming& aTiming) {
+    MOZ_ASSERT(!aTiming.IsUnspecified());
+    const auto phase = aTiming.MarkerPhase();
+    switch (phase) {
+      case MarkerTiming::Phase::Instant:
+        return SumBytes(phase, aTiming.StartTime());
+      case MarkerTiming::Phase::Interval:
+        return SumBytes(phase, aTiming.StartTime(), aTiming.EndTime());
+      case MarkerTiming::Phase::IntervalStart:
+        return SumBytes(phase, aTiming.StartTime());
+      case MarkerTiming::Phase::IntervalEnd:
+        return SumBytes(phase, aTiming.EndTime());
+      default:
+        MOZ_RELEASE_ASSERT(phase == MarkerTiming::Phase::Instant ||
+                           phase == MarkerTiming::Phase::Interval ||
+                           phase == MarkerTiming::Phase::IntervalStart ||
+                           phase == MarkerTiming::Phase::IntervalEnd);
+        return 0;  // Only to avoid build errors.
+    }
+  }
+
+  static void Write(ProfileBufferEntryWriter& aEW,
+                    const MarkerTiming& aTiming) {
+    MOZ_ASSERT(!aTiming.IsUnspecified());
+    const auto phase = aTiming.MarkerPhase();
+    switch (phase) {
+      case MarkerTiming::Phase::Instant:
+        aEW.WriteObjects(phase, aTiming.StartTime());
+        return;
+      case MarkerTiming::Phase::Interval:
+        aEW.WriteObjects(phase, aTiming.StartTime(), aTiming.EndTime());
+        return;
+      case MarkerTiming::Phase::IntervalStart:
+        aEW.WriteObjects(phase, aTiming.StartTime());
+        return;
+      case MarkerTiming::Phase::IntervalEnd:
+        aEW.WriteObjects(phase, aTiming.EndTime());
+        return;
+      default:
+        MOZ_RELEASE_ASSERT(phase == MarkerTiming::Phase::Instant ||
+                           phase == MarkerTiming::Phase::Interval ||
+                           phase == MarkerTiming::Phase::IntervalStart ||
+                           phase == MarkerTiming::Phase::IntervalEnd);
+        return;
+    }
+  }
+};
+
+template <>
+struct ProfileBufferEntryReader::Deserializer<MarkerTiming> {
+  static void ReadInto(ProfileBufferEntryReader& aER, MarkerTiming& aTiming) {
+    aTiming.mPhase = aER.ReadObject<MarkerTiming::Phase>();
+    switch (aTiming.mPhase) {
+      case MarkerTiming::Phase::Instant:
+        aTiming.mStartTime = aER.ReadObject<TimeStamp>();
+        aTiming.mEndTime = TimeStamp{};
+        break;
+      case MarkerTiming::Phase::Interval:
+        aTiming.mStartTime = aER.ReadObject<TimeStamp>();
+        aTiming.mEndTime = aER.ReadObject<TimeStamp>();
+        break;
+      case MarkerTiming::Phase::IntervalStart:
+        aTiming.mStartTime = aER.ReadObject<TimeStamp>();
+        aTiming.mEndTime = TimeStamp{};
+        break;
+      case MarkerTiming::Phase::IntervalEnd:
+        aTiming.mStartTime = TimeStamp{};
+        aTiming.mEndTime = aER.ReadObject<TimeStamp>();
+        break;
+      default:
+        MOZ_RELEASE_ASSERT(aTiming.mPhase == MarkerTiming::Phase::Instant ||
+                           aTiming.mPhase == MarkerTiming::Phase::Interval ||
+                           aTiming.mPhase ==
+                               MarkerTiming::Phase::IntervalStart ||
+                           aTiming.mPhase == MarkerTiming::Phase::IntervalEnd);
+        break;
+    }
+  }
+
+  static MarkerTiming Read(ProfileBufferEntryReader& aER) {
+    TimeStamp start;
+    TimeStamp end;
+    auto phase = aER.ReadObject<MarkerTiming::Phase>();
+    switch (phase) {
+      case MarkerTiming::Phase::Instant:
+        start = aER.ReadObject<TimeStamp>();
+        break;
+      case MarkerTiming::Phase::Interval:
+        start = aER.ReadObject<TimeStamp>();
+        end = aER.ReadObject<TimeStamp>();
+        break;
+      case MarkerTiming::Phase::IntervalStart:
+        start = aER.ReadObject<TimeStamp>();
+        break;
+      case MarkerTiming::Phase::IntervalEnd:
+        end = aER.ReadObject<TimeStamp>();
+        break;
+      default:
+        MOZ_RELEASE_ASSERT(phase == MarkerTiming::Phase::Instant ||
+                           phase == MarkerTiming::Phase::Interval ||
+                           phase == MarkerTiming::Phase::IntervalStart ||
+                           phase == MarkerTiming::Phase::IntervalEnd);
+        break;
+    }
+    return MarkerTiming(start, end, phase);
+  }
+};
+
 }  // namespace mozilla
 
 #endif  // MOZ_GECKO_PROFILER

mozglue/baseprofiler/public/BaseProfilerMarkersPrerequisites.h

@@ -16,6 +16,7 @@
 #ifdef MOZ_GECKO_PROFILER
 
 #  include "mozilla/ProfileChunkedBuffer.h"
+#  include "mozilla/TimeStamp.h"
 
 #  include <string_view>
 #  include <string>
@@ -279,6 +280,116 @@ class MarkerThreadId {
   int mThreadId = 0;
 };
 
+// This marker option contains marker timing information.
+// This class encapsulates the logic for correctly storing a marker based on its
+// Use the static methods to create the MarkerTiming. This is a transient object
+// that is being used to enforce the constraints of the combinations of the
+// data.
+class MarkerTiming {
+ public:
+  // The following static methods are used to create the MarkerTiming based on
+  // the type that it is.
+
+  static MarkerTiming InstantAt(const TimeStamp& aTime) {
+    MOZ_ASSERT(!aTime.IsNull(), "Time is null for an instant marker.");
+    return MarkerTiming{aTime, TimeStamp{}, MarkerTiming::Phase::Instant};
+  }
+
+  static MarkerTiming InstantNow() {
+    return InstantAt(TimeStamp::NowUnfuzzed());
+  }
+
+  static MarkerTiming Interval(const TimeStamp& aStartTime,
+                               const TimeStamp& aEndTime) {
+    MOZ_ASSERT(!aStartTime.IsNull(),
+               "Start time is null for an interval marker.");
+    MOZ_ASSERT(!aEndTime.IsNull(), "End time is null for an interval marker.");
+    return MarkerTiming{aStartTime, aEndTime, MarkerTiming::Phase::Interval};
+  }
+
+  static MarkerTiming IntervalUntilNowFrom(const TimeStamp& aStartTime) {
+    return Interval(aStartTime, TimeStamp::NowUnfuzzed());
+  }
+
+  static MarkerTiming IntervalStart(
+      const TimeStamp& aTime = TimeStamp::NowUnfuzzed()) {
+    MOZ_ASSERT(!aTime.IsNull(), "Time is null for an interval start marker.");
+    return MarkerTiming{aTime, TimeStamp{}, MarkerTiming::Phase::IntervalStart};
+  }
+
+  static MarkerTiming IntervalEnd(
+      const TimeStamp& aTime = TimeStamp::NowUnfuzzed()) {
+    MOZ_ASSERT(!aTime.IsNull(), "Time is null for an interval end marker.");
+    return MarkerTiming{TimeStamp{}, aTime, MarkerTiming::Phase::IntervalEnd};
+  }
+
+  [[nodiscard]] const TimeStamp& StartTime() const { return mStartTime; }
+  [[nodiscard]] const TimeStamp& EndTime() const { return mEndTime; }
+
+  enum class Phase : uint8_t {
+    Instant = 0,
+    Interval = 1,
+    IntervalStart = 2,
+    IntervalEnd = 3,
+  };
+
+  [[nodiscard]] Phase MarkerPhase() const {
+    MOZ_ASSERT(!IsUnspecified());
+    return mPhase;
+  }
+
+  // The following getter methods are used to put the value into the buffer for
+  // storage.
+  [[nodiscard]] double GetStartTime() const {
+    MOZ_ASSERT(!IsUnspecified());
+    // If mStartTime is null (e.g., for IntervalEnd), this will output 0.0 as
+    // expected.
+    return MarkerTiming::timeStampToDouble(mStartTime);
+  }
+
+  [[nodiscard]] double GetEndTime() const {
+    MOZ_ASSERT(!IsUnspecified());
+    // If mEndTime is null (e.g., for Instant or IntervalStart), this will
+    // output 0.0 as expected.
+    return MarkerTiming::timeStampToDouble(mEndTime);
+  }
+
+  [[nodiscard]] uint8_t GetPhase() const {
+    MOZ_ASSERT(!IsUnspecified());
+    return static_cast<uint8_t>(mPhase);
+  }
+
+ private:
+  friend ProfileBufferEntryWriter::Serializer<MarkerTiming>;
+  friend ProfileBufferEntryReader::Deserializer<MarkerTiming>;
+
+  // Default timing leaves it internally "unspecified", serialization getters
+  // and add-marker functions will default to `InstantNow()`.
+  constexpr MarkerTiming() = default;
+
+  // This should only be used by internal profiler code.
+  [[nodiscard]] bool IsUnspecified() const {
+    return mStartTime.IsNull() && mEndTime.IsNull();
+  }
+
+  // Full constructor, used by static factory functions.
+  constexpr MarkerTiming(const TimeStamp& aStartTime, const TimeStamp& aEndTime,
+                         Phase aPhase)
+      : mStartTime(aStartTime), mEndTime(aEndTime), mPhase(aPhase) {}
+
+  static double timeStampToDouble(const TimeStamp& time) {
+    if (time.IsNull()) {
+      // The Phase lets us know not to use this value.
+      return 0;
+    }
+    return (time - TimeStamp::ProcessCreation()).ToMilliseconds();
+  }
+
+  TimeStamp mStartTime;
+  TimeStamp mEndTime;
+  Phase mPhase = Phase::Instant;
+};
+
 }  // namespace mozilla
 
 #endif  // MOZ_GECKO_PROFILER

tools/profiler/core/ProfilerMarkerPayload.cpp

@@ -139,8 +139,8 @@ ProfilerMarkerPayload::DeserializeCommonProps(
 // Deprecated: This function is providing a way for a few payloads to use the
 // start time and end time in their payloads, which is currently deprecated.
 // The startTime and endTime were removed from most payloads, in favor of
-// the MarkerPhase idea. However, IPC and Network markers still have them as
+// the MarkerTiming::Phase idea. However, IPC and Network markers still have
-// it was harder to upgrade the front-end without them.
+// them as it was harder to upgrade the front-end without them.
 void ProfilerMarkerPayload::StreamStartEndTime(
     SpliceableJSONWriter& aWriter, const TimeStamp& aProcessStartTime) const {
   WriteTime(aWriter, aProcessStartTime, mCommonProps.mStartTime, "startTime");

tools/profiler/core/platform.cpp

@@ -1588,80 +1588,6 @@ void ProfilingStackOwner::DumpStackAndCrash() const {
 // The name of the main thread.
 static const char* const kMainThreadName = "GeckoMain";
 
-enum class MarkerPhase : uint8_t {
-  Instant = 0,
-  Interval = 1,
-  IntervalStart = 2,
-  IntervalEnd = 3,
-};
-
-// This class encapsulates the logic for correctly storing a marker based on its
-// timing type, based on the constraints of that type. Use the static methods to
-// create the MarkerTiming. This is a transient object that is being used to
-// enforce the constraints of the combinations of the data.
-class MarkerTiming {
- public:
-  // The following static methods are used to create the MarkerTiming based on
-  // the type that it is.
-  static MarkerTiming Instant(
-      const TimeStamp& aTime = TimeStamp::NowUnfuzzed()) {
-    MOZ_ASSERT(!aTime.IsNull(), "Time is null for an instant marker.");
-    return MarkerTiming{aTime, TimeStamp{}, MarkerPhase::Instant};
-  }
-
-  static MarkerTiming Interval(
-      const TimeStamp& aStartTime,
-      const TimeStamp& aEndTime = TimeStamp::NowUnfuzzed()) {
-    MOZ_ASSERT(!aStartTime.IsNull(),
-               "Start time is null for an interval marker.");
-    MOZ_ASSERT(!aEndTime.IsNull(), "End time is null for an interval marker.");
-    return MarkerTiming{aStartTime, aEndTime, MarkerPhase::Interval};
-  }
-
-  static MarkerTiming IntervalStart(
-      const TimeStamp& aTime = TimeStamp::NowUnfuzzed()) {
-    MOZ_ASSERT(!aTime.IsNull(), "Time is null for an interval start marker.");
-    return MarkerTiming{aTime, TimeStamp{}, MarkerPhase::IntervalStart};
-  }
-
-  static MarkerTiming IntervalEnd(
-      const TimeStamp& aTime = TimeStamp::NowUnfuzzed()) {
-    MOZ_ASSERT(!aTime.IsNull(), "Time is null for an interval end marker.");
-    return MarkerTiming{TimeStamp{}, aTime, MarkerPhase::IntervalEnd};
-  }
-
-  // The following getter methods are used to put the value into the buffer for
-  // storage.
-  double GetStartTime() const {
-    return MarkerTiming::timeStampToDouble(mStartTime);
-  }
-
-  double GetEndTime() const {
-    return MarkerTiming::timeStampToDouble(mEndTime);
-  }
-
-  uint8_t GetMarkerPhase() const { return static_cast<uint8_t>(mMarkerPhase); }
-
- private:
-  MarkerTiming(TimeStamp aStartTime, TimeStamp aEndTime,
-               MarkerPhase aMarkerPhase)
-      : mStartTime(aStartTime),
-        mEndTime(aEndTime),
-        mMarkerPhase(aMarkerPhase) {}
-
-  static double timeStampToDouble(TimeStamp time) {
-    if (time.IsNull()) {
-      // The MarkerPhase lets us know not to use this value.
-      return 0;
-    }
-    return (time - CorePS::ProcessStartTime()).ToMilliseconds();
-  }
-
-  TimeStamp mStartTime;
-  TimeStamp mEndTime;
-  MarkerPhase mMarkerPhase;
-};
-
 // TODO - It is better to have the marker timing created by the original callers
 // of the profiler_add_marker API, rather than deduce it from the payload. This
 // is a bigger code diff for adding MarkerTiming, so do that work in a
@@ -1673,7 +1599,7 @@ MarkerTiming get_marker_timing_from_payload(
   if (start.IsNull()) {
     if (end.IsNull()) {
       // The payload contains no time information, use the current time.
-      return MarkerTiming::Instant(TimeStamp::NowUnfuzzed());
+      return MarkerTiming::InstantAt(TimeStamp::NowUnfuzzed());
     }
     return MarkerTiming::IntervalEnd(end);
   }
@@ -1681,7 +1607,7 @@ MarkerTiming get_marker_timing_from_payload(
     return MarkerTiming::IntervalStart(start);
   }
   if (start == end) {
-    return MarkerTiming::Instant(start);
+    return MarkerTiming::InstantAt(start);
   }
   return MarkerTiming::Interval(start, end);
 }
@@ -1693,12 +1619,11 @@ static void StoreMarker(ProfileChunkedBuffer& aChunkedBuffer, int aThreadId,
                         const MarkerTiming& aMarkerTiming,
                         JS::ProfilingCategoryPair aCategoryPair,
                         const ProfilerMarkerPayload* aPayload) {
-  aChunkedBuffer.PutObjects(ProfileBufferEntry::Kind::MarkerData, aThreadId,
+  aChunkedBuffer.PutObjects(
-                            WrapProfileBufferUnownedCString(aMarkerName),
+      ProfileBufferEntry::Kind::MarkerData, aThreadId,
-                            aMarkerTiming.GetStartTime(),
+      WrapProfileBufferUnownedCString(aMarkerName),
-                            aMarkerTiming.GetEndTime(),
+      aMarkerTiming.GetStartTime(), aMarkerTiming.GetEndTime(),
-                            aMarkerTiming.GetMarkerPhase(),
+      aMarkerTiming.GetPhase(), static_cast<uint32_t>(aCategoryPair), aPayload);
-                            static_cast<uint32_t>(aCategoryPair), aPayload);
 }
 
 ////////////////////////////////////////////////////////////////////////
@@ -2788,7 +2713,7 @@ static void CollectJavaThreadProfileData(ProfileBuffer& aProfileBuffer) {
                             : CorePS::ProcessStartTime() +
                                   TimeDuration::FromMilliseconds(endTimeMs);
     MarkerTiming timing = endTimeMs == 0
-                              ? MarkerTiming::Instant(startTime)
+                              ? MarkerTiming::InstantAt(startTime)
                               : MarkerTiming::Interval(startTime, endTime);
 
     if (!text) {
@@ -5457,7 +5382,7 @@ static void racy_profiler_add_marker(const char* aMarkerName,
 
   const MarkerTiming markerTiming =
       aPayload ? get_marker_timing_from_payload(*aPayload)
-               : MarkerTiming::Instant();
+               : MarkerTiming::InstantNow();
 
   StoreMarker(CorePS::CoreBuffer(), racyRegisteredThread->ThreadId(),
               aMarkerName, markerTiming, aCategoryPair, aPayload);