Closed Bug 1339673 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox Build System :: General, defect)

53 Branch
defect
Not set
normal

Tracking

(firefox52 unaffected, firefox53 fixed, firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- unaffected
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: wlach, Assigned: chmanchester)

References

Details

(Keywords: regression)

Attachments

(3 files)

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.
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 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 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)
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.
Attachment #8838260 - Flags: review?(mh+mozilla)
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 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+
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.
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
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
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.