Talos Tp5 (and other) regressions from new tab page preloading

RESOLVED FIXED in Firefox 24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mbrubeck, Assigned: ttaubert)

Tracking

({perf, regression})

24 Branch
Firefox 24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox23 unaffected, firefox24+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Bug 791670 (enabling browser.newtab.preload) caused regressions in Tp5, SVG Opacity, and Trace Malloc benchmarks:

https://groups.google.com/d/topic/mozilla.dev.tree-management/7P2GK0ELOjY/discussion
https://groups.google.com/d/topic/mozilla.dev.tree-management/plTVr31M6A0/discussion

For example:

Regression: Fx-Team - Tp5 Optimized - MacOSX 10.8 - 6.81% increase
------------------------------------------------------------------
    Previous: avg 209.142 stddev 1.599 of 12 runs up to revision b01fedc71f50
    New     : avg 223.389 stddev 2.388 of 12 runs since revision 7f370b2b1317
    Change  : +14.247 (6.81% / z=8.908)
    Graph   : http://mzl.la/16WeO91

Changeset range: http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=b01fedc71f50&tochange=7f370b2b1317
It looks like the memory regression is in Trace Malloc Allocs?  If so, that's expected, and not a big deal from our PoV.  I'd be much more concerned if we saw FF actually using more memory (as opposed to simply calling malloc more).
Whiteboard: [MemShrink]
The Trace Malloc regressions are indeed expected. Not expected though is a regression in Tp5 and SVG Opacity. tsvgr_opacity is down by over 200% for PGO builds. I investigated this with Avi and we found out there seems to be extra painting once the <browser> has been loaded in the hidden window.

This looks a lot like bug 786484.
Turns out that just creating a blank iframe in the hiddenDOMWindow, putting it into the DOM and stopping its load makes us paint twice. So to be clear:

> let doc = Services.hiddenDOMWindow.document;
> let iframe = doc.createElementNS(HTML_NS, "iframe");
> doc.documentElement.appendChild(iframe);

This works fine, while this doesn't:

> let doc = Services.hiddenDOMWindow.document;
> let iframe = doc.createElementNS(HTML_NS, "iframe");
> doc.documentElement.appendChild(iframe);
> let webNav = iframe.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>                                  .getInterface(Ci.nsIWebNavigation);
> webNav.stop(Ci.nsIWebNavigation.STOP_NETWORK);

The same thing can be triggered if we change the iframe window's location before it has finished loading (I guess there's an implicit stop() call). The double painting only occurs with STOP_NETWORK and does not happen if I comment out mLoadGroup->Cancel() here:

http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsDocLoader.cpp#268

Another thing I could observe on my system is that nsViewManager::PaintWindow() is called twice (when reloading an SVG test page) because of two GTK "expose-event" notifications. It's called only once without the stopped iframe in the hiddenWindow. Not sure what's happening here.
I surrender. I'm not able to find out what exactly is causing the talos regressions but I do know how to work around them. The one thing we must not do is stop/cancel loads in the hiddenWindow.


I pushed some patches to try:

[1] Current trunk with preload=false:
https://tbpl.mozilla.org/?tree=Try&rev=e28bfbdcb997

[2] Trunk with preload changed to 'true':
https://tbpl.mozilla.org/?tree=Try&rev=68dabd0c6e51

[3] Trunk with preload=true and the attached patch:
https://tbpl.mozilla.org/?tree=Try&rev=eb5a43ca2e82


Talos comparisons are as follows:

old:[1] new:[2]
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=e28bfbdcb997&newRev=68dabd0c6e51&submit=true

As that's just trunk with preload flipped to 'enabled' we're seeing similar regressions as reported in the summary.

old:[1] new:[3]
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=e28bfbdcb997&newRev=eb5a43ca2e82&submit=true

The new patch looks much better without any SVG regressions. Main RSS and PBytes are slightly higher but that's expected as there is another page in memory, sitting in the background.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #762918 - Flags: review?(jaws)
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=20df88d4e4ba&newRev=d9782caf1aad&submit=true

Looks like all SVG regressions are gone. Main RSS and Private Bytes are up a little but I think that's to be expected with an active about:newtab instance held in the background.

I don't understand the shutdown paint regression on Mac and Win XP. There also seem to be small TP5 Responsiveness regression but not on all platforms? Maybe they're just noise?
(In reply to Tim Taubert [:ttaubert] from comment #5)
> I don't understand the shutdown paint regression on Mac and Win XP

I triggered 4 more tp5 runs on OS X 10.8. From the 6 runs, one is 817, the five others are 260-270. I think it's a glitch.
(In reply to Avi Halachmi (:avih) from comment #6)
> (In reply to Tim Taubert [:ttaubert] from comment #5)
> > I don't understand the shutdown paint regression on Mac and Win XP
> 
> I triggered 4 more tp5 runs on OS X 10.8. From the 6 runs, one is 817, the
> five others are 260-270. I think it's a glitch.

It looks very much just like a glitch. The 260 from the first run without preload were better than average. Subsequent runs were more like 266+.
I think we're quite confident that the patch is good as is and that there should be no talos regressions (except the 0-2% Main RSS and PBytes).
Attachment #762918 - Flags: review?(jaws) → review+
The previous patch made a stupid mistake, that was holding onto the iframe's contentWindow and then trying to remove() it. We should of course release all handles and remove the parent iframe from the DOM.

The new patch doesn't leak or break tests:
https://tbpl.mozilla.org/?tree=Try&rev=cb4a31681219

It does not regress talos tests:
https://tbpl.mozilla.org/?tree=Try&rev=5ab3bf5cc0aa
https://tbpl.mozilla.org/?tree=Try&rev=f67eaa1ce1f9
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=5ab3bf5cc0aa&newRev=f67eaa1ce1f9&submit=true

I snuck a change to browser_newtab_perwindow_private_browsing.js in here because it permanently failed locally. We should wait for the delayed startup stuff to be finished before opening tabs and assuming things in private windows.
Attachment #762918 - Attachment is obsolete: true
Attachment #765255 - Flags: review?(jaws)
Comment on attachment 765255 [details] [diff] [review]
don't cancel loads when preloading in the hiddenWindow, v2

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

::: browser/modules/BrowserNewTabPreloader.jsm
@@ +11,5 @@
>  const Ci = Components.interfaces;
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");

Why not use Promise.jsm?
Attachment #765255 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] from comment #12)
> >  Cu.import("resource://gre/modules/Services.jsm");
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
> 
> Why not use Promise.jsm?

Didn't know it existed, will use that. There are too many Promises implementations :)
https://hg.mozilla.org/mozilla-central/rev/c36e0340d4e0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
(Assignee)

Updated

6 years ago
Depends on: 904616
You need to log in before you can comment on or make changes to this bug.