1389546 - Set the new tab page as active when preloading enabled

Status: RESOLVED INVALID

(Firefox :: New Tab Page, enhancement)

Description

[Alex Thayer [:alexical] (she/her)]

Attachment: newtab-active.patch

This is a proposal, since it makes the new tab page display effectively instantly, removing the 1+ frame white flicker that we currently see, but there are a few potential tradeoffs.

Talked briefly with mconley about this, and some questions he had were how much extra memory would it use to hold onto the layers that we pre-paint, and can we ensure that it wouldn't be updating itself visually in the background.

Regarding memory, I ran a fairly crude test where I created 100 preloaded new tabs, and compared their memory usage when setting them to active vs not. Without e10s, the activated tabs consumed roughly 100MB more memory, meaning on average they consumed 1MB each.

Regarding updating in the background, I haven't confirmed that the page doesn't do so, but just observing it it doesn't seem to.

The patch is a WIP, there's still some failing tests on try, which mostly seem to do with focus issues.

Ehsan, do you think that it's worth the extra memory usage to do this, or do you know who might make that call?

Comment 1

[(no longer active)]

To answer your main question, Eric is the expert on memory usage, so I'm going to redirect the needinfo to him.

But it would be nice to clarify a couple of things in order to understand the trade-off better.  Is the extra 1MB of memory used per content process, or is that the maximum additional memory usage cost here?  Also, is this guaranteed to only ever save 1 frame?  Could loading about:newtab potentially take longer than that?  I'm asking because if the answer is yes, that would make this more appealing.

Thanks!

Comment 2

[Alex Thayer [:alexical] (she/her)]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #1)
> But it would be nice to clarify a couple of things in order to understand
> the trade-off better.  Is the extra 1MB of memory used per content process,
> or is that the maximum additional memory usage cost here?  

We only ever have one preloaded new tab page, so the extra 1MB is not per content process.

> Also, is this guaranteed to only ever save 1 frame?  Could loading about:newtab
> potentially take longer than that?  I'm asking because if the answer is yes,
> that would make this more appealing.

Under normal circumstances, it's always 1 frame on my computer. However I ran a few tests while compiling m-c and it took 3 frames each time. I don't have any good reference hardware to see how often >1 frame new tab creations happen on typical computers, but I imagine it's not never.

Comment 3

[(no longer active)]

Thanks.  Both of your answers make this change a lot more palatable to my taste buds.  But at the end of the day I think Eric is the final expert as to whether taking it is the right trade-off.  :-)

Comment 4

[Eric Rahm [:erahm]]

I'm inclined to say this is fine, but can you do two awsy runs on try (one without your patch and one with) with multiple retriggers so we can double check your results? I think |./mach try -b o -p linux64 -u awsy-e10s -t none --rebuild 5| should do the job.

Comment 5

[Alex Thayer [:alexical] (she/her)]

With:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f87bfc84f4a4240bfcaab7de90d70265a6de291
Without:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3659171ee84bb5109449e48d4e76168ad0a1ddb

The difference seems like it didn't even show up? Mean resident memory is about 300KB lower with the patch applied.

Comment 6

[Eric Rahm [:erahm]]

(In reply to Doug Thayer [:dthayer] from comment #5)
> With:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2f87bfc84f4a4240bfcaab7de90d70265a6de291
> Without:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c3659171ee84bb5109449e48d4e76168ad0a1ddb
> 
> The difference seems like it didn't even show up? Mean resident memory is
> about 300KB lower with the patch applied.

Yeah it's in the noise, r=me for memory use.

Comment 7

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1389546 - Set the new tab page as active when preloading

When preloading a new tab, we can save a few frames by setting that
tab as active. Since the few frames saved would otherwise be occupied
by a blank white page, and this change brings the amount of time spent
on that blank white page to 0, this makes the new tab page feel instant,
instead of almost instant.

There's two drawbacks to this that I can identify, but I think they're
worth it. One is that it has to use slightly more memory - however from
running awsy runs on try this extra memory doesn't stand out over the
noise. The other is that the new tab page does not update in real time,
which is a plus on one hand since we don't have to redraw in the
background, but it is a minus since it means we can have stale history
in the new tab page. If a new user visits 20 pages and then creates a
new tab, without this preloading they would see that history show up
in the new tab page - with this change, however, they'll see the new
tab that was preloaded at the beginning of their session, which has
none of that history. See the changes to the browser_newtab_update test
for details.

Review commit: https://reviewboard.mozilla.org/r/169836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169836/

Comment 8

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

Comment on attachment 8898490 [details]
Bug 1389546 - Set the new tab page as active when preloading

https://reviewboard.mozilla.org/r/169836/#review176352

I like the idea here, and I'm glad we're exploring this. I want to be cautious though, because we're going to necessarily be doing more work now than we have in the past, and doing it in the background.

So I'd like to see a push to try running Talos to compare this against its base revision to see if we're regressing anything there.

I'm also a little confused about why the change in ESM is needed, so I'm hoping you can expand on that. :)

Thanks!

::: browser/base/content/tabbrowser.xml:2996
(Diff revision 1)
>              // Invalidate hovered tab state tracking for this closing tab.
>              if (this.tabContainer._hoveredTab == aTab)
>                aTab._mouseleave();
>  
>              if (newTab)
> -              this.addTab(BROWSER_NEW_TAB_URL, {skipAnimation: true});
> +              this.addTab(BROWSER_NEW_TAB_URL, {skipAnimation: true, aFocusUrlBar: true});

I assume this was for a failing test? Which test was failing?

::: docshell/base/nsIDocShell.idl:659
(Diff revision 1)
>     * like image frame discarding. Docshells are active unless told otherwise.
>     */
>    attribute boolean isActive;
>  
>    /**
> +   * TODO

TODO

::: dom/events/EventStateManager.cpp:1135
(Diff revision 1)
>    // Only forward accesskeys for the active tab.
>    bool active;
>    aTabParent->GetDocShellIsActive(&active);
> -  if (active) {
> +  bool preloaded;
> +  aTabParent->GetDocShellIsPreloaded(&preloaded);
> +  if (active && !preloaded) {

Can you talk a bit about why this is necessary? I'm not sure I fully understand.

::: dom/interfaces/base/nsITabParent.idl:25
(Diff revision 1)
>      * Manages the docshell active state of the remote browser.
>      */
>    attribute boolean docShellIsActive;
>  
>    /**
> +    * TODO

TODO

Comment 9

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8898490 [details]
Bug 1389546 - Set the new tab page as active when preloading

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169836/diff/1-2/

Comment 10

[Alex Thayer [:alexical] (she/her)]

Made some changes and tried to document a little better, so hopefully that addresses your questions!

Re:

> -              this.addTab(BROWSER_NEW_TAB_URL, {skipAnimation: true});
> +              this.addTab(BROWSER_NEW_TAB_URL, {skipAnimation: true, aFocusUrlBar: true});

I ditched this in favor of setting STATE_LOADED at the end of addTab if we consumed a fully preloaded browser. This was failing two tests (browser_bug455852 and browser_bug520538), because the URL bar wasn't properly being focused. This boiled down to this line: http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#4299, and the fact that we aren't able to observe the MozLayerTreeReady for the preloaded browser with the same mechanism.

Regarding Talos, I have some results in here:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=2ce80c39e64fabef1066f8a65efdbb79ef1dfbb8&newProject=try&newRevision=9f5044ab28b6000e3109e4e178b70bffb9be9d77&framework=1&showOnlyImportant=0

I don't have a great intuition for interpreting the significance of Talos results yet - the gain on tart seems like it makes some amount of sense - I don't know how to interpret the regressions of tsvg_static though. Thoughts?

Comment 11

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

Comment on attachment 8898490 [details]
Bug 1389546 - Set the new tab page as active when preloading

https://reviewboard.mozilla.org/r/169836/#review178182

Thanks! This good looks great. I like where this is going, and the STATE_LOADED thing seems like the right track, but a few things still:

a) You should be aware of bug 1353013 which touches code in the same area, and might accidentally conflict with you depending on whether or not it lands first.
b) I'm noticing very, very short tab switch spinners when opening preloaded newtabs... we should be avoiding those. Are you seeing them?

Comment 12

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

s/This good/This code

Comment 13

[Alex Thayer [:alexical] (she/her)]

Hmm - it looks like the STATE_LOADED change is failing asserts in onLayersCleared in debug mode. Looking into it.

Comment 14

[Alex Thayer [:alexical] (she/her)]

Closing this so as to not mislead anyone. I should have commented earlier, but the white flash going away due to my patch was actually due to unintended side effects in tabbrowser.xml, resulting in this path[1] being taken, which caused the flash to visually go away. If we want to massage this logic for the new tab page, that should probably be taken in a separate bug.


[1]: http://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/browser/base/content/tabbrowser.xml#4544