Closed Bug 1447973 Opened 2 years ago Closed 2 years ago

Convert DOMEvent to tracing markers

Categories

(Core :: Gecko Profiler, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: julienw, Assigned: canova)

References

()

Details

Attachments

(1 file, 1 obsolete file)

See https://github.com/devtools-html/perf.html/issues/606:

When capturing _during_ a DOMEvent, we don't see the marker of it in the captured profile. This happens because a DOMEvent marker is an interval marker that's being emitted after the event finishes. Because DOMEvent can have a big duration in some problematic cases, it makes sense to convert it to a tracing marker instead.

Of course we need to keep the current payload associated with the marker.

This is where the marker is emitted [1], the code is quite self-contained.

[1] https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/dom/events/EventListenerManager.cpp#1234-1257
Moving this to "DOM: Events" but please move it back to Gecko Profiler if you think this is a better component. This bug actually belongs to both components :-)
Component: Gecko Profiler → DOM: Events
Component: DOM: Events → Gecko Profiler
More context and details about reproducing this issue are referenced in this comment: https://github.com/devtools-html/perf.html/issues/606#issue-261043069
Priority: -- → P1
Assignee: nobody → canaltinova
Status: NEW → ASSIGNED
I also created a PR for perf.html. Please see it here: https://github.com/devtools-html/perf.html/pull/1008
Without the PR on the perf.html side, it doesn't show the marker rectangle since we are giving the "Infinity" width. So we had to change it to a max number value to display it properly.
Attachment #8976545 - Flags: review?(felash) → review?(mstange)
I redirected to Markus because I don't feel confident enough to review something there :)

My 2 cents would be that we still need things from the DOMEventMarkerPayload: the type, the phase.
Comment on attachment 8976545 [details]
Bug 1447973 - Convert DOMEvent markers to tracing markers instead

Removing the review request for now. Talked with julienw and it appears this requires a different approach. Working on that.
Attachment #8976545 - Flags: review?(mstange)
Attachment #8976545 - Attachment is obsolete: true
The perf.html part is not completely finished yet but wanted to push the m-c side so I can get some early feedback :)
Comment on attachment 8979406 [details]
Bug 1447973 - Convert DOMEvent markers to use tracing markers

https://reviewboard.mozilla.org/r/245574/#review251624

Looks good to me!
Attachment #8979406 - Flags: review?(mstange) → review+
This works good enough and Markus already gave an r+ so it looks good to me as well. I'm only a bit sad that we're not using a similar approach than for existing tracing markers were one object controls emitting the markers. I would have liked seeing this approach generalized so that it's more easily used elsewhere.

But we can do that later if we actually need it elsewhere.
I created a PR for this bug. Please see: https://github.com/devtools-html/perf.html/pull/1031.

Hi, julienw. Hm, I think we can do that. Also doing it on a new bug looks okay to me.
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/65a6064fb267
Convert DOMEvent markers to use tracing markers r=mstange
https://hg.mozilla.org/mozilla-central/rev/65a6064fb267
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.