Creating the hidden window lazily on non-Mac platforms
Categories
(Core :: General, defect)
Tracking
()
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)
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 |
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.
Updated•12 years ago
|
Updated•12 years ago
|
Comment 1•12 years ago
|
||
Comment 2•12 years ago
|
||
Comment 3•12 years ago
|
||
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•12 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.
Comment 5•12 years ago
|
||
> 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•12 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.
Reporter | ||
Comment 7•12 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.
Comment 8•12 years ago
|
||
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•12 years ago
|
||
We should take this patch regardless.
Comment 10•12 years ago
|
||
Comment 11•12 years ago
|
||
Comment 12•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=0c156144f497
![]() |
||
Comment 13•12 years ago
|
||
Comment on attachment 715875 [details] [diff] [review] Initialize the hidden window lazily, v2 r=me
![]() |
||
Comment 14•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bee7d808f97 https://hg.mozilla.org/integration/mozilla-inbound/rev/bb4938013af5
Comment 16•12 years ago
|
||
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•12 years ago
|
||
Plus 7200 second timeouts in Mac reftest and jsreftest.
Comment 18•12 years ago
|
||
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
Updated•12 years ago
|
Updated•8 years ago
|
![]() |
||
Comment 19•8 years ago
|
||
I spun part 2 out into bug 1276304 so it can actually land.
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 20•6 years ago
|
||
This might be interesting for start-up performance work that the front-end team is working on this quarter. Putting back into triage.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 21•6 years ago
|
||
I'll update this patch and send to try (or try to reland it) to see what the impact is
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 22•6 years ago
|
||
So this is showing some good potential of ~1.5% to ~2.7% wins for ts_paint on Windows and Linux.
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 | ||
Comment 23•5 years ago
|
||
Assignee | ||
Comment 24•5 years ago
|
||
Assignee | ||
Comment 25•5 years ago
|
||
Assignee | ||
Comment 26•5 years ago
|
||
Assignee | ||
Comment 27•5 years ago
|
||
Assignee | ||
Comment 28•5 years ago
|
||
Assignee | ||
Comment 29•5 years ago
|
||
Assignee | ||
Comment 30•5 years ago
|
||
Assignee | ||
Comment 31•5 years ago
|
||
Assignee | ||
Comment 32•5 years ago
|
||
Assignee | ||
Comment 33•5 years 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 34•5 years ago
|
||
Assignee | ||
Comment 35•5 years ago
|
||
Assignee | ||
Comment 36•5 years ago
|
||
Assignee | ||
Comment 37•5 years ago
|
||
Assignee | ||
Comment 38•5 years ago
|
||
Assignee | ||
Comment 39•5 years 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.
Updated•5 years ago
|
Assignee | ||
Comment 40•5 years 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%)
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 41•5 years ago
|
||
Rebased. Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=94715269fd35125ea67d0821a92aa452a191c04e
Comment 42•5 years 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
Comment 43•5 years ago
|
||
bugherder |
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•5 years ago
|
||
This was backed out from central for causing bug 1532054. Resetting the flags.
https://hg.mozilla.org/mozilla-central/rev/bf3cbcc825276823afba2778333cf62cfa75039c
Updated•5 years ago
|
Assignee | ||
Comment 45•5 years 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)
![]() |
||
Comment 46•5 years ago
|
||
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•5 years 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
Comment 48•5 years ago
|
||
bugherder |
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•5 years ago
|
||
Comment 50•5 years ago
|
||
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
Updated•2 years ago
|
Description
•