1913762 - 0.84 - 0.8% Base Content JS / Base Content JS + 1 more (Linux, OSX, Windows) regression on Wed August 14 2024

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Treeherder Bug Filer]

Perfherder has detected a awsy performance regression from push bd066196ee77868018fd3a8514690b0db3c72a7f. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
1%	Base Content JS	linux1804-64-shippable-qr	fission	1,512,747.33 -> 1,525,464.00
1%	Base Content JS	macosx1015-64-shippable-qr	fission	1,513,544.00 -> 1,526,264.00
1%	Base Content JS	windows11-64-2009-shippable-qr	fission	1,518,584.00 -> 1,530,774.00

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 patch(es) may be backed out in accordance with our regression policy.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 1724

For more information on performance sheriffing please see our FAQ.

Comment 1

[:Gijs (he/him)]

I'm confused because I don't expect any of the code I touched to run in the child process at all. (If anything, I was hoping this change would unlock startup savings by future patches, in terms of how many things we load on startup only to register an observer / do other meaningless initialization.)

Except: is it possible this increase is only from the increase in size of XPCOMUtils.sys.mjs code size itself? How would I find out?

Comment 2

[Andrew McCreight [:mccr8]]

The first step for investigating a base content memory regression is to download the about:memory report from before and after the change and use the "load and diff" feature to see where the memory might be. You only want to look at the content processes, of course.

This test is extremely stable so it can detect very small regressions. The regression here is about 12,000 bytes, so yeah the slightly larger code size could cause some random JS engine hash table to go to the next bucket size.

Comment 3

[:Gijs (he/him)]

OK. So something about heap unclassified, and something about more js global bits. But the quantities don't match and there's only memory reports for a different test so 🤷‍♂️.

Let's just see what happens if I put this utility method on some other junk drawer utility class.

remote: Follow the progress of your build on Treeherder:
remote: https://treeherder.mozilla.org/jobs?repo=try&revision=5eb74e927ef5931a78a04a35380b123e90407504
remote: recorded changegroup in replication log in 0.032s
push complete
temporary commit removed, repository restored

!!!NOTE!!!
You'll be able to find a performance comparison here once the tests are complete (ensure you select the right framework): https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=c727ce74e5bff421b3d84aa4e05ff02b12151a95&newProject=try&newRevision=5eb74e927ef5931a78a04a35380b123e90407504

---

•      2 commits/try-runs are created...          *
  

---

Base revision's try run: https://treeherder.mozilla.org/jobs?repo=try&revision=c727ce74e5bff421b3d84aa4e05ff02b12151a95
Local revision's try run: https://treeherder.mozilla.org/jobs?repo=try&revision=5eb74e927ef5931a78a04a35380b123e90407504

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #3)

https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=c727ce74e5bff421b3d84aa4e05ff02b12151a95&newProject=try&newRevision=5eb74e927ef5931a78a04a35380b123e90407504&page=1&framework=4&filter=Content+JS

suggests that yes, this alone is sufficient... which is wild. But OK. I guess I get to find a different home that isn't XPCOMUtils for this code, so that it doesn't get loaded in content processes.

Comment 5

[Andrew McCreight [:mccr8]]

XPCOMParentUtils?

Comment 6

[:Gijs (he/him)]

(In reply to Andrew McCreight [:mccr8] from comment #5)

XPCOMParentUtils?

I don't really want to create a new module just for this, as that also has overhead... plus it feels weird to have a module for just one method. Ironically, I was in the process of documenting the new method by including docs for XPCOMUtils on source-docs and one of my notes was that "XPCOMUtils" is kind of a misnomer for the "meat" of the module as it is...

I'm probably going to add it to https://searchfox.org/mozilla-central/source/toolkit/modules/BrowserUtils.sys.mjs . Which also doesn't feel great but will work and won't introduce new module loads in a way that makes tests complain (I hope).

Comment 7

[:Gijs (he/him)]

Attachment: Bug 1913762 - move callModulesFromCategory from XPCOMUtils to BrowserUtils to avoid bloating out content JS, r?#firefox-desktop-core-reviewers

Comment 8

[:Gijs (he/him)]

https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=c727ce74e5bff421b3d84aa4e05ff02b12151a95&newProject=try&newRevision=d37488a86994efc7da1c2c8c33cb001eac8cf987&page=1&framework=4&filter=content+js says this reduces things back down so I'll land...

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71e765c86dad
move callModulesFromCategory from XPCOMUtils to BrowserUtils to avoid bloating out content JS, r=firefox-desktop-core-reviewers ,dao

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/71e765c86dad

Comment 11

[Mayank Bansal]

Seems to have improved the numbers back to pre-regression range.

Comment 12

[Acasandrei Beatrice (needinfo me)]

(In reply to Pulsebot from comment #9)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71e765c86dad
move callModulesFromCategory from XPCOMUtils to BrowserUtils to avoid
bloating out content JS, r=firefox-desktop-core-reviewers ,dao

Perfherder has detected a awsy performance change from push 71e765c86dad.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
1%	Base Content JS	macosx1015-64-shippable-qr	fission	1,526,264.00 -> 1,513,544.00
1%	Base Content JS	windows11-64-2009-shippable-qr	fission	1,531,368.00 -> 1,518,584.00
1%	Base Content JS	linux1804-64-shippable-qr	fission	1,525,266.00 -> 1,512,744.00

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 1753

For more information on performance sheriffing please see our FAQ.