1324941 - Markers for DOM event scripts

Status: RESOLVED FIXED

(Toolkit :: Performance Monitoring, defect)

Description

[:Harald Kirschner :digitarald]

See DOMEvent docshell marker

https://dxr.mozilla.org/mozilla-central/source/dom/events/EventListenerManager.cpp?q=EventListenerManager.cpp&redirect_type=direct#1267

The markers should include event type and phase.

Comment 1

[:Harald Kirschner :digitarald]

I have checked but have not seen a way to add start/end markers with meta data. What is the recommended way to do that?

Comment 2

[Markus Stange [:mstange]]

We have "payload" markers for this use case - you implement a new subclass of ProfilerMarkerPayload (e.g. "DOMEventMarkerPayload"), and then you add such a marker by doing 

> PROFILER_MARKER_PAYLOAD("DOMEvent",
>                         new DOMEventMarkerPayload(typeStr, phase,
>                                                   startTime));

The ProfilerMarkerPayload base class has mStartTime and mEndTime fields. So apparently we should add just one marker for the whole interval instead of separate start + end markers.

To be honest I'm no longer sure what makes "tracing" markers special / different from regular markers.

During the process of researching this, I wrote a patch for this bug. I hope you don't mind that I've basically stolen this bug from you now :-)
The patch should be useful for seeing how to convert the other types of markers.

My patch also adds a PROFILER_LABEL so that you can see the event type in the call tree.

Comment 3

[Markus Stange [:mstange]]

Attachment: Bug 1324941 - Add a profiler label and a profiler marker for DOMEvent dispatch.

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

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8820760 [details]
Bug 1324941 - Add a profiler label and a profiler marker for DOMEvent dispatch.

https://reviewboard.mozilla.org/r/100196/#review100736

Need to figure out something faster in this kind of very hot code paths.

::: dom/events/EventListenerManager.cpp:1259
(Diff revision 1)
> -            // Maybe add a marker to the docshell's timeline, but only
> -            // bother with all the logic if some docshell is recording.
> +            // Add a profiler label, a profiler marker, and possibly a docshell
> +            // timeline marker. The profiler marker is added at the end of this
> +            // scope.
> +            TimeStamp startTime = TimeStamp::Now();
> +            nsAutoString typeStr;
> +            (*aDOMEvent)->GetType(typeStr);

Do we really need to access the type as string in this hot code path?
Definitely not when MOZ_ENABLE_PROFILER_SPS isn't set.


Is MOZ_ENABLE_PROFILER_SPS set normally?

::: dom/events/EventListenerManager.cpp:1305
(Diff revision 1)
>              if (needsEndEventMarker) {
>                timelines->AddMarkerForDocShell(
>                  docShell, "DOMEvent", MarkerTracingType::END);
>              }
> +            PROFILER_MARKER_PAYLOAD("DOMEvent",
> +                                    new DOMEventMarkerPayload(typeStr, phase,

So we always do an allocation in this hot code path? Not acceptable.

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8820760 [details]
Bug 1324941 - Add a profiler label and a profiler marker for DOMEvent dispatch.

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

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8820760 [details]
Bug 1324941 - Add a profiler label and a profiler marker for DOMEvent dispatch.

https://reviewboard.mozilla.org/r/100196/#review101000

::: dom/events/EventListenerManager.cpp:1306
(Diff revision 2)
> +
> +              rv = HandleEventSubType(listener, *aDOMEvent, aCurrentTarget);
> +
> +              uint16_t phase;
> +              (*aDOMEvent)->GetEventPhase(&phase);
> +              PROFILER_MARKER_PAYLOAD("DOMEvent",

So profiler code will then delete the created object, right?

::: tools/profiler/public/ProfilerMarkers.h:125
(Diff revision 2)
> +class DOMEventMarkerPayload : public ProfilerMarkerPayload
> +{
> +public:
> +  DOMEventMarkerPayload(const nsAString& aType, uint16_t aPhase,
> +                        const mozilla::TimeStamp& aStartTime,
> +                        const mozilla::TimeStamp& aEndTime = mozilla::TimeStamp::Now());

Why you need aEndTime when no one is ever using this param, but just falling back to the default value?

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8820760 [details]
Bug 1324941 - Add a profiler label and a profiler marker for DOMEvent dispatch.

https://reviewboard.mozilla.org/r/100196/#review101000

> So profiler code will then delete the created object, right?

Yes

> Why you need aEndTime when no one is ever using this param, but just falling back to the default value?

You're right, this was a bit silly. I've removed the default value. I do want the end time to be a specifiable parameter though.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8820760 [details]
Bug 1324941 - Add a profiler label and a profiler marker for DOMEvent dispatch.

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

Comment 9

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #7)
> Comment on attachment 8820760 [details]
> Bug 1324941 - Add a profiler label and a profiler marker for DOMEvent
> dispatch.
> 
> https://reviewboard.mozilla.org/r/100196/#review101000
> 
> > So profiler code will then delete the created object, right?
> 
> Yes

This happens at http://searchfox.org/mozilla-central/rev/ac40ca3ec39efe85bfb111274c10ee4ceea5bb7a/tools/profiler/core/platform.cpp#195 . And yes, there is way too much manual memory management in the profiler.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8820760 [details]
Bug 1324941 - Add a profiler label and a profiler marker for DOMEvent dispatch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100196/diff/3-4/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77cbde755c30
Add a profiler label and a profiler marker for DOMEvent dispatch. r=smaug

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77cbde755c30