Closed
Bug 1447116
Opened 6 years ago
Closed 6 years ago
Update builders to rust 1.28
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(firefox63 fixed)
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: chmanchester, Assigned: glandium)
References
Details
Attachments
(4 files, 1 obsolete file)
The 1.25 is scheduled for March 29, we should be ready to update the toolchains used in integration branch builds by then.
Reporter | ||
Updated•6 years ago
|
Assignee: nobody → cmanchester
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Attachment #8963825 -
Flags: review?(core-build-config-reviews) → review?(nalexander)
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8963825 [details] Bug 1447116 - Update rust builders to 1.25 https://reviewboard.mozilla.org/r/232698/#review238144 Technically, this looks fine.
Attachment #8963825 -
Flags: review?(nalexander) → review+
Pushed by cmanchester@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/37c026e3a6a5 Update rust builders to 1.25 r=nalexander
Comment 4•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/37c026e3a6a5
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 5•6 years ago
|
||
Per bug 1451703 and bug 1456150, can we please back this out from m-c before the soft freeze starts on Thursday?
Flags: needinfo?(cmanchester)
Reporter | ||
Comment 6•6 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e755d9d291fe4eabf97c5f912cc2ad98688b0dda
Flags: needinfo?(cmanchester)
Updated•6 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•6 years ago
|
status-firefox61:
fixed → ---
Target Milestone: mozilla61 → ---
Updated•6 years ago
|
Version: Version 3 → Trunk
Reporter | ||
Comment 7•6 years ago
|
||
For the reasons discussed in bug 1456150, rustc 1.28 is likely to be our next upgrade, on or around August 16th.
Summary: Update builders to rust 1.25 → Update builders to rust 1.28
Assignee | ||
Comment 8•6 years ago
|
||
We may want to jump ship early by using 1.28 when it is beta on nightly. As long as we keep the base toolchains job with the minimum version we support (which we could say is the corresponding stable, 1.27), we're good.
Reporter | ||
Comment 9•6 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #8) > We may want to jump ship early by using 1.28 when it is beta on nightly. As > long as we keep the base toolchains job with the minimum version we support > (which we could say is the corresponding stable, 1.27), we're good. That's a good idea. I'll get that going at some point after the merge.
Assignee | ||
Comment 10•6 years ago
|
||
FWIW, 1.28.0-beta.1 is beta-2018-06-22 for --channel.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Attachment #8987770 -
Flags: review?(core-build-config-reviews) → review?(nfroyd)
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8987770 [details] Bug 1447116 - Require rust 1.26. https://reviewboard.mozilla.org/r/253040/#review259662 r=me with the obvious below change. ::: commit-message-2bb75:3 (Diff revision 1) > +We're well overdue for an upgrade of the rust compiler requirements. > +Now that we're building with 1.28 (albeit a beta, due to be bumped when > +it's released), we can bump the requirement away from 1.24 which is now > +old. 1.27 is too new, though, so settle for the older 1.26. If we're going to bump the base toolchain job, presumably we also want to bump the requirement in rust.configure: https://searchfox.org/mozilla-central/source/build/moz.configure/rust.configure#70 ?
Attachment #8987770 -
Flags: review?(nfroyd) → review+
Updated•6 years ago
|
Attachment #8987762 -
Flags: review?(core-build-config-reviews) → review?(nfroyd)
Comment 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8987762 [details] Bug 1447116 - Update builders to rust 1.28. https://reviewboard.mozilla.org/r/253036/#review259666 Sure, we can do this.
Attachment #8987762 -
Flags: review?(nfroyd) → review+
Reporter | ||
Comment 17•6 years ago
|
||
mozreview-review |
Comment on attachment 8987770 [details] Bug 1447116 - Require rust 1.26. https://reviewboard.mozilla.org/r/253040/#review259768 ::: commit-message-2bb75:4 (Diff revision 1) > +Now that we're building with 1.28 (albeit a beta, due to be bumped when > +it's released), we can bump the requirement away from 1.24 which is now > +old. 1.27 is too new, though, so settle for the older 1.26. If we're going to make this change here it should be accompanied by a change to bootstrap.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•6 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 497fc5c45f44239a54263639268361e5e521cfe2 -d 9f4d1a4296ba: rebasing 469906:497fc5c45f44 "Bug 1447116 - Update builders to rust 1.28. r=froydnj" merging taskcluster/ci/toolchain/windows.yml warning: conflicts while merging taskcluster/ci/toolchain/windows.yml! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 23•6 years ago
|
||
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/autoland/rev/82dc9159f28d Update builders to rust 1.28. r=froydnj https://hg.mozilla.org/integration/autoland/rev/0c8c7b025aee Require rust 1.26. r=froydnj
Reporter | ||
Updated•6 years ago
|
Assignee: cmanchester → mh+mozilla
Comment 24•6 years ago
|
||
Backed out 2 changesets (bug 1447116) for debug reftests failures. Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0c8c7b025aeed7de7a6303aa3ea1c07da020f450 Backout link: https://hg.mozilla.org/integration/autoland/rev/bdd5d81235fbea330de7188b9ada1ce6ca508643 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=185051882&repo=autoland
Assignee | ||
Updated•6 years ago
|
Attachment #8963825 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•6 years ago
|
||
mozreview-review |
Comment on attachment 8987770 [details] Bug 1447116 - Require rust 1.26. https://reviewboard.mozilla.org/r/253040/#review259950 ::: build/moz.configure/rust.configure:71 (Diff revisions 3 - 4) > > You can install rust by running './mach bootstrap' > or by directly running the installer from https://rustup.rs/ > ''')) > rustc_min_version = Version('1.26.0') > - cargo_min_version = Version('0.{}'.format(rustc_min_version.minor + 1)) > + cargo_min_version = rustc_min_version I learned today that as of 1.26 cargo has the same version as rustc, so changing that at the same time. That can't reland until bug 1471497 is figured out, though.
Comment 28•6 years ago
|
||
When this landed (comment 23), we noticed these build_metrics regressions: == Change summary for alert #14048 (as of Tue, 26 Jun 2018 23:20:22 GMT) == Regressions: 37% build times windows2012-64 opt plain taskcluster-c4.4xlarge 1,969.34 -> 2,707.11 0% installer size osx-cross opt 65,616,489.92 -> 65,903,526.00 0% installer size linux32 pgo 63,218,017.50 -> 63,430,315.17 0% installer size linux64 pgo 62,524,675.50 -> 62,717,406.83 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14048
Comment 29•6 years ago
|
||
It looks like *landing 1.28* made build times *substantially* better: it was the backout that caused the regression. https://treeherder.mozilla.org/perf.html#/graphs?timerange=1209600&series=autoland,1682241,1 shows a graph. The original Perfherder alert showing the improvement is at https://treeherder.mozilla.org/perf.html#/alerts?id=14037.
Comment 30•6 years ago
|
||
It appears Linux also saw a major bump: https://treeherder.mozilla.org/perf.html#/graphs?timerange=604800&series=autoland,1682289,1,2&series=autoland,1682265,1,2&series=autoland,1691945,1,2&series=autoland,1682223,1,2&series=autoland,1682281,1,2&series=autoland,1697287,1,2 But this was backed out before a Perfherder alert could be generated. We should see the alerts when this sticks its landing.
Comment 31•6 years ago
|
||
> it was the backout that caused the regression
This is great to read! I was *very* worried for a while there :)
Comment 32•6 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #29) > It looks like *landing 1.28* made build times *substantially* better: it was > the backout that caused the regression. > > https://treeherder.mozilla.org/perf.html#/ > graphs?timerange=1209600&series=autoland,1682241,1 shows a graph. The > original Perfherder alert showing the improvement is at > https://treeherder.mozilla.org/perf.html#/alerts?id=14037. Oh, I apologize for making this confusion. I looked at the wrong alert. :gps thank you for correcting me!
Comment 33•6 years ago
|
||
The correct results are these: == Change summary for alert #14037 (as of Tue, 26 Jun 2018 19:02:03 GMT) == Improvements: 26% build times windows2012-64 opt plain taskcluster-c4.4xlarge 2,664.49 -> 1,961.31 4% build times linux64 opt plain taskcluster-c4.4xlarge 2,052.91 -> 1,968.52 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14037
Assignee | ||
Comment 34•6 years ago
|
||
1.28 beta 6 has the fix for bug 1471497. I'll update the patches and land that, and keep this bug open to update to final 1.28 when it's released at the beginning of August.
Keywords: leave-open
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 37•6 years ago
|
||
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/autoland/rev/d3bb349d164c Update builders to rust 1.28. r=froydnj https://hg.mozilla.org/integration/autoland/rev/679b00b480af Require rust 1.26. r=froydnj
Comment 38•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d3bb349d164c https://hg.mozilla.org/mozilla-central/rev/679b00b480af
Comment 39•6 years ago
|
||
I couldn't help but notice that the compile speedups seem to mostly have an impact on opt builds. It appears opt builds in CI are now almost as fast as debug builds. We had previously changed the Rust optimization level on local builds to a lower level to speed up builds. This was definitely a compromise and a handful of people complained about this because it meant local builds no longer had reasonable performance/behavior characteristics by default. With optimized builds apparently getting a hefty speedup, should we revisit this decision and make local builds use higher optimization levels again [if Rust is sufficiently modern]?
Flags: needinfo?(mh+mozilla)
Comment 40•6 years ago
|
||
I guess we may need a separate task to enforce that code gets pushed still builds with 1.26, otherwise people may start breaking the 1.26 requirement by using new features from 1.27 and 1.28.
Comment 41•6 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive until July 7) from comment #40) > I guess we may need a separate task to enforce that code gets pushed still > builds with 1.26, otherwise people may start breaking the 1.26 requirement > by using new features from 1.27 and 1.28. They're called the linux64-base-toolchain tasks: https://searchfox.org/mozilla-central/source/taskcluster/ci/build/linux.yml#242
Comment 42•6 years ago
|
||
Oh, okay, thanks for the pointer. Maybe it's worth having something similar for Windows and macOS as well, but maybe there aren't too many platform-specific code in Rust so it's probably not too bad. It may still make sense to have that for C++/ObjC code though.
Comment 43•6 years ago
|
||
Talos also noticed perf improvements: == Change summary for alert #14131 (as of Mon, 02 Jul 2018 04:00:17 GMT) == Improvements: 3% displaylist_mutate windows10-64-qr opt e10s stylo 4,861.50 -> 4,731.66 2% displaylist_mutate linux64-qr opt e10s stylo 5,323.16 -> 5,206.26 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14131
Comment 44•6 years ago
|
||
Also note we saw a ~580K reduction in libxul section sizes with this push, primarily in .text and .rodata but also a 26K reduction in .data.rel.ro.
Reporter | ||
Comment 45•6 years ago
|
||
Merged to central in comment 38.
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
Comment 46•6 years ago
|
||
glandium wanted to keep this open per comment 34. As long as we have an open bug blocking release to track upgrading to non-beta 1.28, I don't care whether we reopen this or file a new bug. But I think sheriffs prefer new bugs over bugs with patches landing over the span of weeks.
Reporter | ||
Comment 47•6 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #46) > glandium wanted to keep this open per comment 34. As long as we have an open > bug blocking release to track upgrading to non-beta 1.28, I don't care > whether we reopen this or file a new bug. But I think sheriffs prefer new > bugs over bugs with patches landing over the span of weeks. Yes, missed that, my mistake.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 48•6 years ago
|
||
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 49•6 years ago
|
||
Assignee | ||
Comment 50•6 years ago
|
||
If you look at these graphs you'll see how crazy long both gkrust and gkrust_gtest were taking to build with 1.24, compared to 1.28, and that's most probably LTO. So that can't really tell much about what optimization levels changes would do to local builds.
Assignee | ||
Comment 51•6 years ago
|
||
I'll note that gkrust waiting for gkrust_gtest to finish is weird too.
Assignee | ||
Comment 52•6 years ago
|
||
Erf, I had kept this open for the update to the final release, but filed a separate bug for that, so let's close this.
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Keywords: leave-open
Updated•6 years ago
|
status-firefox63:
--- → fixed
Target Milestone: --- → mozilla63
You need to log in
before you can comment on or make changes to this bug.
Description
•