Last Comment Bug 827976 - Create hidden windows lazily on non-Mac platforms
: Create hidden windows lazily on non-Mac platforms
Status: NEW
[Snappy:P1] c=startup_misc u= p=
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 5 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: start-faster
  Show dependency treegraph
 
Reported: 2013-01-08 12:36 PST by :Ehsan Akhgari (busy, don't ask for review please)
Modified: 2016-05-27 10:15 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Lazy initialization WIP1 (5.97 KB, patch)
2013-01-28 15:30 PST, Vladan Djeric (:vladan)
ehsan: feedback+
Details | Diff | Review
Remove unused GetHiddenWindowAndJSContext function (4.33 KB, patch)
2013-01-28 15:31 PST, Vladan Djeric (:vladan)
ehsan: feedback+
Details | Diff | Review
Initialize the hidden window lazily, v2 (6.80 KB, patch)
2013-02-19 21:17 PST, Vladan Djeric (:vladan)
bzbarsky: review+
Details | Diff | Review
Remove unused GetHiddenWindowAndJSContext function, v2 (5.18 KB, patch)
2013-02-19 21:18 PST, Vladan Djeric (:vladan)
bzbarsky: review+
Details | Diff | Review

Description :Ehsan Akhgari (busy, don't ask for review please) 2013-01-08 12:36:15 PST
On Mac we actually need the hidden window but I don't think that we do need that on other platforms, so I guess I can port the idea in the patch to bug 827188 to the regular hidden window as well and win some startup time.
Comment 1 Vladan Djeric (:vladan) 2013-01-28 15:30:03 PST
Created attachment 707324 [details] [diff] [review]
Lazy initialization WIP1
Comment 2 Vladan Djeric (:vladan) 2013-01-28 15:31:00 PST
Created attachment 707325 [details] [diff] [review]
Remove unused GetHiddenWindowAndJSContext function
Comment 3 Vladan Djeric (:vladan) 2013-01-29 09:06:03 PST
The talos ts-paint results don't look too encouraging. 

Before: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6d633b6a8d43
After: https://tbpl.mozilla.org/?tree=Try&rev=e9c42fb5fcbb

Compare-talos: http://perf.snarkfest.net/compare-talos/index.html?oldRevs=6d633b6a8d43&newRev=e9c42fb5fcbb&tests=ts,ts_paint,ts_cold,ts_cold_shutdown,twinopen,tpaint&submit=true

There looks to be a 12% win on Android and ~1% improvements/regressions on tspaint and tpaint (likely noise).
Comment 4 :Ehsan Akhgari (busy, don't ask for review please) 2013-01-29 09:52:32 PST
I wouldn't be entirely surprised if this provides a similar win for b2g.  b2g isn't (shouldn't!) be using the hidden window at all.
Comment 5 Justin Lebar (not reading bugmail) 2013-01-29 10:11:52 PST
> There looks to be a 12% win on Android and ~1% improvements/regressions on
> tspaint and tpaint (likely noise).

FWIW the 12% win on Android is also well within the range of noise.  Perhaps it averages out to a win.  But note also that a build on try isn't the same as a build from m-i.

Anyway, this seems virtuous even if we can't point to a win on benchmarks.
Comment 6 :Ehsan Akhgari (busy, don't ask for review please) 2013-01-30 10:59:00 PST
Comment on attachment 707324 [details] [diff] [review]
Lazy initialization WIP1

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

::: toolkit/xre/nsAppRunner.cpp
@@ +3759,1 @@
>      rv = appStartup->CreateHiddenWindow();

Please add a comment explaining why this code is Mac-only.

::: xpfe/appshell/public/nsIAppShellService.idl
@@ +121,5 @@
>    /**
> +   * Whether the hidden non-private window has been lazily created.
> +   */
> +  [noscript]
> +  readonly attribute boolean hasHiddenWindow;

rev the iid please.

::: xpfe/appshell/src/nsAppShellService.cpp
@@ +89,5 @@
>  void
> +nsAppShellService::EnsureHiddenWindow()
> +{
> +  if (!mHiddenWindow) {
> +    CreateHiddenWindowHelper(false);

Nit: just call CreateHiddenWindow.
Comment 7 :Ehsan Akhgari (busy, don't ask for review please) 2013-01-30 10:59:33 PST
Comment on attachment 707325 [details] [diff] [review]
Remove unused GetHiddenWindowAndJSContext function

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

::: xpfe/appshell/public/nsIAppShellService.idl
@@ -90,5 @@
> -   * @aJSContext       the corresponding JavaScript context
> -   */
> -  [noscript]
> -  void getHiddenWindowAndJSContext(out nsIDOMWindow aHiddenDOMWindow,
> -                                   out JSContext aJSContext);

rev the iid please.
Comment 8 Vladan Djeric (:vladan) 2013-02-01 19:30:23 PST
Talos results from 5 Talos runs don't show much of a change in Ts except on Android where there is a 16.8% improvement:

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=8f0037912f0a&newRev=e9c42fb5fcbb&tests=ts,ts_paint,tpaint&submit=true
Comment 9 :Ehsan Akhgari (busy, don't ask for review please) 2013-02-02 11:14:56 PST
We should take this patch regardless.
Comment 10 Vladan Djeric (:vladan) 2013-02-19 21:17:17 PST
Created attachment 715875 [details] [diff] [review]
Initialize the hidden window lazily, v2
Comment 11 Vladan Djeric (:vladan) 2013-02-19 21:18:23 PST
Created attachment 715876 [details] [diff] [review]
Remove unused GetHiddenWindowAndJSContext function, v2
Comment 12 Vladan Djeric (:vladan) 2013-02-19 21:22:45 PST
https://tbpl.mozilla.org/?tree=Try&rev=0c156144f497
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-02-20 05:47:00 PST
Comment on attachment 715875 [details] [diff] [review]
Initialize the hidden window lazily, v2

r=me
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-02-20 05:47:32 PST
Comment on attachment 715876 [details] [diff] [review]
Remove unused GetHiddenWindowAndJSContext function, v2

r=me if you're pretty sure binary extensions don't use this.
Comment 16 Marco Bonardo [::mak] 2013-02-20 15:59:28 PST
This has been backed out for various mochitests failures in mochitest-other and mochitest-browser (and maybe mochitest-5, not confirmed yet) plus Marionette.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a77f4a652627

some of the error logs
https://tbpl.mozilla.org/php/getParsedLog.php?id=19927363&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=19926708&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=19928424&tree=Mozilla-Inbound

It's probably safer to get a full green Try before next relanding, considered the number of failures.
Comment 17 Phil Ringnalda (:philor) 2013-02-20 17:19:46 PST
Plus 7200 second timeouts in Mac reftest and jsreftest.
Comment 18 Marco Bonardo [::mak] 2013-02-21 02:03:22 PST
tree-management for whatever reason is also blaming this push for a huge trace malloc increase and looks like after the backout it went down to normal
http://mzl.la/YBzcn6
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2016-05-27 10:15:43 PDT
I spun part 2 out into bug 1276304 so it can actually land.

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