Closed Bug 1324941 Opened 7 years ago Closed 7 years ago

Markers for DOM event scripts

Categories

(Toolkit :: Performance Monitoring, defect)

Product:
Toolkit
Component:
Performance Monitoring
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: Harald, Assigned: mstange)

References

Details

Attachments

(1 file)

Bug 1324941 - Add a profiler label and a profiler marker for DOMEvent dispatch.
59 bytes, text/x-review-board-request
smaug
: review+
Details 
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?
Flags: needinfo?(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.
Flags: needinfo?(mstange)
Assignee: nobody → mstange
Status: NEW → ASSIGNED
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.
Attachment #8820760 - Flags: review?(bugs) → review-
Blocks: 1325161
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?
Attachment #8820760 - Flags: review?(bugs) → review+
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.
(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.
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/77cbde755c30
Add a profiler label and a profiler marker for DOMEvent dispatch. r=smaug
https://hg.mozilla.org/mozilla-central/rev/77cbde755c30
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1326524
Depends on: 1352782
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: