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

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Matt Brubeck (:mbrubeck)]

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

Comment 1

[Justin Lebar (not reading bugmail)]

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).

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: don't cancel loads when preloading in the hiddenWindow

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.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 6

[Avi Halachmi (:avih)]

(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.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(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+.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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).

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Backed out for OSX-only leaks:

https://hg.mozilla.org/integration/fx-team/rev/716233c37b18

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: don't cancel loads when preloading in the hiddenWindow, v2

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.

Comment 12

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

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?

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(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 :)

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/c36e0340d4e0