Closed Bug 791670 Opened 12 years ago Closed 11 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 24
Tracking Status
relnote-firefox --- 24+

People

(Reporter: ttaubert, Assigned: ttaubert)

References

(Depends on 1 open bug)

Details

(Keywords: feature, Whiteboard: [Snappy])

Attachments

(3 files)

Tracking bug for enabling new tab page preloading by default.
This doesn't affect tab animations anymore, as far as I can tell. Bug 752839 originally tracked this.
No longer blocks: 593680
Blocks: 792922
Depends on: 794041
Depends on: 799495
Depends on: 799501
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.
Attachment #664455 - Attachment description: flip the browser.newtab.preload pref → part 1 - flip the browser.newtab.preload pref
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.
Attachment #709143 - Flags: review?(ehsan)
With both of these patches and the one from bug 794041 applied, try finally looks good:

https://tbpl.mozilla.org/?tree=Try&rev=b93bbe2fac84
Attachment #709143 - Flags: review?(ehsan) → review+
Depends on: 842607
Depends on: 875257
No longer depends on: 786484
Depends on: 875496
Depends on: 875509
Blocks: 843853
Pushed to try again and there's a lot of intermittent failures that need fixing.
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
Attachment #755869 - Flags: review?(jaws)
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.
Attachment #755869 - Flags: review?(jaws) → review+
(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.
Depends on: 878801
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/
Attachment #664455 - Flags: review?(jaws)
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/
Attachment #664455 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/94e5027d22a4
https://hg.mozilla.org/mozilla-central/rev/bbac98ae1fea
https://hg.mozilla.org/mozilla-central/rev/7f370b2b1317
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Blocks: 879752
No longer blocks: 879752
Depends on: 880308
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
(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.
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?
(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.
Flags: needinfo?(jmaher)
(In reply to Avi Halachmi (:avih) from comment #18)
> Also, note that stddev went from 8k to 40k, ...

Ermm 8k to 400k.
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.
Flags: needinfo?(jmaher)
(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
Depends on: 881590
I filed bug 881590 to track the Talos regressions.
Backed out part 1 for causing bug 881590:

https://hg.mozilla.org/integration/fx-team/rev/993ea1d1efaf
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 883099
https://hg.mozilla.org/mozilla-central/rev/8f5749eb49f6
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Depends on: 879752
Depends on: 888972
Depends on: 880101
Adding the feature keyword to be included in the new Release Tracking page.
Keywords: feature
"Performance improvements for new tab page loading" as a release note.
See Also: → 1548683
You need to log in before you can comment on or make changes to this bug.