We have detected a build metrics regression from push: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=51fe9a44a5d3f1448cd7a2e50077e80ef919a3f3 As author of one of the patches included in that push, we need your help to address this regression. Regressions: 0% installer size windows2012-64 pgo 61,607,204.58 -> 61,774,616.75 You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=11731 On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the jobs in a pushlog format. To learn more about the regressing test(s), please see: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Automated_Performance_Testing_and_Sheriffing/Build_Metrics

:jorendorff Looks like some of the changesets from your patch caused an 150KBytes increase in installer size, on Windows x64. Please investigate this for a binary reduction.

Also, please mention which of the following bugs is more related to this issue: bug 1439063 bug 1440043 bug 1439936

Hm, that's surprising. I wouldn't expect any of those patches to affect the generated code at all. Ionut, can you confirm/bisect with backout try pushes?

Yes, I already pushed to Try. Will come back with the results on that.

I bisected the three bugs I mentioned in comment 2. I was able to reproduce the installer size regression. Turns out bug 1439063 is the culprit, with 130KBytes increase on Windows 64bit and an 60KBytes increase on Windows 32bit. This is the comparison view for that: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1c9e9f563485&newProject=try&newRevision=108866393781&framework=2&filter=installer%20size For completeness, these are the other comparison views. For bug 1439936: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1c9e9f563485&newProject=try&newRevision=b87acb9b215e&framework=2&filter=installer%20size For bug 1440043: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1c9e9f563485&newProject=try&newRevision=737351999d63&framework=2&filter=installer%20size

SymbolSort.exe -in d:\after.target.crashreporter-symbols-full\xul.pdb\BCF9477A04B347718BB3929A744819B62\xul.pdb -diff d:\before.target.crashreporter-symbols-full\xul.pdb\AC7435B5102E493B804F46C45F4BC51C2\xul.pdb -out jsheaders64.txt

(In reply to David Major [:dmajor] from comment #6) > Created attachment 8954845 [details] > SymbolSort output The "Increases in Total Size" headings are likely to be the most interesting. Note that many increases are red herrings. ICF may have chosen a different name for the same code in the two builds, so you get one increase balanced by a similar decrease listed later on. I do see a lot of increases in mozilla::dom::*::ToObjectInternal functions, that well exceed the amount of decreases in similarly-named functions. I'm trying to disassemble some ToObjectInternal's to see if there's more inlining or something, but my debuger is hanging when I try to dump them.

Ionut, can you bisect the three patches within bug 1439063? My theory is that it's the middle patch, and that it's due to the reordering of UNIFIED_SOURCES in [1]. So please also do a try push with that patch but with the reordering reverted. This will mean that the sources aren't alphabetical anymore, so you may also need to disable the appropriate lint to make things compile. [1] https://hg.mozilla.org/mozilla-central/rev/0ceb91c42b0f#l89.67

(In reply to Bobby Holley (:bholley) from comment #8) > Ionut, can you bisect the three patches within bug 1439063? > > My theory is that it's the middle patch, and that it's due to the reordering > of UNIFIED_SOURCES in [1]. So please also do a try push with that patch but > with the reordering reverted. This will mean that the sources aren't > alphabetical anymore, so you may also need to disable the appropriate lint > to make things compile. > > [1] https://hg.mozilla.org/mozilla-central/rev/0ceb91c42b0f#l89.67 Can you please hint me the tweak I need to make to the linting, to enable the compilation? I first need to fix my local environment, as it is not currently able to do any kind of compilation.

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #9) > (In reply to Bobby Holley (:bholley) from comment #8) > > Ionut, can you bisect the three patches within bug 1439063? > > > > My theory is that it's the middle patch, and that it's due to the reordering > > of UNIFIED_SOURCES in [1]. So please also do a try push with that patch but > > with the reordering reverted. This will mean that the sources aren't > > alphabetical anymore, so you may also need to disable the appropriate lint > > to make things compile. > > > > [1] https://hg.mozilla.org/mozilla-central/rev/0ceb91c42b0f#l89.67 > > Can you please hint me the tweak I need to make to the linting, to enable > the compilation? I just tried reordering a file and building, and got this: > UnsortedError: An attempt was made to add an unsorted sequence to a list Searching for UnsortedError on searchfox, I see: https://searchfox.org/mozilla-central/rev/769222fadff46164f8cc0dc7a0bae5a60dc2f335/python/mozbuild/mozbuild/util.py#448 So you probably want to comment out those two lines. It looks like there are also some build system unit tests that check for UnsortedError: python/mozbuild/mozbuild/test/frontend/test_namespaces.py python/mozbuild/mozbuild/test/test_util.py Depending on whether those run in the build job or as a separate job, and depending on whether failures are fatal for the build, you may or may not need to disable them. > I first need to fix my local environment, as it is not currently able to do > any kind of compilation. I think it would be very worthwhile for you to be able to do local builds. :-)

You can add to UNIFIED_SOURCES multiple times, too, so given something like: UNIFIED_SOURCES += [ ... 'FileToPutSomeplaceElseInTheOrder.cpp', 'OtherFileToPutSomeplaceElseInTheOrder.cpp', ... ] you could split things up: UNIFIED_SOURCES += [ ... # and other files you want to come before... ] UNIFIED_SOURCES += [ 'FileToPutSomeplaceElseInTheOrder.cpp', ] UNIFIED_SOURCES += [ ... # and again, other files to come before... ] UNIFIED_SOURCES += [ 'OtherFileToPutSomeplaceElseInTheOrder.cpp', ] UNIFIED_SOURCES += [ ...any remaining files... ] Which may or may not be easier than chasing down all the places in the build system that you have to modify.

That is, when you add to UNIFIED_SOURCES: UNIFIED_SOURCES += [...] the list that you're adding needs to be alphabetically ordered. But with multiple additions: UNIFIED_SOURCES += [...] UNIFIED_SOURCES += [...] The second list does not have be alphabetically sorted as coming entirely after the first list; the second list is considered entirely on its own, without reference to lists that were added before or after it.

I did a bisect, and it turns out the middle of the patch is indeed the problem. These are the comparison views for the 1st part [1], the middle [2] and the 3rd part of the patch [3]. Unfortunately, I haven't been able to see whether reversing the order in UNIFIED_SOURCES would cancel the regression from the middle patch. [1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1167ccd18dd4&newProject=try&newRevision=d9bfe634c9c8&framework=2&filter=installer%20size [2] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1167ccd18dd4&newProject=try&newRevision=be594145070e&framework=2&filter=installer%20size [3] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1167ccd18dd4&newProject=try&newRevision=b51593501e95&framework=2&filter=installer%20size

I'm curious to know if increasing FILES_PER_UNIFIED_FILE would help here, but (no surprise) there's build bustage: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3137d237fdc4fdd693db7f2dc734844e896a702&selectedJob=166490974 I don't really want to spend the cycles to push on it further myself.

Per IRC discussion with jorendorff, this looks pretty inactionable.