Closed Bug 1447973 Opened 3 years ago Closed 3 years ago
Convert DOMEvent to tracing markers
59 bytes, text/x-review-board-request
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 , the code is quite self-contained.  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
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 firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/65a6064fb267 Convert DOMEvent markers to use tracing markers r=mstange
You need to log in before you can comment on or make changes to this bug.