Last Comment Bug 791670 - [New Tab Page] enable new tab page preloading by default
: [New Tab Page] enable new tab page preloading by default
Status: RESOLVED FIXED
[Snappy]
: feature
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: Firefox 24
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
: 792942 (view as bug list)
Depends on: 880308 753448 780123 791669 794041 799495 799501 842607 875257 875496 875509 878801 879752 880101 881590 883099 888972
Blocks: tabopen 843853
  Show dependency treegraph
 
Reported: 2012-09-17 06:28 PDT by Tim Taubert [:ttaubert]
Modified: 2013-12-27 14:21 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
24+


Attachments
part 1 - flip the browser.newtab.preload pref (859 bytes, patch)
2012-09-25 05:23 PDT, Tim Taubert [:ttaubert]
jaws: review+
Details | Diff | Splinter Review
part 2 - fix browser_bug763468_perwindowpb.js test (2.88 KB, patch)
2013-02-01 11:09 PST, Tim Taubert [:ttaubert]
ehsan: review+
Details | Diff | Splinter Review
part 3 - fix newtab page tests (3.70 KB, patch)
2013-05-30 04:24 PDT, Tim Taubert [:ttaubert]
jaws: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2012-09-17 06:28:55 PDT
Tracking bug for enabling new tab page preloading by default.
Comment 1 Dão Gottwald [:dao] 2012-09-17 07:37:50 PDT
This doesn't affect tab animations anymore, as far as I can tell. Bug 752839 originally tracked this.
Comment 2 Tim Taubert [:ttaubert] 2012-09-20 13:57:41 PDT
*** Bug 792942 has been marked as a duplicate of this bug. ***
Comment 3 Tim Taubert [:ttaubert] 2012-09-25 05:23:58 PDT
Created attachment 664455 [details] [diff] [review]
part 1 - flip the browser.newtab.preload pref
Comment 4 Avi Halachmi (:avih) 2012-12-19 17:33:36 PST
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] 2013-02-01 11:09:10 PST
Created attachment 709143 [details] [diff] [review]
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] 2013-02-01 13:54:46 PST
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] 2013-05-30 02:34:37 PDT
Pushed to try again and there's a lot of intermittent failures that need fixing.
Comment 8 Tim Taubert [:ttaubert] 2013-05-30 04:24:49 PDT
Created attachment 755869 [details] [diff] [review]
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) 2013-05-30 05:43:05 PDT
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] 2013-05-30 05:51:09 PDT
(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] 2013-06-04 08:45:35 PDT
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) 2013-06-04 09:01:05 PDT
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 15 Matt Brubeck (:mbrubeck) 2013-06-06 15:30:14 PDT
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] 2013-06-06 23:45:50 PDT
(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] 2013-06-10 01:16:21 PDT
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) 2013-06-10 01:44:56 PDT
(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) 2013-06-10 01:50:26 PDT
(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 ) 2013-06-10 05:43:57 PDT
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) 2013-06-10 09:43:23 PDT
(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) 2013-06-10 19:52:59 PDT
I filed bug 881590 to track the Talos regressions.
Comment 23 Tim Taubert [:ttaubert] 2013-06-13 03:55:42 PDT
Backed out part 1 for causing bug 881590:

https://hg.mozilla.org/integration/fx-team/rev/993ea1d1efaf
Comment 24 Ryan VanderMeulen [:RyanVM] 2013-06-13 12:33:15 PDT
https://hg.mozilla.org/mozilla-central/rev/993ea1d1efaf
Comment 25 Tim Taubert [:ttaubert] 2013-06-17 10:50:06 PDT
Re-landed together with the fix for bug 881590:

https://hg.mozilla.org/integration/fx-team/rev/825e5d09c5ec
Comment 26 Tim Taubert [:ttaubert] 2013-06-17 11:47:35 PDT
Backed out again because bug 881590 leaks on OS X:

https://hg.mozilla.org/integration/fx-team/rev/801ada6caf7c
Comment 27 Tim Taubert [:ttaubert] 2013-06-21 14:11:17 PDT
https://hg.mozilla.org/integration/fx-team/rev/8f5749eb49f6
Comment 28 Tim Taubert [:ttaubert] 2013-06-21 21:14:29 PDT
https://hg.mozilla.org/mozilla-central/rev/8f5749eb49f6
Comment 29 Alex Keybl [:akeybl] 2013-09-02 12:47:18 PDT
Adding the feature keyword to be included in the new Release Tracking page.
Comment 30 bhavana bajaj [:bajaj] 2013-09-13 13:31:40 PDT
"Performance improvements for new tab page loading" as a release note.

Note You need to log in before you can comment on or make changes to this bug.