1357849 - Instrument performance.measure with markers

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Greg Tatum [:gregtatum]]

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

Comment 1

[Greg Tatum [:gregtatum]]

Attachment: Bug 1357849 - Instrument performance.measure with markers;

Review commit: https://reviewboard.mozilla.org/r/133038/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133038/

Comment 2

[Markus Stange [:mstange]]

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.

Comment 3

[Greg Tatum [:gregtatum]]

Comment on attachment 8861037 [details]
Bug 1357849 - Instrument performance.measure with markers;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133038/diff/1-2/

Comment 4

[Markus Stange [: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.

Comment 5

[Andrea Marchesini [:baku]]

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?

Comment 6

[Greg Tatum [:gregtatum]]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4651fc0179ee
Instrument performance.measure with markers; r=baku,mstange

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4651fc0179ee