1469403 - We dump display lists from about:newtab a bunch when it's not even visible

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[Kartikaya Gupta (email:[e-mail])]

Start Firefox with a clean profile and do this:
- in about:preferences, check the "Restore previous session" box
- in about:config, set "layout.display-list.dump-content" to true
- close all tabs except for one, and leave that one tab at about:blank

Then shut down firefox. Restart it, recording stdout to a file:

./mach run | tee output.log

As soon as it's open and the about:blank tab is done loading, close it again. Now look at what display lists got painted by examining the logfile.

In my case, of the 16 paints [1] that happened, 12 of them [2] were for about:home, even though about:home was not visible to the user at any point.

This seems like a huge waste. We seem to also repaint about:home every time you alt-tab to and from Firefox.

[1] grep "Painting --- before optimization" output.log | wc -l
[2] grep "FixedPosition" output.log | wc -l (divide the result by 2, because FixedPosition appears in both the "before optimization" and "after optimization" display lists). I used FixedPosition because it appears once on about:home and zero times on about:blank, but there are other things you can search for as well.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Sorry, you also need to capture stderr, that's where most of the output ends up.

./mach run 2>&1 | tee output.log

Comment 2

[Justin Dolske [:Dolske]]

Moving to Core, but FF:ActivityStream could also be a fit if this is AS-specific.

Comment 3

[Andrew Overholt [:overholt]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #0)
> In my case, of the 16 paints [1] that happened, 12 of them [2] were for
> about:home, even though about:home was not visible to the user at any point.
> 
> This seems like a huge waste. We seem to also repaint about:home every time
> you alt-tab to and from Firefox.

How do we go about figuring out what's causing these paints (honest, naive question)?

Comment 4

[Johann Hofmann [:johannh]]

Also, and possibly related, can we write a test for this?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Hmm, I can't reproduce this any more. I tried both on latest nightly and the June 17 nightly using my STR from comment 0 and I don't see the same behaviour. I don't know what happened :(

Let's close this for now, I'll reopen if I figure it out.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Ok I'm seeing this again, on a Linux build from recent m-c. Slightly different STR in that I have two tabs open: one is about:config and the other is https://bug1471578.bmoattachments.org/attachment.cgi?id=8988147 (but a local copy, if that matters). When I start firefox with display-list.dump-content enabled I see the about:home display list show up a bunch of times. I traced it a bit and I think what's happening is that the tab gets a RenderLayers call, and that makes the PuppetWidget "visible" at [1]. This sets the PuppetWidget::mVisible flag to true and that makes the NeedsPaint() function start returning true, and from then onwards at [2] we start going into the code to paint the thing instead of just bouncing out. I don't know what the expected codepaths are for avoiding painting a background tab, but it seems fishy to me that the PuppetWidget thinks its visible when it's clearly not. I'm guessing this is some optimization to pre-paint the layers (tab warming?) that's going awry.

I verified that we're painting the presShell even though mPaintingSuppressed is true on it. I didn't find a codepath that actually aborts painting based on this flag, but it seems wrong that we are painting a suppressed presShell.

/cc mconley who presumably knows more about the RenderLayers stuff, I think that's a thing invoked by the frontend. I'm going to move this bug into Web Painting because it seems like the most appropriate Core component, although this might need to be moved back to the Firefox product.

[1] https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/dom/ipc/TabChild.cpp#2632
[2] https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/view/nsViewManager.cpp#444

Comment 7

[Matt Woodrow (:mattwoodrow)]

I notice this too, can be annoying when trying to look at display list dump, and switching focus to the terminal prints a whole new display list for hidden content.

It seems like it might be intentional to load and paint about:home in the background so that opening a new tab becomes much quicker, though I'll wait for mconley to comment on that.

There's also a platform bug here, that we run a paint cycle for all visible tabs whenever we change window focus. That's because some native themed content (usually the background of the tabstrip!) changes it's rendering depending on if the window is focused is not.

Whenever the window state changes, we schedule paints (which would include for actually hidden, but pretending to be visible documents). That paint will build a full display list (no RDL, since the schedule paint marks the root as being changed), build layers, and then run DLBI. Generally for a content process tab, no actual invalidations would happen, since there's no themed content that uses the window state, and then we'd be done. A lot of work for no gain in the tabs.

Ideally we'd do 'normal' invalidation here, where all frames/elements that paint relevant themed content would register as listeners for the content state change event, and on changes we'd mark the actual affected frames as changed. That'd quickly be a no-op in the content process usually. It'd also mean would could do RDL partial rebuilding for when we do have affected frames.

An easier workaround might just be to flag the whole document as having at least one instance of this themed content when applicable (based on the previous display list build), and only schedule a paint if it's set.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

So I worked a bit with kats and IRC, and we've determined that what you're seeing is painting for the preloaded about:newtab page.

mattwoodrow's instincts are spot on: the preloaded about:newtab page is a perceived performance optimization to make opening new tabs feel faster. That preloading occurs the first time a tab finishes opening, per window. That explains why you're seeing it after your session restores - restoring your session with more than 1 tab caused the preloading to kick in.

The preloading loads the about:newtab document. Up until Firefox 61, that's all it did, but then in bug 1408026, we went a step further and had it render and upload its layers to the compositor. This was to avoid a white flash or delay when opening new tabs.

Note that the preloading can be disabled by setting browser.newtab.preload to false.

So I suspect this is a WONTFIX / WORKSFORME unless this is an unacceptable situation - although I'd want to hear alternatives on how we can make the newtab page appear seemingly-instantaneously.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos / reviews) from comment #8)
> So I suspect this is a WONTFIX / WORKSFORME unless this is an unacceptable
> situation - although I'd want to hear alternatives on how we can make the
> newtab page appear seemingly-instantaneously.

Ok, that's fair.

Maybe to deal with the annoyance of display-list-dump pollution we can skip dumping the display list in cases where the presshell has suppressed painting. We can just output a single line instead ("display list suppressed for presShell with url <whatever>").

Comment 10

[Mike Conley (:mconley) (:⚙️)]

You can also detect paints from non-foreground tabs by checking to see if their DocShells are active. If not, that's a good signal that we're warming them.

Comment 11

[:Gijs (he/him)]

Taking this out of frontend perf triage as it seems the issue isn't really fixable on the frontend side of things.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Based on comment 8/9 I'm going to close this as WONTFIX. We can file a followup to deal with the display list dumping pollution if needed.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

The display list pollution is still super annoying. I'm going to take this bug to skip the DL dumping for non-foreground tabs.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Try push is at https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=b310896517f0913d08d1f0268366df77eb55add3 although I accidentally based it on top of a local patch on top of m-c from a few days ago. But still it should catch any obvious bustage.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1469403 - Set a flag on the DL builder to indicate if the docshell is active. r?mattwoodrow

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1469403 - Skip DL dumps when the DL builder is from an inactive docshell. r?mattwoodrow

Depends on D9141

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1469403 - Skip DL dumps when the DL builder is from an inactive docshell. r?mattwoodrow

Depends on D9141

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9e373554f51
Set a flag on the DL builder to indicate if the docshell is active. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/859e7d50fc03
Skip DL dumps when the DL builder is from an inactive docshell. r=mattwoodrow

Comment 19

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/d9e373554f51
https://hg.mozilla.org/mozilla-central/rev/859e7d50fc03

Comment 20

[Markus Stange [:mstange]]

(Updating summary to reflect what was actually fixed here.)