Creating the hidden window lazily on non-Mac platforms

RESOLVED FIXED in Firefox 67

Status

()

RESOLVED FIXED
6 years ago
a day ago

People

(Reporter: Ehsan, Assigned: Felipe)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs)

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

(Whiteboard: [fxperf:p1][Snappy:P1][qf:p3]c=startup_misc u= p=)

Attachments

(19 attachments, 2 obsolete attachments)

6.80 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.18 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
52 bytes, text/x-github-pull-request
Details | Review | Splinter Review
(Reporter)

Description

6 years ago
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
Posted patch Lazy initialization WIP1 (obsolete) — Splinter Review
Attachment #707324 - 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).
(Reporter)

Comment 4

6 years ago
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.
(Reporter)

Comment 6

6 years ago
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+
(Reporter)

Comment 7

6 years ago
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
(Reporter)

Comment 9

6 years ago
We should take this patch regardless.
Attachment #707324 - Attachment is obsolete: true
Attachment #715875 - Flags: review?(bzbarsky)
Attachment #707325 - Attachment is obsolete: true
Attachment #715876 - Flags: review?(bzbarsky)
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+
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.
Whiteboard: [Snappy:P1] c=startup_misc u= p= → [fxperf][Snappy:P1] c=startup_misc u= p=

Updated

a year ago
Whiteboard: [fxperf][Snappy:P1] c=startup_misc u= p= → [fxperf:p3][Snappy:P1][qf] c=startup_misc u= p=

Updated

a year ago
Whiteboard: [fxperf:p3][Snappy:P1][qf] c=startup_misc u= p= → [fxperf:p3][Snappy:P1][qf:f64][qf:p3] c=startup_misc u= p=
See Also: → bug 1367830

Updated

10 months ago
Whiteboard: [fxperf:p3][Snappy:P1][qf:f64][qf:p3] c=startup_misc u= p= → [fxperf:p3][Snappy:P1][qf:p3:f64]c=startup_misc u= p=

Updated

5 months ago
Whiteboard: [fxperf:p3][Snappy:P1][qf:p3:f64]c=startup_misc u= p= → [fxperf:p3][Snappy:P1][qf:p3]c=startup_misc u= p=

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

Whiteboard: [fxperf:p3][Snappy:P1][qf:p3]c=startup_misc u= p= → [fxperf][Snappy:P1][qf:p3]c=startup_misc u= p=
(Assignee)

Updated

a month ago
Whiteboard: [fxperf][Snappy:P1][qf:p3]c=startup_misc u= p= → [fxperf:p2][Snappy:P1][qf:p3]c=startup_misc u= p=
(Assignee)

Comment 21

a month ago

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

Assignee: nobody → felipc
Status: NEW → ASSIGNED
(Assignee)

Updated

a month ago
Whiteboard: [fxperf:p2][Snappy:P1][qf:p3]c=startup_misc u= p= → [fxperf:p1][Snappy:P1][qf:p3]c=startup_misc u= p=
(Assignee)

Comment 22

a month ago

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.

(Assignee)

Updated

a month ago
Blocks: 1522877
(Assignee)

Updated

a month ago
Depends on: 1527213
(Assignee)

Updated

a month ago
See Also: → bug 1527219
(Assignee)

Comment 33

23 days ago

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.

(Assignee)

Comment 39

21 days ago

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.

No longer depends on: 1527213
Summary: Create hidden windows lazily on non-Mac platforms → Support creating the hidden windows lazily on non-Mac platforms
Attachment #9046472 - Attachment description: Bug 827976 - Create the hidden window lazily on non-Mac platforms. r=aklotz → Bug 827976 - Support creating the hidden window lazily on non-Mac platforms. r=aklotz
(Assignee)

Updated

19 days ago
Blocks: 1531854
(Assignee)

Comment 40

19 days ago

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

Attachment #9046472 - Attachment description: Bug 827976 - Support creating the hidden window lazily on non-Mac platforms. r=aklotz → Bug 827976 - Create the hidden window lazily on non-Mac platforms. r=aklotz
Attachment #9046475 - Attachment description: Bug 827976 - Don't create the hidden window in nsCCUncollectableMarker.cpp if it doesn't exist. r=njn → Bug 827976 - Don't create the hidden window in nsCCUncollectableMarker.cpp if it doesn't exist. r=mccr8
Attachment #9046478 - Attachment description: Bug 827976 - Don't trigger the creation if the hidden window on non-Mac platforms from newInstall.js r=mossop → Bug 827976 - Don't trigger the creation of the hidden window on non-Mac platforms from newInstall.js. r=Gijs
(Assignee)

Comment 41

19 days ago
Summary: Support creating the hidden windows lazily on non-Mac platforms → Creating the hidden window lazily on non-Mac platforms

Comment 42

19 days ago
Pushed by fgomes@mozilla.com:
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
Depends on: 1532054

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

Status: RESOLVED → REOPENED
status-firefox67: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla67 → ---
Flags: needinfo?(felipc)
(Assignee)

Comment 45

16 days ago

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

Flags: needinfo?(felipc)

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

Depends on: 1532656

Comment 47

15 days ago
Pushed by fgomes@mozilla.com:
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

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

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