1050773 - Reflows, restyle and paints are only recorded within refreshdriver::tick

Status: RESOLVED WORKSFORME

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Paul Rouget [:paul]]

They also happen in other places. We need to cover these too.

Comment 1

[Paul Rouget [:paul]]

Sync reflows are not recorded for example.

And if the docshell is an iframe inside a tab, nothing is recorded.

Comment 2

[Paul Rouget [:paul]]

Attachment: failing test (only fail in iframe)

First run this:

> ./mach mochitest-chrome  --keep-open docshell/test/chrome/test_timelineMarkers.html

You'll see failures. Open the console. You'll there never was any markers found.

Now only load: chrome://mochitests/content/chrome/docshell/test/chrome/test_timelineMarkers.html

Test is green.

The difference being the iframe.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: sync-reflow.html

So, sync reflows are already recorded since they aren't instrumented in refreshdriver::tick but instead in nsPresShell::DoReflow, which is called whether the reflow is interruptible or not.

The test html page attached should demonstrate that (open the timeline panel, and alternatively click on the sync and async buttons, you should see reflow markers in both cases).

Comment 5

[Patrick Brosset <:pbro>]

Attachment: iframe.html

However, only Paint markers are recorded in iframes, not Reflow nor Styles.
Open this HTML test page and click on the sync/async buttons with the timeline panel opened. You should only see paint events.

Since paint is always handled at top docshell level, we put in place since the start a mechanism that records by recursing through docshells. But that is not the case for reflows and restyles.

Comment 6

[Paul Rouget [:paul]]

What about this configuration (non-e10s): browser.xul docshell > tab docshell > iframe docshell

You connect the devtools to the iframe. What do we miss?

Comment 7

[Patrick Brosset <:pbro>]

Attachment: bug1050773-timeline-iframe v1.patch

Olli, this patch helps with recording profile timeline markers at ancestor docshell level. The use case is the following:

Open a page that has an iframe, open the new devtools timeline panel, make sure there are reflows/styles in the child iframe ==> The timeline right now doesn't show those.

We do want child frame operations to be recorded too, so for this, what I did in this patch is: whenever a new marker is add, if the docshell it is added on isn't actually recording right now, we walk up its parents until we find one that is, and we store the marker there.

I also added a new test for this.

Let me know if this is a correct way to address the use case.

Comment 8

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Comment on attachment 8489346 [details] [diff] [review]
bug1050773-timeline-iframe v1.patch

> nsDocShell::AddProfileTimelineMarker(const char* aName,
>                                      TracingMetadata aMetaData)
> {
> #ifdef MOZ_ENABLE_PROFILER_SPS
>   if (!mProfileTimelineStartTime.IsNull()) {
>     float delta = GetProfileTimelineDelta();
>     ProfilerMarkerTracing* payload = new ProfilerMarkerTracing("Timeline",
>                                                                aMetaData);
>     mProfileTimelineMarkers.AppendElement(
>       new InternalProfileTimelineMarker(aName, payload, delta));
>+  } else {
>+    // If the docShell isn't recording, check if it has an ancestor that does
>+    // and add the marker on that docShell instead.
>+    nsRefPtr<nsDocShell> parentDocShell = GetProfileTimelineRecordingAncestor();
>+    if (parentDocShell) {
>+      parentDocShell->AddProfileTimelineMarker(aName, aMetaData);
>+    }
This makes no sense to me.
Say you have some ad in the page and it is doing plenty of reflows or so, but we're recording only the main page, the
ad related stuff gets recorded as being part of the main page.

Perhaps we should record all the child iframes, but data for their behavior shouldn't be part of the parent's.
Shouldn't devtools handle this case, and just start recording/profiling on child docshells if needed?

>+  } else {
>+    // If the docShell isn't recording, check if it has an ancestor that does
>+    // and add the marker on that docShell instead.
>+    nsRefPtr<nsDocShell> parentDocShell = GetProfileTimelineRecordingAncestor();
>+    if (parentDocShell) {
>+      parentDocShell->AddProfileTimelineMarker(aName, aCause, aMetaData);
>+    }
ditto


So, I don't understand why we want this behavior.

Comment 9

[Paul Rouget [:paul]]

We can do what we do for the layout actor (record per docshell, up to the actor to go down the tree).

Patrick, I'm not sure exactly what your patch does, but the issue here is to not being to track docshells that are inside a content docshell. See comment 6.

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #9)
> We can do what we do for the layout actor (record per docshell, up to the
> actor to go down the tree).
Yes that's another option for sure, this would be pretty easy too. And, as discussed with Olli over IRC, the advantage of doing this is that we would be able to know which frame do operations come from (so potentially, we would be able to show that in the UI somehow).
> Patrick, I'm not sure exactly what your patch does, but the issue here is to
> not being to track docshells that are inside a content docshell. See comment
> 6.
If devtools is inspecting the tab's docShell and the tab contains an iframe: in this situation my patch would save at tab docShell's level all reflow/style operations that happen at child iframe level. So that's what we need.

I'll provide a new patch soon with the new solution.

Comment 11

[Paul Rouget [:paul]]

STR:
- load http://jsbin.com/comuquneluxo
- open the toolbox
- select the iframe "http://jsbin.com/banefohoyeko"
- start the timeline
- move mouse over the iframe

Only reflow and restyle are recorded, not the paint.

Comment 12

[Patrick Brosset <:pbro>]

Thanks Paul for the STR.
So we have 2 problems to solve:
1 - the timeline devtools actor isn't recording markers from all frames.
2 - paint operations for sub-iframes are actually reported at parent frame level.

1 is relatively easy to solve by making the actor iterate over |this.tabActor.windows| and reacting to |window-ready| events.

2 is a bit more complex, I'll need to investigate.
Right now the mechanism is that we record paint START and STOP markers at top level (which is where we know paint is happening) and we store these in all child docshells. And we only turn them into a real Paint marker for the docShell(s) that actually had layers drawn (in FrameLayerBuilder::DrawThebesLayer) in between start and stop.
Thing is, I just realized that the docShell we get via |presContext->GetDocShell()| in this method is the one for the parent frame when paint occurs in the child frame.

Comment 13

[Patrick Brosset <:pbro>]

I'm trying to find a place where we know when a docShell is being painted.

My use case is the following: http://bgrins.github.io/devtools-demos/inspector/iframe.html
This page contains a few iframes, but we're only interested in the first, the one that has a markup mutation every second.

Now, I'm logging the document.title (using something like |docShell->GetWindow()->GetDoc()->GetTitle()|) in the following methods:
- nsLayoutUtils::PaintFrame (docShell in this method being |aFrame->PresContext()->GetDocShell()|)
- FrameLayerBuilder::PaintItems (docShell being |aPresContext->GetDocShell()| here)
- FrameLayerBuilder::DrawThebesLayer (same)

And every second, when the child iframe content changes and gets painted, my logs tell me that it was the parent page that built the layer (or painted the frame).

Is there a place where I can safely get the child iframe's docShell whenever the mutation occurs?

Comment 14

[Benoit Girard (:BenWa)]

The painting that happens is pretty intertwined. We paint for the window at once. I don't think we can really get a breakdown of how much time was spent painting what without adding a lot more instrumentation.

Basically we will go through our layer tree and update them all at once. Assuming we don't have a layer that mixes content from more then one docshell together (feels like a bad assumption) then we would have to keep track of which docshell a layer belongs to and as we start painting we would have to track.

Even with all that there's still times where it's not clear which layer you're working on like when we're working on our display list.

Matt can you confirm?

Comment 15

[Patrick Brosset <:pbro>]

Attachment: bug1050773-1-timeline-actor-record-all-docshells v1.patch

So that's the patch for part 1, it makes the timeline actor pull markers from all current docshell in the tabactor.

I've added a new test for it and cleaned up some of the tests in the same directory at the same time.

Comment 16

[Patrick Brosset <:pbro>]

Attachment: bug1050773-1-timeline-actor-record-all-docshells v2.patch

Thanks for the review Paul.
I had some try failures because of the test cleanup, so here's a new patch that fixes those.
Pending try build: https://tbpl.mozilla.org/?tree=Try&rev=10783e523a48

Comment 17

[Patrick Brosset <:pbro>]

Attachment: bug1050773-1-timeline-actor-record-all-docshells v3.patch

Rebased, and landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/a48989ed6aa6

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a48989ed6aa6

Comment 19

[Patrick Brosset <:pbro>]

Discussed with Paul: at least for now, we can make sure the timeline actor always listens to all frames, so that we are sure to get paint markers. This means that even if the user switches to a sub frame using the iframe switcher drop-down menu, we should still listen to the parent frame.

Comment 20

[Patrick Brosset <:pbro>]

There's another bug related to iframe that actually depends on bug 1070089 being fixed: Say you record timeline markers on a page that has an iframe, and you then, later, refresh the iframe.
In that case, attachment 8491344 [details] [diff] [review] (which already landed) will make sure the refreshed iframe is recorded too, but its record start time is going to restart from 0 while the parent page has been recording for some time already, so markers from the child frame aren't going to be visible.

Comment 21

[Patrick Brosset <:pbro>]

Attachment: bug1050773-2-always-record-root v1.patch

This makes sure we always record markers from the top level window, so that we don't miss paint markers.
It would be more ideal to be able to track paint markers per window, but since we can't, it seems fine enough in v1 to at least do that.

Comment 22

[Patrick Brosset <:pbro>]

Attachment: bug1050773-2-always-record-root v2.patch

Minor last minute change as I had some test failures on try.
There isn't always a tabActor.originalWindow available, so when this is the case, just default to using tabActor.window.

Comment 23

[Patrick Brosset <:pbro>]

Pending try build: https://tbpl.mozilla.org/?tree=Try&rev=171eec8557d4

Comment 24

[Patrick Brosset <:pbro>]

Try is green, so pushed this part 2 patch to fx-team: https://hg.mozilla.org/integration/fx-team/rev/11ba7e5e8834

Leaving the bug open as Matt may have feedback about how to track paint markers at the right docshell level.
Also, Paul was investigating earlier how to force refresh drivers in nested iframes, which could help with this too.

Comment 25

[Paul Rouget [:paul]]

Attachment: forceOwnRefreshDriver

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #24)
> Also, Paul was investigating earlier how to force refresh drivers in nested
> iframes, which could help with this too.

This is what I tried:
- added a new flag to the docshell "forceOwnRefreshDriver"
- layout/base/nsPresContext.cpp: force ownRefreshDriver if docshell.forceOwnRefreshDriver
- to re-init the contentViewer:
- mark iframe's docshell non-sticky
- hide contentViewer
- showContenViewer

>  let d = frame.QueryInterface(Ci.nsIInterfaceRequestor)
>               .getInterface(Ci.nsIWebNavigation)
>               .QueryInterface(Ci.nsIDocShell);
>  
>  d.forceOwnRefreshDriver = true;
>  d.contentViewer.sticky = false;
>  d.contentViewer.hide();
>  d.contentViewer.show();


The logs tell me that the iframe is indeed using its own refreshDriver.

But apparently, we still miss the paints :(

Comment 26

[Matt Woodrow (:mattwoodrow)]

What BenWa said was correct.

What do you mean by having an iframe use its own RefreshDriver? Every PresShell (document/iframe etc) always has a refresh driver for driving style/layout flushes, requestAnimationFrame callbacks etc. However, only the refresh driver for the root document for a given window ever drives painting.

Content within an iframe isn't separated from content from the parent document from a displaylist/layers perspective, so its going to be very hard to measure these separately. One option would be to change layerization heuristics when the timeline is enabled to force content from an iframe into a separate layer sub-tree (we do this for the root content document to separate it from the chrome). Another options would be to split up the paint measurement and measure each individual item we paint (background, border, image etc), and then attribute that time to the appropriate document.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/11ba7e5e8834

Comment 28

[Paul Rouget [:paul]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #26)
> What BenWa said was correct.
> 
> What do you mean by having an iframe use its own RefreshDriver? Every
> PresShell (document/iframe etc) always has a refresh driver for driving
> style/layout flushes, requestAnimationFrame callbacks etc. However, only the
> refresh driver for the root document for a given window ever drives painting.

There's obviously something wrong with my understanding of refresh drivers :)

For example, look at nsPresContext.cpp, I see that we use the refreshDriver
from the parent docshell:

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp#1006

> Content within an iframe isn't separated from content from the parent
> document from a displaylist/layers perspective, so its going to be very hard
> to measure these separately.

I thought that what "forceOwnRefreshDriver" was doing:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp#1002

> One option would be to change layerization
> heuristics when the timeline is enabled to force content from an iframe into
> a separate layer sub-tree (we do this for the root content document to
> separate it from the chrome). Another options would be to split up the paint
> measurement and measure each individual item we paint (background, border,
> image etc), and then attribute that time to the appropriate document.

Comment 29

[Patrick Brosset <:pbro>]

(In reply to Matt Woodrow (:mattwoodrow) from comment #26)
> One option would be to change layerization
> heuristics when the timeline is enabled to force content from an iframe into
> a separate layer sub-tree (we do this for the root content document to
> separate it from the chrome).
That sounds like a good idea to me! Is this going to degrade performance in a noticeable way?

Comment 30

[Matt Woodrow (:mattwoodrow)]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #28)

> There's obviously something wrong with my understanding of refresh drivers :)
> 
> For example, look at nsPresContext.cpp, I see that we use the refreshDriver
> from the parent docshell:
> 
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.
> cpp#1006
> 

Hrm, maybe there's something wrong with my understanding of refresh drivers too, hadn't seen that code before.

Anyway, this shoudn't affect that painting is always driven by the 'root' refresh driver.


(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #29)
> That sounds like a good idea to me! Is this going to degrade performance in
> a noticeable way?

It shouldn't do! It'll increase memory usage for iframes since we'll have pixels for the iframe as well as the content behind it, but that's unlikely to matter for a debugging option.

The decision is made in nsSubDocumentFrame [1], which currently makes sure we have a layer wrapping the root content document and subdocuments that are currently being scrolled. If we make that true for iframes that we are trying to track from devtools, then we can guaranteee that all layers for the iframe are within a single subtree of the layer tree.


[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp#448

Comment 31

[Victor Porof [:vporof][:vp]]

Moving into the Profiler component. Filter on GUTHRIE'S WAVY CAKES.

Comment 32

[Jordan Santell [:jsantell] [@jsantell]]

Read through this, and I don't quite understand -- is this about recording markers for iframes? Is this still an issue?

Comment 33

[Paul Rouget [:paul]]

All of the initial concerns I had are now covered.

Comment 34

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.