Set marionette.actors.enabled to true by default for all builds
Categories
(Remote Protocol :: Marionette, task, P1)
Tracking
(Fission Milestone:M7, firefox84 fixed)
Tracking | Status | |
---|---|---|
firefox84 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
(Blocks 3 open bugs)
Details
(Keywords: memory-footprint, Whiteboard: [marionette-fission-mvp][simple])
Attachments
(3 files, 1 obsolete file)
Once Fission builds work fine with the marionette.actors.enabled
preference set to true
, we will have to enable the preference for all the builds, means non-Fission too.
Comment 1•4 years ago
|
||
Tracking marionette-fission-mvp bugs for Fission Beta milestone (M7)
Assignee | ||
Comment 2•4 years ago
|
||
FYI actors for Fission jobs will be enabled tomorrow via bug 1660168. I will check soon what needs to be done to enable it for all builds.
Assignee | ||
Comment 3•4 years ago
|
||
Here a preliminary try push with actors enabled for non-Fission jobs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b5f6e645275bb7cc1cd6c369621c1678ac43d2f
Assignee | ||
Comment 4•4 years ago
|
||
We seem to hit a performance regression for non-Fission builds at least when both the actor and framescript implementations are active.
That means that we should only register the framescript when actors aren't in use. There is bug 1669174.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 5•4 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [βοΈUTC+2] from comment #0)
Once Fission builds work fine with the
marionette.actors.enabled
preference set totrue
, we will have to enable the preference for all the builds, means non-Fission too.
By doing so we still have to keep test coverage for the framescript implementation. As such a new job kind would have to be added for non-Fission builds only. For Fission we should only support actors.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 6•4 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [βοΈUTC+2] from comment #4)
We seem to hit a performance regression for non-Fission builds at least when both the actor and framescript implementations are active.
That means that we should only register the framescript when actors aren't in use. There is bug 1669174.
Looks like the performance regression was a red herring, and that work is not really necessary to get actors enabled for all builds. Maybe we should at least move the page navigation events over to the actors (bug 1673823) to make navigation a bit more stable.
Assignee | ||
Comment 7•4 years ago
|
||
Given that bug 1675320 is actually a low intermittent failure for Fission builds we decided to not let it block enabling actors for all builds. As such I will upload the patches soon that will turn actors on by default, and also adds a new Marionette job for TaskCluster that runs the framescript code path on opt builds only.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 8•4 years ago
|
||
Assignee | ||
Comment 11•4 years ago
|
||
Comment 12•4 years ago
|
||
Comment on attachment 9186839 [details]
Bug 1669169 - [marionette] Hardening registration and unregistration of MarionetteEvents actor.
Revision D96525 was moved to bug 1676283. Setting attachment 9186839 [details] to obsolete.
Assignee | ||
Updated•4 years ago
|
Comment 13•4 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1363594da38e [marionette] Enable usage of JSWindowActor by default when Fission is disabled. r=marionette-reviewers,jdescottes https://hg.mozilla.org/integration/autoland/rev/4229097b45c7 [marionette] Add --disable-actors command line argument to force usage of deprecated framescripts instead of actors. r=marionette-reviewers,jdescottes,maja_zf https://hg.mozilla.org/integration/autoland/rev/bbf90b97018a [marionette] Add MnFr TaskCluster job to run Marionette tests with actors disabled. r=marionette-reviewers,jdescottes,jmaher,maja_zf
Comment 14•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1363594da38e
https://hg.mozilla.org/mozilla-central/rev/4229097b45c7
https://hg.mozilla.org/mozilla-central/rev/bbf90b97018a
Assignee | ||
Comment 15•4 years ago
|
||
As per bug 1677475 this change attributes to huge performance improvements:
Improvements:
Ratio Suite Test Platform Options Absolute values (old vs new) 22% Images macosx1014-64-shippable-qr 6,808,488.80 -> 5,324,379.10 21% Images windows10-64-shippable 7,281,816.21 -> 5,743,408.09 20% Images linux1804-64-shippable 5,531,400.10 -> 4,416,162.52 20% Images linux1804-64-shippable-qr 5,723,944.14 -> 4,580,189.36 19% Images windows10-64-shippable-qr 7,683,332.16 -> 6,187,840.94 19% Images windows7-32-shippable 6,165,551.84 -> 5,005,238.78 14% Explicit Memory windows10-64-shippable 328,308,125.66 -> 282,289,160.78 14% Explicit Memory windows7-32-shippable 270,060,971.52 -> 232,226,621.77 14% Resident Memory linux1804-64-shippable-qr 1,147,429,973.58 -> 988,011,424.95 14% Explicit Memory linux1804-64-shippable 358,584,795.96 -> 309,388,253.07 14% Explicit Memory windows10-64-shippable-qr 347,818,343.53 -> 300,111,776.46 13% Explicit Memory linux1804-64-shippable-qr 599,887,245.23 -> 520,569,325.12 12% Explicit Memory macosx1014-64-shippable-qr 398,158,096.69 -> 348,672,169.15 12% Resident Memory windows10-64-shippable-qr 547,322,517.50 -> 479,487,633.70 11% Resident Memory windows10-64-shippable 507,236,742.83 -> 449,415,892.09 11% Heap Unclassified windows7-32-shippable 35,549,067.62 -> 31,636,763.74 11% Heap Unclassified windows10-64-shippable-qr 56,739,704.72 -> 50,561,922.16 10% Heap Unclassified linux1804-64-shippable-qr 288,328,260.46 -> 258,227,806.70 10% Resident Memory windows10-64-shippable-qr 538,771,884.24 -> 483,816,172.30 10% Heap Unclassified windows10-64-shippable 44,664,979.54 -> 40,177,199.78 10% JS linux1804-64-shippable-qr 88,494,494.90 -> 79,660,064.82 10% JS linux1804-64-shippable 88,521,389.48 -> 79,873,908.01 10% Resident Memory linux1804-64-shippable 627,056,782.27 -> 565,954,584.79 9% Resident Memory macosx1014-64-shippable-qr 787,653,100.93 -> 714,441,972.31 9% JS windows10-64-shippable-qr 89,035,157.31 -> 80,776,586.75 9% JS windows10-64-shippable 88,619,251.19 -> 80,547,273.17 9% Resident Memory windows7-32-shippable 528,511,962.94 -> 480,395,089.35 9% JS macosx1014-64-shippable-qr 89,875,671.52 -> 82,001,873.13 8% Heap Unclassified linux1804-64-shippable 69,651,112.99 -> 64,218,327.82 7% JS windows7-32-shippable 66,986,737.48 -> 62,417,510.05 7% Images linux1804-64-shippable-qr tp6 8,421,448.20 -> 7,860,156.90 6% Heap Unclassified macosx1014-64-shippable-qr 109,628,575.20 -> 103,274,877.49 6% Images linux1804-64-shippable tp6 8,516,567.48 -> 8,031,870.89 6% Images windows7-32-shippable tp6 7,873,318.17 -> 7,429,401.20 5% Images macosx1014-64-shippable-qr tp6 9,296,358.33 -> 8,810,724.50 Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the offending patch(es) will be backed out in accordance with our regression policy.
For more information on performance sheriffing please see our FAQ.
Comment 16•4 years ago
|
||
This is awesome, it's always nice to reduce the overhead of our testing infrastructure, and get a more accurate read on real-world performance.
We should definitely make sure to verify that these improvements are real though, when changing a test framework, there's always the possibility of accidentally breaking our measurements, which could also lead to this kind of improvement.
Assignee | ||
Comment 17•4 years ago
|
||
So after some investigation I would say that we should trust those values. Here is what I did:
-
I run awsy jobs locally by modifying the load time to quicker get results while it iterates through all the test pages. I did that once with actors and then with the framescript. Both modes behave exactly the same as far as I can see.
-
I then remembered that we have memory leaks with Marionette like https://github.com/mozilla/geckodriver/issues/181, and that users of geckodriver report that Firefox's memory usage grows a lot while navigating pages. And that is exactly what awsy is doing here.
-
I run both awsy tests again by creating a gecko profile for each. So here is the profile with actors, and the profile with the framescript. Note that both awsy were quick runs with a bit of over 1 minute. Usually those jobs run for about 1 hour.
When you compare both you can already see that the profile for the framescript case uses about twice that much as for the actor case. If awsy runs way longer and does a lot more navigations, those numbers should become even worse.
Assignee | ||
Updated•4 years ago
|
Comment 18•4 years ago
|
||
I'm not sure if this bug or bug 1676283 cause the improvement but I presume that you're happy that regression bug 1677475 was partially inaccurate and the items left are all imrpovements. Can't guarantee that some other alert items that will appear won't be regressions, but that's it for the moment.
== Change summary for alert #27636 (as of Fri, 13 Nov 2020 12:48:23 GMT) ==
Improvements:
Ratio | Suite | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|---|
10% | Resident Memory | windows10-64-shippable-qr | 538,771,884.24 -> 483,816,172.30 | ||
6% | Images | linux1804-64-shippable | tp6 | 8,516,567.48 -> 8,031,870.89 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=27636
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 19•4 years ago
|
||
Note that most of the memory improvements as mentioned in comment 15 are crap. Those only existed because we disabled bfcache by accident. With my patch on bug 1682757 we actually enabled bfache again by accident. See bug 1683841 for the investigation.
Updated•2 years ago
|
Description
•