836198 - Spending 20-30 samples (~50ms) reflowing/painting "phony" about:blank

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

After bug 819000, we "load" about:blank very early in content process startup to preinitialize various things, but keep its docshell inactive.

In recent profiles I see this strategy backfiring slightly.  The sequence of events looks like

 1. TabChild::RecvLoadURL("app://foo/index.html").  This initiates an async navigation.
 2. BrowserElementChild::RecvSetVisible(???).  This activates (or inactivates) the docshell ... which still has about:blank loaded.  Either way, the state seems to be changing.
 3. TabChild::UpdateDimensions(). This triggers a flush and reflow of about:blank.  (This takes a surprisingly long time, but it's contending for CPU while running.)
 4. Refresh driver tick.  Paint about:blank.

All this adds up to nontrivial overhead.

I'm not sure how to avoid it though ... will think about it a bit.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We get a new presshell on navigation, right?  Maybe the right thing to do is freeze/whatever the initial wastrel about:blank presshell.  Will poke at that.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> We get a new presshell on navigation, right?

Yes.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Freeze() doesn't appear to suppress reflow/painting, so need to keep looking ...

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Disable reflow/repaint

Is there a way to mutate PresShell to achieve this same effect?  I want to suppress reflow and painting, but without munging the docshell active state.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

If I could destroy the presshell for about:blank out from under the docshell and be assured it wouldn't be recreated, that would be best of all.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Set display:none on the relevant iframe node?  Or is there no iframe involved?

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I don't think so.  This is the top-level content document in content processes.

Comment 8

[Justin Lebar (not reading bugmail)]

Indeed, if this is the preloaded app, there isn't even an iframe in the parent process corresponding to the docshell here, nor is there a TabParent.

Comment 9

[Boris Zbarsky [:bzbarsky]]

We could add some explicit flag to the docshell or content viewer to suppress presshell creation, I suppose...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 708316 [details] [diff] [review]
Disable reflow/repaint

This would totally break websites.  Just because it's about:blank doesn't mean it has no content...  :(

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oh, sorry for the misunderstanding: to re-emphasize

> Is there a way to mutate PresShell to ***achieve this same effect***?

I obviously don't want to land that patch :).

So you're suggesting a new docshell or maybe presshell state is the way to go?  I think in this case we probably do want to create the presshell for the original about:blank load, because that will initialize a lot of things we'll want for the first "real" docshell navigation.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Adding some boolean to the presshell that will suppress stuff seems fine to me.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Aha!  Maybe I can directly call PresShell->Destroy() after we're done with it in early init, and let it live on as a zombie until replaced by the first navigation.

/me smite smite smite

Comment 14

[Boris Zbarsky [:bzbarsky]]

Please.  No hacks like that.  Just add a boolean flag.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Well, my thought was to reuse the mIsDestroying flag.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

No fear!  Comment 13 doesn't work anyway (which is a bit scary).

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Jeff, are you available to help with startup work?  What need here is to create a new "zombie" docshell state, something like SetZombie() in which it
 - never registers with the refresh driver
 - refuses to reflow
 - refuses to repaint

The state transition can only be one way.  We want to set this state from TabChild::PreloadSlowThings() at the end of the function.

Comment 18

[Jeff Muizelaar [:jrmuizel]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #17)
> Jeff, are you available to help with startup work? 

I can help some, but my primary focus right now is Android layers-refactor. If you need this, this week I'd suggest finding someone else. If getting something in the next week or so, I can at least take a look.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Create a "zombie" state for PresShell (doesn't work)

Most likely for the same reason why the Destroy() impl didn't work: we're getting two different presshells for the about:blank load.  The sequence of events looks like

 1. TabChild::PreloadSlowThings()
    - docShell.isActive = false
    - docShell.loadURI("about:blank")
    - docShell->GetPresShell->MakeZombie() [new method added in this patch]

At this point I see

I/Gecko   ( 2374): ERROR: making zombie for 0x4324f200

OK, looking good.  "2374" is the PID.

We're still in the prelaunch state, but soon thereafter I see

I/Gecko   ( 2374): ERROR: reflowing presshell 0x43251200
I/Gecko   ( 2374): ERROR: reflowing presshell 0x43251200
I/Gecko   ( 2374): ERROR: reflowing presshell 0x43251200

WTF?  Two things wrong here: (i) TabChild's docShell got a new presShell; (ii) we're throwing away three reflows on a useless hidden thing.

After that, we

 2. TabChild::RecvLoadURL("app://foo")

 3. TabChild::RecvSetIsVisible(true) [I think]
    - docShell.isActive = true

 4. TabChild::RecvUpdateDimensions()

And then we get, unsurprisingly per above

I/Gecko   ( 2374): ERROR: reflowing presshell 0x43251200

This is the throwaway reflow of about:blank that I want to nuke in this bug.

Why are we seeing two different presshells here?

Comment 20

[Boris Zbarsky [:bzbarsky]]

> WTF?

When you do that first GetPresShell call that creates a synthetic about:blank document.  And then the async loadURI finishes and blows it away.

If all you want is to create a document in there, skip the loadURI call altogether, perhaps?

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yeah, I'm thinking now that the entire effect of the patch in bug 819000 might have just come from touching the docShell in the JS frame script!  Checking.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #21)
> Yeah, I'm thinking now that the entire effect of the patch in bug 819000
> might have just come from touching the docShell in the JS frame script! 
> Checking.

Nope: if I remove that, there's an easily measurable startup regression and I no longer get a presshell from the docshell.

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Explicitly calling |docShell.createAboutBlankContentViewer(null);| instead of navigating to about:blank makes the log look how I want it: get a presshell in PreloadSlowThings(), neuter it, and then don't see any reflows until the first real load.

But it's a huge startup regression :(.  Something about the nav to about:blank or the wastrel reflows must be having a big positive effect.

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

If I docShell.createAboutBlankContentViewer(null), and then *don't* neuter it, then there's no change: still a big regression.  However, we also never attempt to reflow that about:blank.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Aha, yes!

So it's entirely possible that no one ever calls Initialize() on the presshell in that case.  I thought we did it in CreateAboutBlankContentViewer, but I'm not seeing that anywhere now.

Take a look at nsContentSink::StartLayout, where it calls Initialize on the shell.  What you probably want to do is call createAboutBlankContentViewer, then do the Initialize() thing, then flush the pending layout (e.g. via a corresponding FlushPendingNotifications call on the document)...

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

OK!  That worked.  Now we're back to where we were with the navigation.

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Create a "zombie" state for PresShell and use it for the preloaded TabChild PresShell

Incorporates what we discussed here and on IRC.

I'm not sure where else to toss the mIsZombie check, but this patch gets all the samples out of my profiles.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 708924 [details] [diff] [review]
Create a "zombie" state for PresShell and use it for the preloaded TabChild PresShell

Might be good to also toss in an early return at the top of FlushPendingNotifications(mozilla::ChangesToFlush aFlush).  r=me either way

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Create a "zombie" state for PresShell and use it for the preloaded TabChild PresShell, v2

- rebased to inbound
 - added forgotten IID rev
 - added new bail in FlushPendingNotifications()

https://tbpl.mozilla.org/?tree=Try&rev=9d6f5f56982a

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Create a "zombie" state for PresShell and use it for the preloaded TabChild PresShell, v2.1

Didn't rebase hard enough.

https://tbpl.mozilla.org/?tree=Try&rev=c0de60b91ed1

Comment 31

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c07ddaf74ece

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Rebased on gecko-18

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #33)
> https://hg.mozilla.org/mozilla-central/rev/c07ddaf74ece

1.13 +    nsCOMPtr<nsIPresShell> presShell;
1.14 +    if (nsIPresShell* presShell = docShell->GetPresShell()) {

Not sure those were both required...

Comment 35

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 709942 [details] [diff] [review]
Create a "zombie" state for PresShell and use it for the preloaded TabChild PresShell, v2.1

This is a relatively low-risk patch that stops us from doing some fairly significant useless work during early app startup.

Comment 36

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to :Ms2ger from comment #34)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #33)
> > https://hg.mozilla.org/mozilla-central/rev/c07ddaf74ece
> 
> 1.13 +    nsCOMPtr<nsIPresShell> presShell;
> 1.14 +    if (nsIPresShell* presShell = docShell->GetPresShell()) {
> 
> Not sure those were both required...

Rebase mistake, obviously.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4e4e6ce404e

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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

Comment 38

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 709942 [details] [diff] [review]
Create a "zombie" state for PresShell and use it for the preloaded TabChild PresShell, v2.1

Please go ahead with uplift to the tip of mozilla-b2g18 branch for v1.0.1

Comment 39

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/100a4391364b
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d4e8f371d7ac

Comment 40

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs fixed on b2g18 after 1/25 merge to v1.0 branch are fixed on v1.0.1 branch.