1578254 - 10.81% build times (windows2012-aarch64) regression on push c8660505bc7e65f20c5959fb4e940df17a1c3d9e (Tue August 27 2019)

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Marian Raiciof [:marauder]]

We have detected a build metrics regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=c8660505bc7e65f20c5959fb4e940df17a1c3d9e

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

11% build times windows2012-aarch64 opt aarch64-no-eme nightly taskcluster-c4.4xlarge 3,664.54 -> 4,060.69

You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=22857

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

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Comment 1

[Michal Novotny [:michal]]

Bug 1567616 adds a code that builds only on Linux. On windows it just changes prefs to static prefs.

Comment 2

[Marian Raiciof [:marauder]]

Thanks for the info Michal.

Chris, Mike - in this alert pushlog :
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2bebda12a12a64915ad26887bdf94c503bc6103d&tochange=c8660505bc7e65f20c5959fb4e940df17a1c3d9e
there are some patches related to Firefox Build System that you created,
do you think that this regression might be related to those changes?
Thanks!

Comment 3

[Mike Hommey [:glandium]]

The graph points at bug 1575760, but that makes no sense. I mean, it's not supposed to have changed anything that would impact build times. Also, why only these builds?

Comment 4

[Chris Manchester (limited bugmail, email directly)]

I'm pretty sure my patch didn't impact this build. But this is a significant increase and it's hard to tell where it's coming from.

Comment 5

[Kershaw Chang [:kershaw]]

Add [necko-triaged] for removing this from out bug list.

Comment 7

[Mike Hommey [:glandium]]

Attachment: Bug 1578254 - Trick Make into running rust targets earlier again.

One of the things bug 1567616 did was to change how .cargo/config is
preprocessed, from it happening during configure to it happening during
the build. And to make things happen properly, dependencies were added
on the rust targets to ensure the .cargo/config file is created before
they run.

Unfortunately, that changed the order in which Make would run all the
targets while recursing for the compile tier, when the file doesn't
exist first. So instead of starting the compile tier with rust targets,
it would start with most C++, then do rust... which we know to make
builds slower overall because of the need to wait for those rust builds
to finish which C++ has all been dealt with already, and lacking
parallelism during the rust build.

So instead of using the ideal dependencies, we force .cargo/config to be
generated during export (which it is not already because OBJDIR_PP_FILES
are currently dealt with during misc).

As the rust_targets variable is not used anymore, we remove its
creation from recursivemake.py.

And while here, we extend the if block in recurse.mk that excludes all
the top-level recursion dependencies when running from subdirectories.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4f0f505702b
Trick Make into running rust targets earlier again. r=nalexander

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a4f0f505702b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Seems like this would be good to get uplifted to Beta at some point.

Comment 11

[Marian Raiciof [:marauder]]

Thank you Mike!
After your patch landed, perfherder created a new alert for improvements :

== Change summary for alert #22985 (as of Sat, 07 Sep 2019 19:09:27 GMT) ==

Improvements:

12% build times windows2012-aarch64 opt aarch64-no-eme nightly taskcluster-c4.4xlarge 4,183.02 -> 3,701.56

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=22985

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 9091003 [details]
Bug 1578254 - Trick Make into running rust targets earlier again.

Beta/Release Uplift Approval Request

• User impact if declined: Improve build times
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Minor build system fixup for a regression from bug 1575760
• String changes made/needed:

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9091003 [details]
Bug 1578254 - Trick Make into running rust targets earlier again.

Fixes a regression causing builds to run slower. Approved for 70.0b8.

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9e76d7234a07