Closed Bug 827976 Opened 12 years ago Closed 6 years ago

Creating the hidden window lazily on non-Mac platforms

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Project Flags:
Performance Impact low
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: Felipe)

References

(Blocks 2 open bugs)

Details

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

Attachments

(19 files, 2 obsolete files)

Initialize the hidden window lazily, v2
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
Remove unused GetHiddenWindowAndJSContext function, v2
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
Bug 827976 - Create the hidden window lazily on non-Mac platforms. r=aklotz
(deleted), text/x-phabricator-request
Details
Bug 827976 - Create the hidden window after first paint, but before session restore. r=mconley
(deleted), text/x-phabricator-request
Details
Bug 827976 - Tablet Mode detection should favor using an existing window instead of the hidden window. r=aklotz
(deleted), text/x-phabricator-request
Details
Bug 827976 - Don't create the hidden window in nsCCUncollectableMarker.cpp if it doesn't exist. r=mccr8
(deleted), text/x-phabricator-request
Details
Bug 827976 - Improve usage of the hidden window for nsXULWindow::Destroy. r=aklotz
(deleted), text/x-phabricator-request
Details
Bug 827976 - Don't trigger the creation of the hidden window on non-Mac platforms from newInstall.js. r=Gijs
(deleted), text/x-phabricator-request
Details
Bug 827976 - Use a different method to detect if xpcshell tests are running. r=ochameau
(deleted), text/x-phabricator-request
Details
Bug 827976 - Downloads Manager doesn't need a reference to the hidden window. r=mak
(deleted), text/x-phabricator-request
Details
Bug 827976 - browser-places.js doesn't need to get a reference to the hidden window. r=mak
(deleted), text/x-phabricator-request
Details
Bug 827976 - Try to use an existing window first to open preferences. r=Gijs
(deleted), text/x-phabricator-request
Details
Bug 827976 - Make Sanitizer.jsm not use the hidden window. r=Gijs
(deleted), text/x-phabricator-request
Details
Bug 827976 - Add requestIdleCallback support to Timer.jsm. r=mconley
(deleted), text/x-phabricator-request
Details
Bug 827976 - Use requestIdleCallback from Timer.jsm instead of from the hidden window. r=mconley
(deleted), text/x-phabricator-request
Details
Bug 827976 - Use an existing window for media telemetry. r=Gijs
(deleted), text/x-phabricator-request
Details
Bug 827976 - Test that the hidden window is not loaded before first paint. r=florian
(deleted), text/x-phabricator-request
Details
Bug 827976 - Teach leaks.py to ignore the hidden window. r=Ehsan
(deleted), text/x-phabricator-request
Details
Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4830
(deleted), text/x-github-pull-request
Details
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: start-faster
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [Snappy:P1]
Assignee: ehsan → vdjeric
Attached patch Lazy initialization WIP1 (obsolete) (deleted) — — Splinter Review
Attachment #707324 - Flags: feedback?(ehsan)
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.
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=
Whiteboard: [fxperf][Snappy:P1] c=startup_misc u= p= → [fxperf:p3][Snappy:P1][qf] c=startup_misc u= p=
Whiteboard: [fxperf:p3][Snappy:P1][qf] c=startup_misc u= p= → [fxperf:p3][Snappy:P1][qf:f64][qf:p3] c=startup_misc u= p=
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=
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=
Whiteboard: [fxperf][Snappy:P1][qf:p3]c=startup_misc u= p= → [fxperf:p2][Snappy:P1][qf:p3]c=startup_misc u= p=

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
Whiteboard: [fxperf:p2][Snappy:P1][qf:p3]c=startup_misc u= p= → [fxperf:p1][Snappy:P1][qf:p3]c=startup_misc u= p=

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.

Blocks: 1522877
Depends on: 1527213

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.

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
Blocks: 1531854

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

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

Summary: Support creating the hidden windows lazily on non-Mac platforms → Creating the hidden window lazily on non-Mac platforms
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)

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
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
Depends on: 1533405

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

No longer depends on: 1533405
Performance Impact: --- → P3
Whiteboard: [fxperf:p1][Snappy:P1][qf:p3]c=startup_misc u= p= → [fxperf:p1][Snappy:P1]c=startup_misc u= p=
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: