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

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

unspecified
mozilla21
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(b2g1819+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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.
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.
> We get a new presshell on navigation, right?

Yes.
Freeze() doesn't appear to suppress reflow/painting, so need to keep looking ...
Posted patch Disable reflow/repaint (obsolete) — Splinter Review
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.
Attachment #708316 - Flags: feedback?(bzbarsky)
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.
Set display:none on the relevant iframe node?  Or is there no iframe involved?
I don't think so.  This is the top-level content document in content processes.
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.
We could add some explicit flag to the docshell or content viewer to suppress presshell creation, I suppose...
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...  :(
Attachment #708316 - Flags: feedback?(bzbarsky) → feedback-
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.
Adding some boolean to the presshell that will suppress stuff seems fine to 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
Please.  No hacks like that.  Just add a boolean flag.
Well, my thought was to reuse the mIsDestroying flag.
No fear!  Comment 13 doesn't work anyway (which is a bit scary).
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.
(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.
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?
Attachment #708316 - Attachment is obsolete: true
Attachment #708882 - Flags: feedback?(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?
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.
(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.
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.
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.
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)...
OK!  That worked.  Now we're back to where we were with the navigation.
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.
Assignee: nobody → jones.chris.g
Attachment #708882 - Attachment is obsolete: true
Attachment #708882 - Flags: feedback?(bzbarsky)
Attachment #708924 - Flags: review?
Attachment #708924 - Flags: review? → review?(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
Attachment #708924 - Flags: review?(bzbarsky) → review+
- rebased to inbound
 - added forgotten IID rev
 - added new bail in FlushPendingNotifications()

https://tbpl.mozilla.org/?tree=Try&rev=9d6f5f56982a
Attachment #708924 - Attachment is obsolete: true
Attachment #709876 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c07ddaf74ece
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(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 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.
Attachment #709942 - Flags: approval-mozilla-b2g18?
(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 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
Attachment #709942 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Batch edit: Bugs fixed on b2g18 after 1/25 merge to v1.0 branch are fixed on v1.0.1 branch.
You need to log in before you can comment on or make changes to this bug.