Add a profiler annotation to PresShell::Paint that gives the URL of the document being painted

RESOLVED FIXED in Firefox 54

Status

()

Core
Layout
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

I have a profile with a very expensive paint and it's impossible to tell where the paint is coming from.  It would be nice if we annotated the PresShell::Paint marker with the URL of the document being painted or some such.
Created attachment 8830860 [details] [diff] [review]
Add a profiler annotation to PresShell::Paint that gives the URL of the document being painted

This essentially copies what we do for the PresShell::DoReflow marker.
Attachment #8830860 - Flags: review?(dholbert)
Comment on attachment 8830860 [details] [diff] [review]
Add a profiler annotation to PresShell::Paint that gives the URL of the document being painted

Review of attachment 8830860 [details] [diff] [review]:
-----------------------------------------------------------------

heads-up: this patch doesn't apply cleanly, due to some contextual changes in the lines after it -- but it's fine if you use fuzz. (patch -F6)

r=me with one nit:

::: layout/base/PresShell.cpp
@@ +6237,5 @@
>                   const nsRegion& aDirtyRegion,
>                   uint32_t        aFlags)
>  {
> +#ifdef MOZ_GECKO_PROFILER
> +  nsIURI *uri = mDocument->GetDocumentURI();

Nit: please move the * to the left of the space character, per gecko coding style.
Attachment #8830860 - Flags: review?(dholbert) → review+
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Comment on attachment 8830860 [details] [diff] [review]
Add a profiler annotation to PresShell::Paint that gives the URL of the document being painted

For non-e10s browsers the url will always be "browser.xul" so basically useless.

Comment 4

7 months ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0bfce6fc98a
Add a profiler annotation to PresShell::Paint that gives the URL of the document being painted; r=dholbert
(In reply to Timothy Nikkel (:tnikkel) from comment #3)
> Comment on attachment 8830860 [details] [diff] [review]
> Add a profiler annotation to PresShell::Paint that gives the URL of the
> document being painted
> 
> For non-e10s browsers the url will always be "browser.xul" so basically
> useless.

Is there a better URL to include for that configuration?  I'd be happy to do that in a follow-up.

(I personally don't care much about profiling the non-e10s browser. :-)
Flags: needinfo?(tnikkel)
(I suspect we'd need to move this profiler-annotation a bit deeper in the painting callstack, in order for it to be able to easily discover which content-document it's going to be painting [in a non-e10s browser].)
There isn't a place where we have easy access to the url of the active tab AND would contain all the time used to paint. We could capture the url in nsSubDocumentFrame::BuildDisplayList when we enter a content doc from a chrome doc, that would only capture the time spent building the display list not painting.

There is PresShell::GetTouchEventTargetDocument which gets the primary content shell, that should be the active tab according to the comments there. So we could record that url, and assign all the blame for painting chrome+content to the url of the active tab, which would be somewhat incorrect, but more helpful then the current state as long as it is clear to people who interpret the profile what is going on.
Flags: needinfo?(tnikkel)

Comment 8

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f0bfce6fc98a
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Filed bug 1335070 as a follow-up.
You need to log in before you can comment on or make changes to this bug.