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

RESOLVED FIXED in Firefox 24

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

(Depends on: 1 bug, {feature})

Trunk
Firefox 24
feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 24+)

Details

(Whiteboard: [Snappy])

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Updated

5 years ago
Duplicate of this bug: 792942
(Assignee)

Updated

5 years ago
Blocks: 792922
(Assignee)

Updated

5 years ago
Depends on: 794041
(Assignee)

Comment 3

5 years ago
Created attachment 664455 [details] [diff] [review]
part 1 - flip the browser.newtab.preload pref

Updated

5 years ago
Depends on: 799495

Updated

5 years ago
Depends on: 799501

Comment 4

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #664455 - Attachment description: flip the browser.newtab.preload pref → part 1 - flip the browser.newtab.preload pref
(Assignee)

Comment 5

4 years ago
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.
Attachment #709143 - Flags: review?(ehsan)
(Assignee)

Comment 6

4 years ago
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+
(Assignee)

Updated

4 years ago
Depends on: 842607
(Assignee)

Updated

4 years ago
Depends on: 875257
(Assignee)

Updated

4 years ago
No longer depends on: 786484
(Assignee)

Updated

4 years ago
Depends on: 875496
(Assignee)

Updated

4 years ago
Depends on: 875509
(Assignee)

Updated

4 years ago
Blocks: 843853
(Assignee)

Comment 7

4 years ago
Pushed to try again and there's a lot of intermittent failures that need fixing.
(Assignee)

Comment 8

4 years ago
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
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+
(Assignee)

Comment 10

4 years ago
(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.
(Assignee)

Updated

4 years ago
Depends on: 878801
(Assignee)

Comment 11

4 years ago
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+
(Assignee)

Comment 13

4 years ago
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
(Assignee)

Comment 14

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24

Updated

4 years ago
Blocks: 879752

Updated

4 years ago
No longer blocks: 879752

Updated

4 years ago
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.
(Assignee)

Comment 17

4 years ago
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.
(Assignee)

Comment 23

4 years ago
Backed out part 1 for causing bug 881590:

https://hg.mozilla.org/integration/fx-team/rev/993ea1d1efaf
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/993ea1d1efaf
(Assignee)

Comment 25

4 years ago
Re-landed together with the fix for bug 881590:

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

Comment 26

4 years ago
Backed out again because bug 881590 leaks on OS X:

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

Updated

4 years ago
Depends on: 883099
(Assignee)

Comment 27

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/8f5749eb49f6
(Assignee)

Comment 28

4 years ago
https://hg.mozilla.org/mozilla-central/rev/8f5749eb49f6
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED

Updated

4 years ago
relnote-firefox: --- → 24+
(Assignee)

Updated

4 years ago
Depends on: 879752

Updated

4 years ago
Depends on: 888972
(Assignee)

Updated

4 years ago
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.
You need to log in before you can comment on or make changes to this bug.