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)
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)
Comment 1•7 years ago
|
||
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.
Reporter | ||
Comment 2•7 years ago
|
||
(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
Comment 3•7 years ago
|
||
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•7 years 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)
Comment 5•7 years ago
|
||
We should also add a way to disable building the gtest libxul at all, for the valgrind builds.
Comment hidden (mozreview-request) |
Comment 7•7 years 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•7 years 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•7 years 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`.
Comment 13•7 years ago
|
||
(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•7 years ago
|
Attachment #8838260 -
Flags: review?(mh+mozilla)
Comment hidden (mozreview-request) |
Comment 16•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Updated•7 years ago
|
status-firefox52:
--- → unaffected
status-firefox53:
--- → affected
Updated•7 years ago
|
Component: Untriaged → Build Config
Product: Firefox → Core
Target Milestone: Firefox 54 → mozilla54
Version: unspecified → 53 Branch
Comment 24•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/a2dd787448dc https://hg.mozilla.org/releases/mozilla-aurora/rev/f16426d0e0e7 https://hg.mozilla.org/releases/mozilla-aurora/rev/d9d91d6e389e
Reporter | ||
Comment 25•7 years ago
|
||
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•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•