6.07% build times (linux64) regression on push cff9ca695cca7bf21d6c628f2007c28e778d42f1 (Tue Feb 7 2017)

RESOLVED FIXED in Firefox 53

Status

Firefox Build System
General
RESOLVED FIXED
a year ago
3 months ago

People

(Reporter: wlach, Assigned: chmanchester)

Tracking

({regression})

53 Branch
mozilla54
regression

Firefox Tracking Flags

(firefox52 unaffected, firefox53 fixed, firefox54 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

This might be one of the first build times regressions perfherder has caught. Chris, I don't know if this was expected or can be mitigated, so please feel free to mark this as wontfix.

--

We have detected a build metrics regression from push cff9ca695cca7bf21d6c628f2007c28e778d42f1. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

  6%  build times summary linux64 opt nightly taskcluster-m4.4xlarge valgrind     1465.18 -> 1554.13


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

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
Flags: needinfo?(cmanchester)
That patch moves linking the gtest libxul from the package-tests tier to the compile tier. Presumably the valgrind builds weren't ever running package-tests, so they're now doing additional work they weren't before.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> That patch moves linking the gtest libxul from the package-tests tier to the
> compile tier. Presumably the valgrind builds weren't ever running
> package-tests, so they're now doing additional work they weren't before.

The regression is noticeable on non-valgrind builds as well (needed to reassign something for that to be visible):

== Change summary for alert #5038 (as of February 07 2017 18:09 UTC) ==

Regressions:

  6%  build times summary linux64 opt nightly taskcluster-m4.4xlarge valgrind     1465.18 -> 1554.13
  6%  build times summary linux64 pgo nightly taskcluster-m4.4xlarge              2762.55 -> 2915.56

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=5038
Something is very wrong here. I looked at an arbitrary linux32 pgo taskcluster build job on autoland:
https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=77850850
https://public-artifacts.taskcluster.net/QTxlUDXAQ8uOtcqraIYMQA/0/public/logs/live_backing.log

Searching the log for '-o libxul.so' finds *6 instances*. So there's an obvious bug, and a non-obvious bug.

The obvious bug is that we shouldn't be linking the gtest libxul twice in a PGO build.

The non-obvious bug is that we're linking an extra libxul somewhere, and we're doing it twice for PGO builds.
(Assignee)

Comment 4

a year ago
Thanks for filing Will, it's a good thing perfherder caught this.

I'll post a patch to skip linking the gtest libxul during MOZ_PROFILE_GENERATE shortly.

The other issue is that we're re-linking libxul during the misc tier on PGO builds due to a bad interaction with generated files -- something in misc now depends on libxul, so we re-build it there, apparently due to http://searchfox.org/mozilla-central/rev/cac6cb6a10afb8ebb2ecfbeeedaff7c66f57dd75/config/rules.mk#605 

This was already happening for the js shell, but linking that doesn't take very long, so it wasn't an issue. Maybe we can just move the generated files target to the compile tier when they end up depending on a library or program.
Assignee: nobody → cmanchester
Flags: needinfo?(cmanchester)
We should also add a way to disable building the gtest libxul at all, for the valgrind builds.
Comment hidden (mozreview-request)

Comment 7

a year ago
mozreview-review
Comment on attachment 8838154 [details]
Bug 1339673 - Don't link the gtest libxul during MOZ_PROFILE_GENERATE.

https://reviewboard.mozilla.org/r/113140/#review114598
Attachment #8838154 - Flags: review?(ted) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

a year ago
mozreview-review
Comment on attachment 8838260 [details]
Bug 1339673 - Only force re-linking during PGO builds for the compile tier.

https://reviewboard.mozilla.org/r/113212/#review114722

::: config/rules.mk:589
(Diff revision 1)
>  endif
>  endif
>  endif
>  
>  ifneq (,$(MOZ_PROFILE_GENERATE)$(MOZ_PROFILE_USE))
> +ifneq (,$(filter recurse_compile,$(MAKECMDGOALS)))

By the time the rule is making any difference, the makecmdgoal is not "recurse_compile", but likely "target".
Attachment #8838260 - Flags: review?(mh+mozilla)
(Assignee)

Comment 12

a year ago
mozreview-review-reply
Comment on attachment 8838260 [details]
Bug 1339673 - Only force re-linking during PGO builds for the compile tier.

https://reviewboard.mozilla.org/r/113212/#review114722

> By the time the rule is making any difference, the makecmdgoal is not "recurse_compile", but likely "target".

Oh, looks like it's `compile`.
(In reply to Chris Manchester (:chmanchester) from comment #12)
> Comment on attachment 8838260 [details]
> Bug 1339673 - Only force re-linking during PGO builds for the compile tier.
> 
> https://reviewboard.mozilla.org/r/113212/#review114722
> 
> > By the time the rule is making any difference, the makecmdgoal is not "recurse_compile", but likely "target".
> 
> Oh, looks like it's `compile`.

recurse_compile depends on path/host and path/target targets, which then invoke make -C path {host,target}, so I'd expect target, not compile.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8838260 - Flags: review?(mh+mozilla)
Comment hidden (mozreview-request)

Comment 16

a year ago
mozreview-review
Comment on attachment 8838260 [details]
Bug 1339673 - Only force re-linking during PGO builds for the compile tier.

https://reviewboard.mozilla.org/r/113212/#review115488
Attachment #8838260 - Flags: review?(mh+mozilla) → review+

Comment 17

a year ago
mozreview-review
Comment on attachment 8838259 [details]
Bug 1339673 - Add an option to disable building the gtest xul and set it for valgrind builds in automation.

https://reviewboard.mozilla.org/r/113210/#review117976

::: moz.configure:160
(Diff revision 1)
>  def build_backends(backends):
>      return backends
>  
>  set_config('BUILD_BACKENDS', build_backends)
>  
> +option('--disable-gtest',

nit: might be better as `--disable-gtest-in-build` or something, but given that it only applies to automation builds maybe I'm being too picky? You could build with this locally and still run `mach gtest` and it would work, right?
Attachment #8838259 - Flags: review?(ted) → review+
(Assignee)

Comment 18

a year ago
mozreview-review-reply
Comment on attachment 8838259 [details]
Bug 1339673 - Add an option to disable building the gtest xul and set it for valgrind builds in automation.

https://reviewboard.mozilla.org/r/113210/#review117976

> nit: might be better as `--disable-gtest-in-build` or something, but given that it only applies to automation builds maybe I'm being too picky? You could build with this locally and still run `mach gtest` and it would work, right?

That's reasonable, I'll update this.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

a year ago
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/067614ea94c0
Don't link the gtest libxul during MOZ_PROFILE_GENERATE. r=ted
https://hg.mozilla.org/integration/autoland/rev/2426cf95699f
Add an option to disable building the gtest xul and set it for valgrind builds in automation. r=ted
https://hg.mozilla.org/integration/autoland/rev/092a88bec83a
Only force re-linking during PGO builds for the compile tier. r=glandium

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/067614ea94c0
https://hg.mozilla.org/mozilla-central/rev/2426cf95699f
https://hg.mozilla.org/mozilla-central/rev/092a88bec83a
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
status-firefox52: --- → unaffected
status-firefox53: --- → affected
Component: Untriaged → Build Config
Product: Firefox → Core
Target Milestone: Firefox 54 → mozilla54
Version: unspecified → 53 Branch
Improvements visible now.

== Change summary for alert #5293 (as of March 02 2017 22:53 UTC) ==

Improvements:

 18%  build times summary linux32 pgo taskcluster-c4.4xlarge              3335.73 -> 2742.93
  7%  build times summary linux64 opt taskcluster-c4.4xlarge valgrind     1401.78 -> 1298.64
  7%  build times summary linux64 pgo taskcluster-c4.4xlarge              2638.45 -> 2465.14

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

Updated

3 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.