Closed Bug 1447973 Opened 3 years ago Closed 3 years ago

Convert DOMEvent to tracing markers


(Core :: Gecko Profiler, enhancement, P1)




Tracking Status
firefox61 --- wontfix
firefox62 --- fixed


(Reporter: julienw, Assigned: canova)





(1 file, 1 obsolete file)


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.

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:
Priority: -- → P1
Assignee: nobody → canaltinova
I also created a PR for perf.html. Please see it here:
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

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:

Hi, julienw. Hm, I think we can do that. Also doing it on a new bug looks okay to me.
Pushed by
Convert DOMEvent markers to use tracing markers r=mstange
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.