961249 - PresShell::Initialize may invalidate lots of frames

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Olli Pettay [:smaug][[e-mail]]]

In my profile I tend to have 150+ tabs open.
When (re)loading page[1] there is some jank, and based on
the profiles, lots of it is coming from PresShell::Initialize calling 
InvalidateFrameSubtree() and then at later state we clear all the invalidation bits.
If I remove that InvalidateFrameSubtree(); (around NS_FRAME_NO_COMPONENT_ALPHA handling)
page loading with lots of tabs open feels as fast as it is with just one tab open.

It is not clear to me why I have NS_FRAME_NO_COMPONENT_ALPHA set somewhere.

If I read the code correctly, calling InvalidateFrameSubtree may in fact
invalidate all the frames in the whole browser window, even in the background tabs.

[1] http://ilmatieteenlaitos.fi/saa/Tampere

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

PresShell::Initialize+Invalidate and then handling and clearing invalidation flags take 15-20% of the page load in this case.
(I haven't managed to find a minimized testcase, but code inspection should be enough here)

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Looks like odd things start to happen when one has enough tabs to overflow the tabstrip

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

And usually we set NS_FRAME_NO_COMPONENT_ALPHA flag to the root frame of the browser.xul document 
when opening a new page.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

...or when reloading some tab. But apparently not every tab triggers it.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

And we managed to actually set the flag twice.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

er, we manage to set it twice during a reload.

Comment 7

[Mats Palmgren (inactive)]

I wonder if we should break out of that loop if "f->PresContext()->IsChrome() !=
mPresContext->IsChrome()" ?
http://hg.mozilla.org/mozilla-central/annotate/b3c08e6fa790/layout/base/nsPresShell.cpp#l1779

Comment 8

[Jet Villegas (inactive)]

To Mats for a look

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

This is also 26% of devtools opening time, even in the case browser has just one about:blank tab -
at least in this profile I'm looking at.

Comment 10

[Mats Palmgren (inactive)]

Attachment: wip

Olli, does this help?

Comment 11

[Timothy Nikkel (:tnikkel)]

InvalidateFrameSubtree probably shouldn't be going into hidden tabs and invalidating, they're hidden, so by definition don't need any invalidation.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Seems to help.

Comment 13

[Mats Palmgren (inactive)]

Attachment: wallpaper

Since InvalidateFrameSubtree recurse into all children, it seems slightly
more efficient to call it on just the top-most ancestor rather than every
ancestor along the way (with the frame bit set).
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?rev=257d152e487f#4850

(In addition to the first suggested patch to stop at a pres-context
with a different IsChrome()-ness as the current one.)

Comment 14

[Mats Palmgren (inactive)]

I tried to reproduce bug 777924, with the suggested add-ons installed, but I couldn't.
I did add a printf to detect this case (invalidating a pres-context with different
chrome-ness) and I can reproduce that, it just doesn't seem to lead to any paint
errors for me.  So if anyone can reproduce that (by just commenting out the
InvalidateFrameSubtree call in the loop), then I'd appreciate if you can verify that
the suggested wallpaper in this bug doesn't cause a regression.

Comment 15

[Mats Palmgren (inactive)]

roc, I forgot to check that it's green on Try, perhaps you should hold the review until then:
https://tbpl.mozilla.org/?tree=Try&rev=8d489b666d3c

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8396077 [details] [diff] [review]
wallpaper

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

::: layout/base/nsPresShell.cpp
@@ +1805,5 @@
>  
> +  nsIFrame* needInvalidateFrameSubtree = nullptr;
> +  const bool isChrome = mPresContext->IsChrome();
> +  for (nsIFrame* f = rootFrame;
> +       f && f->PresContext()->IsChrome() == isChrome;

I don't think this check is correct. I think we need to clear NS_FRAME_NO_COMPONENT_ALPHA in the chrome when a new content document loads. The rest is OK though.

Comment 17

[Mats Palmgren (inactive)]

So if that implies that we also need to call InvalidateFrameSubtree on that
frame (in addition to clearing the bit) then we're back to square one with
regards to the perf problem.  I wonder if it would be enough to call
InvalidateFrame on those?  (i.e. call InvalidateFrameSubtree as in the
patch, but go on and clear the bit + call InvalidateFrame on those above).

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The first time InvalidateFrameSubtree is called on the root frame of the chrome, it will set NS_FRAME_ALL_DESCENDANTS_NEED_PAINT on every frame in every document on the window, including pages in background tabs. The problem is that when we finish painting the window, we call ClearInvalidationStateBits on the root which clears all those bits again. So we keep setting and clearing these bits.

I think the best option here is to make InvalidateFrameSubtree refuse to mark frames invalid that are in a hidden document. We could even go further and pass around a dirty rect to avoid marking frames that are outside the last displayport. Or, we could use a new state bit to indicate that a frame or its descendants has an associated display item retained for it and effectively replace "invalid bits" with "valid bits".

Matt, any thoughts?

Comment 19

[Mats Palmgren (inactive)]

Attachment: wip3

Something like this then?

I found one way of hitting this code path - on the Bugzilla Preferences
page, navigate the "General Prefs", "Email Prefs" etc a few times,
then go Back in history - when you do, all browser tabs gets invalidated.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Swapping from an invalid bit to a valid bit makes the most sense to me. That way we can also avoid walking offscreen frames in the visible tab.

For setting the bits after a paint, we'd have to walk the display list again (instead of the frame tree) setting the bit for every frame we find.

We could try setting the bits while doing layer building (to avoid walking the display list again), but we'd need to be sure we didn't modify the bit before all display items for a given frame had been processed.

I guess we could have display list building also build a list of all unique frame pointers found, and then just walk that.

Comment 21

[Mats Palmgren (inactive)]

Sounds like Matt has a better solution. ;-)

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Matt, can you take this? I think we should fix this. If no, I'll take it.

Comment 23

[Matt Woodrow (:mattwoodrow)]

I'm going to be in Toronto for a work week, so I probably won't be able to get to it this week. If it's not urgent then I can do it though.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Any news? This shows up in my FF usage all the time rather badly.
Up to 50% of page load time can be invalidation
(I admit I have couple of tabs with single page HTML spec open).

Comment 25

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't invalidate more than we need to

Does this help?

I'd still like to do the original fix, but I think this is worthwhile (and probably sufficient for your bug).

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(Isn't it propagate, not propogate.)

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8459367 [details] [diff] [review]
Don't invalidate more than we need to

Seems to help
(though I got some odd hang while testing, could be unrelated. Trying to reproduce that.)

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

(that hang is unrelated. It is about the net cache badness recent nightlies have)

Comment 29

[Matt Woodrow (:mattwoodrow)]

Sweet, let's take this for now then and I'll try get the bigger change done soonish. And I'll fix the spelling mistake before landing :)

Comment 30

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d3378b935d7

Comment 31

[Matt Woodrow (:mattwoodrow)]

Actually, I no longer like the approach of switching invalid bits to valid bits.

Any time we invalidate a frame we need to walk up the frame tree looking for SVG rendering observers to notify them of the change.

We currently track NS_FRAME_DESCENDANT_NEEDS_PAINT so that we only need to walk up the nearest common ancestor of previously invalidated frames which avoids a lot of frame traversal.

With only valid bits we wouldn't have this information, and that would likely make a lot of common cases slower.

One option would be to add flags to all frames that have an ancestor with a SVG rendering observer, but that sounds like a lot of work.

If the current patch has fixed the performance issue, then I'm happy to leave it as is.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d3378b935d7