791670 - [New Tab Page] enable new tab page preloading by default

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Tracking bug for enabling new tab page preloading by default.

Comment 1

[Dão Gottwald [:dao]]

This doesn't affect tab animations anymore, as far as I can tell. Bug 752839 originally tracked this.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 1 - flip the browser.newtab.preload pref

Comment 4

[Avi Halachmi (:avih)]

While I wouldn't consider it a regression for preload, I added some measurements at bug 752839 comment #11, which indicate that preload doesn't really solve the sluggish animation on a slow PC.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 2 - fix browser_bug763468_perwindowpb.js test

We need to fix browser_bug763468_perwindowpb.js as that's waiting for new tab to load when in fact it's just swapped in.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

With both of these patches and the one from bug 794041 applied, try finally looks good:

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

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Pushed to try again and there's a lot of intermittent failures that need fixing.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 3 - fix newtab page tests

The fixes here are:

* There were a couple of tests that disable the newtab page but didn't re-enable it before finishing. Not sure why it didn't cause problems before but that's fixed now.

* whenNewTabLoaded() in addNewTabPageTab() needs to use executeSoon() so that when we have a preloaded and disabled newtab page, TestRunner.next() isn't called synchronously. That doesn't work with the TestRunner and fails because the iterator is already running.

* Now that there are multiple newtab pages loaded/loading concurrently while running the test suite there is a race condition after a page has registered but its grid isn't initialized, yet. Page.update() shouldn't do anything if the grid isn't ready.

With all those fixes try is finally green again:

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

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 755869 [details] [diff] [review]
part 3 - fix newtab page tests

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

(In reply to Tim Taubert [:ttaubert] from comment #8)
> * Now that there are multiple newtab pages loaded/loading concurrently while
> running the test suite there is a race condition after a page has registered
> but its grid isn't initialized, yet. Page.update() shouldn't do anything if
> the grid isn't ready.

Does Page.update() get called eventually with gGrid.ready returning true?

::: browser/base/content/test/newtab/head.js
@@ +220,5 @@
>        NewTabUtils.links.populateCache(function () {
>          executeSoon(TestRunner.next);
>        });
>      } else {
> +      executeSoon(TestRunner.next);

Please add a comment here about the iterator already running.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Jared Wein [:jaws] from comment #9)
> Does Page.update() get called eventually with gGrid.ready returning true?

It doesn't but that's okay because the grid hasn't been rendered, yet. When the grid will eventually be rendered it will have up-to-date data. This function exists only to update existing grids that have already been rendered.

> >      } else {
> > +      executeSoon(TestRunner.next);
> 
> Please add a comment here about the iterator already running.

Will do.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 664455 [details] [diff] [review]
part 1 - flip the browser.newtab.preload pref

Looks like we'll be ready to land this soon \o/

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 664455 [details] [diff] [review]
part 1 - flip the browser.newtab.preload pref

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

rs=me \o/

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/94e5027d22a4
https://hg.mozilla.org/integration/fx-team/rev/bbac98ae1fea
https://hg.mozilla.org/integration/fx-team/rev/7f370b2b1317

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/94e5027d22a4
https://hg.mozilla.org/mozilla-central/rev/bbac98ae1fea
https://hg.mozilla.org/mozilla-central/rev/7f370b2b1317

Comment 15

[Matt Brubeck (:mbrubeck)]

These patches caused regressions in the Tp5 benchmark (and possibly other pageloader tests) on all platforms:

https://groups.google.com/d/topic/mozilla.dev.tree-management/dcNTQnhWXZA/discussion

Comment 16

[Dão Gottwald [:dao]]

(In reply to Matt Brubeck (:mbrubeck) from comment #15)
> These patches caused regressions in the Tp5 benchmark (and possibly other
> pageloader tests) on all platforms:

It should probably be backed out then.

I suspect the underlying cause is the change in bug 875509 that would load about:newtab in the background while web content is loading.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

AFAIK, the talos pageloader tests don't involve a tabbrowser and thus do not open and close tabs:

http://hg.mozilla.org/build/talos/file/default/talos/pageloader/chrome

The code from bug 875509 should therefore actually not interfere with those tests. Maybe talos is affected more indirectly?

Comment 18

[Avi Halachmi (:avih)]

(In reply to Matt Brubeck (:mbrubeck) from comment #15)
> These patches caused regressions in the Tp5 benchmark (and possibly other
> pageloader tests) on all platforms:
> 
> https://groups.google.com/d/topic/mozilla.dev.tree-management/dcNTQnhWXZA/
> discussion

This message shows regression at |Trace Malloc MaxHeap|.

(In reply to Tim Taubert [:ttaubert] from comment #17)
> AFAIK, the talos pageloader tests don't involve a tabbrowser and thus do not
> open and close tabs:

AFAIK, the normal pageloader tests do load a normal browser with one tab at which the pages are loaded. The no-chrome tests do not.

Also, note that stddev went from 8k to 40k, but since it's over a value of 34M, stddev is still 0.1%, which I find quite extraordinary still, though less for for MaxHeap than for tp5.

Comment 19

[Avi Halachmi (:avih)]

(In reply to Avi Halachmi (:avih) from comment #18)
> Also, note that stddev went from 8k to 40k, ...

Ermm 8k to 400k.

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

maxheap is not a talos test, this is run during build time:
https://wiki.mozilla.org/Buildbot/Talos/Tests#Other_data

I am not too familiar with this test, I believe it just launches the browser with a single tab and loads some pages.

Comment 21

[Matt Brubeck (:mbrubeck)]

(In reply to Avi Halachmi (:avih) from comment #18)
> (In reply to Matt Brubeck (:mbrubeck) from comment #15)
> > These patches caused regressions in the Tp5 benchmark (and possibly other
> > pageloader tests) on all platforms:
> > 
> > https://groups.google.com/d/topic/mozilla.dev.tree-management/dcNTQnhWXZA/
> > discussion
> 
> This message shows regression at |Trace Malloc MaxHeap|.

Read the entire thread.  The complete list of benchmarks that regressed is:
* Trace Malloc MaxHeam
* Tp5 Optimized
* Tp5 Optimized (XRes)
* SVG, Opacity Row Major

See also this thread for the Mac OS X regression mails:
https://groups.google.com/d/topic/mozilla.dev.tree-management/plTVr31M6A0/discussion

Comment 22

[Matt Brubeck (:mbrubeck)]

I filed bug 881590 to track the Talos regressions.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Backed out part 1 for causing bug 881590:

https://hg.mozilla.org/integration/fx-team/rev/993ea1d1efaf

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/993ea1d1efaf

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Re-landed together with the fix for bug 881590:

https://hg.mozilla.org/integration/fx-team/rev/825e5d09c5ec

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Backed out again because bug 881590 leaks on OS X:

https://hg.mozilla.org/integration/fx-team/rev/801ada6caf7c

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/8f5749eb49f6

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8f5749eb49f6

Comment 29

[Alex Keybl [:akeybl]]

Adding the feature keyword to be included in the new Release Tracking page.

Comment 30

[bhavana bajaj [:bajaj]]

"Performance improvements for new tab page loading" as a release note.