Closed Bug 1357849 Opened 3 years ago Closed 3 years ago

Instrument performance.measure with markers

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: gregtatum, Assigned: gregtatum)

Details

Attachments

(1 file)

We currently only capture markers for performance.mark. Implement markers with timing information for the performance.measure. Frameworks like React are using this information to profile their code.

http://searchfox.org/mozilla-central/rev/214345204f1e7d97abb571b7992b6deedb5ff98f/dom/performance/Performance.cpp#314
Assignee: nobody → gtatum
Comment on attachment 8861037 [details]
Bug 1357849 - Instrument performance.measure with markers;

https://reviewboard.mozilla.org/r/133038/#review135802

::: dom/performance/Performance.cpp:363
(Diff revision 1)
> +  if (profiler_is_active()) {
> +    TimeStamp startTimeStamp = CreationTimeStamp() +
> +                               TimeDuration::FromMilliseconds(startTime);
> +    TimeStamp endTimeStamp = CreationTimeStamp() +
> +                             TimeDuration::FromMilliseconds(endTime);
> +    PROFILER_MARKER_PAYLOAD("UserTiming",
> +                            new UserTimingMarkerPayload(NS_ConvertUTF16toUTF8(aName).get(),
> +                                                        startTimeStamp,
> +                                                        endTimeStamp));
> +  }

This if needs to be wrapped in an #ifdef MOZ_GECKO_PROFILER because you're using the class UserTimingMarkerPayload which is only available if MOZ_GECKO_PROFILER is defined.

Unfortunately we don't have any builds in CI that build without MOZ_GECKO_PROFILER.

::: dom/performance/Performance.cpp:369
(Diff revision 1)
> +    TimeStamp startTimeStamp = CreationTimeStamp() +
> +                               TimeDuration::FromMilliseconds(startTime);
> +    TimeStamp endTimeStamp = CreationTimeStamp() +
> +                             TimeDuration::FromMilliseconds(endTime);
> +    PROFILER_MARKER_PAYLOAD("UserTiming",
> +                            new UserTimingMarkerPayload(NS_ConvertUTF16toUTF8(aName).get(),

If you make UserTimingMarkerPayload::mName an nsString, as I'm requesting below, then you can just pass aName here.

::: tools/profiler/core/ProfilerMarkers.cpp:163
(Diff revision 1)
>  }
>  
> +UserTimingMarkerPayload::UserTimingMarkerPayload(const char* aName,
> +                                                 const mozilla::TimeStamp& aStartTime)
> +  : ProfilerMarkerPayload(aStartTime, aStartTime, nullptr)
> +  , mEntryType((char *)"mark")

Is the cast still needed after you changed the type of mEntryType to be const char*? I would think not.

::: tools/profiler/public/ProfilerMarkers.h:159
(Diff revision 1)
> +                             UniqueStacks& aUniqueStacks) override;
> +
> +private:
> +  // Either "mark" or "measure".
> +  const char* mEntryType;
> +  char* mName;

Let's make this one an nsString. That saves the manual memory management. We can convert it to utf-8 inside StreamPayload.
Attachment #8861037 - Flags: review?(mstange)
Comment on attachment 8861037 [details]
Bug 1357849 - Instrument performance.measure with markers;

https://reviewboard.mozilla.org/r/133038/#review136450

Looks good to me. You should probably also get baku to review it because I'm not a peer of this code.
Attachment #8861037 - Flags: review?(mstange) → review+
Attachment #8861037 - Flags: review?(amarchesini)
Comment on attachment 8861037 [details]
Bug 1357849 - Instrument performance.measure with markers;

https://reviewboard.mozilla.org/r/133038/#review136654

::: tools/profiler/public/ProfilerMarkers.h:142
(Diff revision 2)
>  private:
>    nsString mType;
>    uint16_t mPhase;
>  };
>  
> +class UserTimingMarkerPayload : public ProfilerMarkerPayload

final

::: tools/profiler/public/ProfilerMarkers.h:150
(Diff revision 2)
> +  UserTimingMarkerPayload(const nsAString& aName,
> +                          const mozilla::TimeStamp& aStartTime);
> +  UserTimingMarkerPayload(const nsAString& aName,
> +                          const mozilla::TimeStamp& aStartTime,
> +                          const mozilla::TimeStamp& aEndTime);
> +  ~UserTimingMarkerPayload();

= default;

::: tools/profiler/public/ProfilerMarkers.h:152
(Diff revision 2)
> +  UserTimingMarkerPayload(const nsAString& aName,
> +                          const mozilla::TimeStamp& aStartTime,
> +                          const mozilla::TimeStamp& aEndTime);
> +  ~UserTimingMarkerPayload();
> +
> +  virtual void StreamPayload(SpliceableJSONWriter& aWriter,

if final, remove virtual.

::: tools/profiler/public/ProfilerMarkers.h:166
(Diff revision 2)
> +
>  /**
>   * Contains the translation applied to a 2d layer so we can
>   * track the layer position at each frame.
>   */
>  class LayerTranslationPayload : public ProfilerMarkerPayload

this is final as well, right?
Attachment #8861037 - Flags: review?(amarchesini) → review+
Comment on attachment 8861037 [details]
Bug 1357849 - Instrument performance.measure with markers;

https://reviewboard.mozilla.org/r/133038/#review137022

::: tools/profiler/public/ProfilerMarkers.h:142
(Diff revision 2)
>  private:
>    nsString mType;
>    uint16_t mPhase;
>  };
>  
> +class UserTimingMarkerPayload : public ProfilerMarkerPayload

Thanks for pointing these out. I think I'll keep the implementation the same as the other existing ones in this file. These classes need to be cleaned up, and I'd prefer to hit them all at once with some additional changes in a follow-up.
Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4651fc0179ee
Instrument performance.measure with markers; r=baku,mstange
https://hg.mozilla.org/mozilla-central/rev/4651fc0179ee
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.