827976 - Creating the hidden window lazily on non-Mac platforms

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[(no longer active)]

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

Attachment: Lazy initialization WIP1

Comment 2

[Vladan Djeric (:vladan)]

Attachment: Remove unused GetHiddenWindowAndJSContext function

Comment 3

[Vladan Djeric (:vladan)]

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

[(no longer active)]

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

> 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

[(no longer active)]

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

[(no longer active)]

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

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

[(no longer active)]

We should take this patch regardless.

Comment 10

[Vladan Djeric (:vladan)]

Attachment: Initialize the hidden window lazily, v2

Comment 11

[Vladan Djeric (:vladan)]

Attachment: Remove unused GetHiddenWindowAndJSContext function, v2

Comment 12

[Vladan Djeric (:vladan)]

https://tbpl.mozilla.org/?tree=Try&rev=0c156144f497

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 715875 [details] [diff] [review]
Initialize the hidden window lazily, v2

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

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 15

[Vladan Djeric (:vladan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bee7d808f97
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb4938013af5

Comment 16

[Marco Bonardo [:mak]]

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

Plus 7200 second timeouts in Mac reftest and jsreftest.

Comment 18

[Marco Bonardo [:mak]]

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 [:bzbarsky]]

I spun part 2 out into bug 1276304 so it can actually land.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

This might be interesting for start-up performance work that the front-end team is working on this quarter. Putting back into triage.

Comment 21

[:Felipe Gomes (needinfo for replies!)]

I'll update this patch and send to try (or try to reland it) to see what the impact is

Comment 22

[:Felipe Gomes (needinfo for replies!)]

So this is showing some good potential of ~1.5% to ~2.7% wins for ts_paint on Windows and Linux.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=358448ef71c591ea59c137879cf3d0b5363b5352&framework=1&selectedTimeRange=172800

There are some big regressions in sessionstore tests because it's now paying the price of being the first to access the hidden window, but I think we can actually get rid of the hidden window usage by session restore (and several other pieces of code.. I'll be filing separate bugs for each case).

I've also seen some crashes by some core code that can't handle the window being just created if it's the first to access. I imagine the reference to the window is returned properly, but the DOM hasn't finished loading.. I'll need to investigate it.

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 827976 - Create the hidden window lazily on non-Mac platforms. r=aklotz

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 827976 - Create the hidden window after first paint, but before session restore. r=mconley

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 827976 - Tablet Mode detection should favor using an existing window instead of the hidden window. r=aklotz

Comment 26

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 827976 - Don't create the hidden window in nsCCUncollectableMarker.cpp if it doesn't exist. r=mccr8

Comment 27

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 827976 - Improve usage of the hidden window for nsXULWindow::Destroy. r=aklotz

Comment 28

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 827976 - Don't trigger the creation of the hidden window on non-Mac platforms from newInstall.js. r=Gijs

Comment 29

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 827976 - Use a different method to detect if xpcshell tests are running. r=ochameau

Comment 30

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 827976 - Downloads Manager doesn't need a reference to the hidden window. r=mak

Comment 31

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 827976 - browser-places.js doesn't need to get a reference to the hidden window. r=mak

Comment 32

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 827976 - Try to use an existing window first to open preferences. r=Gijs

Comment 33

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 827976 - Make Sanitizer.jsm not use the hidden window. r=Gijs

It doesn't need a high precision performance.now() to count minutes. In addition, if there are no windows to be closed, it's not doing anything, so it doesn't need to open a new one.

Comment 34

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 827976 - Add requestIdleCallback support to Timer.jsm. r=mconley

Comment 35

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 827976 - Use requestIdleCallback from Timer.jsm instead of from the hidden window. r=mconley

Comment 36

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 827976 - Use an existing window for media telemetry. r=Gijs

Comment 37

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 827976 - Test that the hidden window is not loaded before first paint. r=florian

Comment 38

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 827976 - Teach leaks.py to ignore the hidden window. r=Ehsan

Comment 39

[:Felipe Gomes (needinfo for replies!)]

Here's an update where things stand on this bug:

I started out this bug with the goal to make the hidden window lazy and (A) not created before first paint. Then I went for a stretch goal of (B) not using the hidden window at all on a "normal" Firefox session (i.e., excluding corner features that might be harder to port).

Obviously, there are more blockers for (B) than for (A), and the most important performance improvement that we want (speeding up startup) relies on A, so I'll move (B) to a separate bug. That way, we don't need to treat bug 1527213 (and some other things) as blockers.

There's still just one blocking issue for (A) in a strange talos sessionstore regression. In order to make it easier and faster to investigate it, I'll land this behind a pref (turned off), so it's just a matter of toggling a pref instead of recompiling. And we'll track turning the pref to true in a separate bug.

Comment 40

[:Felipe Gomes (needinfo for replies!)]

So we finished investigating the sessionstore regression. Turns out, what that test measures is a delta of time between two events: when the first SessionRestore module is loaded (SessionStartup.jsm), until the end of session restoration.

With this work, both measurements shift back (happens earlier), so there's actually an improvement in time from process creation until session restore finishes. However, the first measurements shifts back more, so the delta is increased and that gets reported as a regression.

We filed bug 1531520 to improve this talos test so that it's measuring something better. With that in mind, I'm waiting for one final try run and will land these patches with the pref toggled on.

I should note that the hidden window will still be forced to load after first paint (by the second patch in this queue), because there's remaining work to do there to remove it. This will be tracked by bug 1531854.

======

Here are the numbers, for anyone who is interested. Each measurements are from 10 profiled runs on talos.
(std dev for all of them were pretty much stable):

==== talos sessionrestore =====

Base (these patches, preffed off)

Linux: Windows:
646.6ms 662.6ms

Lazy window (these patches, preffed on)

Linux: Windows:
647.0ms 664.9ms
(+0.06%) (+0.34%)

No hidden window (patches from follow up 1531854)

Linux: Windows:
633.4ms 637.3ms
(-2.04%) (-3.81%)

==== talos sessionrestore_no_auto_restore =====

Base (these patches, preffed off)

Linux: Windows:
551.9ms 509.3ms

Lazy window (these patches, preffed on)

Linux: Windows:
535.3ms 512.7ms
(-3.259%) (+0.668%)

No hidden window (patches from follow up 1531854)

Linux: Windows:
535.8ms 485.8ms
(-2.92%) (-4.61%)

Comment 41

[:Felipe Gomes (needinfo for replies!)]

Rebased. Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=94715269fd35125ea67d0821a92aa452a191c04e

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e254496ff95b
Create the hidden window lazily on non-Mac platforms. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/96a507c818e2
Create the hidden window after first paint, but before session restore. r=mconley
https://hg.mozilla.org/integration/autoland/rev/9203871a5c6f
Tablet Mode detection should favor using an existing window instead of the hidden window. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/69d0378e0c09
Don't create the hidden window in nsCCUncollectableMarker.cpp if it doesn't exist. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/3b64368cff52
Improve usage of the hidden window for nsXULWindow::Destroy. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/eed600ceb606
Don't trigger the creation of the hidden window on non-Mac platforms from newInstall.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/9d3805d77b99
Use a different method to detect if xpcshell tests are running. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/07ef335770a8
Downloads Manager doesn't need a reference to the hidden window. r=mak
https://hg.mozilla.org/integration/autoland/rev/e253b264e7bd
browser-places.js doesn't need to get a reference to the hidden window. r=mak
https://hg.mozilla.org/integration/autoland/rev/fd94066a1d76
Try to use an existing window first to open preferences. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/845e1d0b2402
Make Sanitizer.jsm not use the hidden window. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/66cffb171024
Add requestIdleCallback support to Timer.jsm. r=mconley
https://hg.mozilla.org/integration/autoland/rev/f976c2d4cebb
Use requestIdleCallback from Timer.jsm instead of from the hidden window. r=mconley
https://hg.mozilla.org/integration/autoland/rev/443087a359f9
Use an existing window for media telemetry. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/92b45080d080
Test that the hidden window is not loaded before first paint. r=florian
https://hg.mozilla.org/integration/autoland/rev/54c0b12443ed
Teach leaks.py to ignore the hidden window. r=Ehsan

Comment 43

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/e254496ff95b
https://hg.mozilla.org/mozilla-central/rev/96a507c818e2
https://hg.mozilla.org/mozilla-central/rev/9203871a5c6f
https://hg.mozilla.org/mozilla-central/rev/69d0378e0c09
https://hg.mozilla.org/mozilla-central/rev/3b64368cff52
https://hg.mozilla.org/mozilla-central/rev/eed600ceb606
https://hg.mozilla.org/mozilla-central/rev/9d3805d77b99
https://hg.mozilla.org/mozilla-central/rev/07ef335770a8
https://hg.mozilla.org/mozilla-central/rev/e253b264e7bd
https://hg.mozilla.org/mozilla-central/rev/fd94066a1d76
https://hg.mozilla.org/mozilla-central/rev/845e1d0b2402
https://hg.mozilla.org/mozilla-central/rev/66cffb171024
https://hg.mozilla.org/mozilla-central/rev/f976c2d4cebb
https://hg.mozilla.org/mozilla-central/rev/443087a359f9
https://hg.mozilla.org/mozilla-central/rev/92b45080d080
https://hg.mozilla.org/mozilla-central/rev/54c0b12443ed

Comment 44

[Cosmin Sabou [:CosminS]]

This was backed out from central for causing bug 1532054. Resetting the flags.
https://hg.mozilla.org/mozilla-central/rev/bf3cbcc825276823afba2778333cf62cfa75039c

Comment 45

[:Felipe Gomes (needinfo for replies!)]

The cause for the content-process crash that got this backed out was because, before this patch, there was no code in the content process that triggered the creation of the hidden window (since it was only ever created from XRE_mainRun).

So when WindowsUIUtils::UpdateTabletModeState() ran in the content process, it tried to get the hidden window and just failed silently [1]. With these patches, the getter code itself would try to create it, and since it's not supposed to run in the content process at all, it crashes.

The reason this was not caught in tests was because that code is only triggered by a site running a "pointer" or "any-pointer" media query. Although it was surprising that there are apparently no tests at all testing this media query :O. And the returned value is broken for Win10 machines in Tablet Mode. (I'll file a separate bug for that)

https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/widget/windows/WindowsUIUtils.cpp#143-146

Comment 46

[Boris Zbarsky [:bzbarsky]]

Bug 1532358 tracks improving the test coverage for these pseudo-classes to include the widget codepath. We do in fact have tests for these in content processes, but they use prefs to simulate hardware and hence don't quite test what actually happens in the wild...

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/200e4a37af47
Create the hidden window lazily on non-Mac platforms. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/1f136a1cafc3
Create the hidden window after first paint, but before session restore. r=mconley
https://hg.mozilla.org/integration/autoland/rev/3c2bc3b312f3
Tablet Mode detection should favor using an existing window instead of the hidden window. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/3606e7bc5406
Don't create the hidden window in nsCCUncollectableMarker.cpp if it doesn't exist. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/3a2cf44afaa3
Improve usage of the hidden window for nsXULWindow::Destroy. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/8e41627afd1d
Don't trigger the creation of the hidden window on non-Mac platforms from newInstall.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/fd1095cf21af
Use a different method to detect if xpcshell tests are running. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/2cc8de9e4c12
Downloads Manager doesn't need a reference to the hidden window. r=mak
https://hg.mozilla.org/integration/autoland/rev/e4ff7ce96248
browser-places.js doesn't need to get a reference to the hidden window. r=mak
https://hg.mozilla.org/integration/autoland/rev/51a46315f660
Try to use an existing window first to open preferences. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/38db33426dd4
Make Sanitizer.jsm not use the hidden window. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/52c87e58dff3
Add requestIdleCallback support to Timer.jsm. r=mconley
https://hg.mozilla.org/integration/autoland/rev/1960442645ea
Use requestIdleCallback from Timer.jsm instead of from the hidden window. r=mconley
https://hg.mozilla.org/integration/autoland/rev/5f4cebc62cbd
Use an existing window for media telemetry. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/cd731fe8a50f
Test that the hidden window is not loaded before first paint. r=florian
https://hg.mozilla.org/integration/autoland/rev/6013e1f3b648
Teach leaks.py to ignore the hidden window. r=Ehsan

Comment 48

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/200e4a37af47
https://hg.mozilla.org/mozilla-central/rev/1f136a1cafc3
https://hg.mozilla.org/mozilla-central/rev/3c2bc3b312f3
https://hg.mozilla.org/mozilla-central/rev/3606e7bc5406
https://hg.mozilla.org/mozilla-central/rev/3a2cf44afaa3
https://hg.mozilla.org/mozilla-central/rev/8e41627afd1d
https://hg.mozilla.org/mozilla-central/rev/fd1095cf21af
https://hg.mozilla.org/mozilla-central/rev/2cc8de9e4c12
https://hg.mozilla.org/mozilla-central/rev/e4ff7ce96248
https://hg.mozilla.org/mozilla-central/rev/51a46315f660
https://hg.mozilla.org/mozilla-central/rev/38db33426dd4
https://hg.mozilla.org/mozilla-central/rev/52c87e58dff3
https://hg.mozilla.org/mozilla-central/rev/1960442645ea
https://hg.mozilla.org/mozilla-central/rev/5f4cebc62cbd
https://hg.mozilla.org/mozilla-central/rev/cd731fe8a50f
https://hg.mozilla.org/mozilla-central/rev/6013e1f3b648

Comment 49

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4830

Comment 50

[Ionuț Goldan [:igoldan]]

Perf improvement noticed, thanks to push from comment 47.

== Change summary for alert #19944 (as of Mon, 18 Mar 2019 06:33:07 GMT) ==

Improvements:

3% ts_paint windows10-64-pgo-qr opt e10s stylo 326.62 -> 318.15

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=19944