Create hidden windows lazily on non-Mac platforms

NEW
Unassigned

Status

()

Core
General
4 years ago
11 months ago

People

(Reporter: Ehsan, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P1] c=startup_misc u= p=)

Attachments

(2 attachments, 2 obsolete attachments)

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.
Assignee: nobody → ehsan
Blocks: 810156
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [Snappy:P1]
Assignee: ehsan → vdjeric
Created attachment 707324 [details] [diff] [review]
Lazy initialization WIP1
Attachment #707324 - Flags: feedback?(ehsan)
Created attachment 707325 [details] [diff] [review]
Remove unused GetHiddenWindowAndJSContext function
Attachment #707325 - Flags: feedback?(ehsan)
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).
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.
> 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 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.
Attachment #707324 - Flags: feedback?(ehsan) → feedback+
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.
Attachment #707325 - Flags: feedback?(ehsan) → feedback+
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
We should take this patch regardless.
Created attachment 715875 [details] [diff] [review]
Initialize the hidden window lazily, v2
Attachment #707324 - Attachment is obsolete: true
Attachment #715875 - Flags: review?(bzbarsky)
Created attachment 715876 [details] [diff] [review]
Remove unused GetHiddenWindowAndJSContext function, v2
Attachment #707325 - Attachment is obsolete: true
Attachment #715876 - Flags: review?(bzbarsky)
https://tbpl.mozilla.org/?tree=Try&rev=0c156144f497
Comment on attachment 715875 [details] [diff] [review]
Initialize the hidden window lazily, v2

r=me
Attachment #715875 - Flags: review?(bzbarsky) → review+
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.
Attachment #715876 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bee7d808f97
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb4938013af5
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.
Plus 7200 second timeouts in Mac reftest and jsreftest.
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
Whiteboard: [Snappy:P1] → [Snappy:P1] c=startup_misc u= p=
Assignee: vladan.bugzilla → nobody
I spun part 2 out into bug 1276304 so it can actually land.
You need to log in before you can comment on or make changes to this bug.