Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow for setting a ThinLTO import limit during bootstrap #67450

Merged
merged 2 commits into from
Jan 3, 2020
Merged

Allow for setting a ThinLTO import limit during bootstrap #67450

merged 2 commits into from
Jan 3, 2020

Conversation

michaelwoerister
Copy link
Member

The benchmarks in #66625 have shown that a lower ThinLTO import limit can be a net win for bootstrap times. This PR:

  • exposes the setting to config.toml,
  • defaults to a lower limit if incremental = true in config.toml, and
  • sets a lower limit for x86_64-gnu-llvm-7 CI image in order to make the jobs complete more quickly (which remains to be tested).

This setting will affect how the compiler and it's tools are compiled. It will not affect the settings the compiler uses when compiling user code.

r? @pietroalbini
cc @rust-lang/infra

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 20, 2019
@Mark-Simulacrum
Copy link
Member

r=me on the code changes

@michaelwoerister
Copy link
Member Author

michaelwoerister commented Dec 20, 2019 •
edited
Loading

I'm not sure if that pr (Linux x86_64-gnu-llvm-7) job has additional overhead for rebuilding the docker image?

@michaelwoerister
Copy link
Member Author

Fixed a typo in a comment. We'll see if this builds faster now that the docker image is cached.

@pietroalbini
Copy link
Member

Fixed a typo in a comment. We'll see if this builds faster now that the docker image is cached.

PR builders don't cache the built docker images.

@pietroalbini pietroalbini added I-nominated T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Dec 20, 2019
@michaelwoerister
Copy link
Member Author

Not sure if this build was an outlier but it finished 10-15 min faster than all the other PRs I was browsing through just now.

@michaelwoerister
Copy link
Member Author

Or not :) variance seems to be quite high.

@bors
Copy link
Contributor

bors commented Dec 23, 2019

☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts.

@michaelwoerister michaelwoerister mentioned this pull request Dec 24, 2019
Closed
@pietroalbini
Copy link
Member

Just testing highfive, ignore this.

r? @ghost

@michaelwoerister
Copy link
Member Author

@Mark-Simulacrum, is this still an r+ from you if I remove the changes to the CI job, but keep the changes to bootstrap? I think those are pretty useful for local builds.

@Mark-Simulacrum
Copy link
Member

Actually, in the interest of seeing if it is an improvement, I would be on board landing the Docker file change - it's unlikely to hurt and could plausibly be something that we propagate to other builders, too. I don't think we'll get data unless we try it out :)

Looks like this has some merge conflicts but I'm otherwise r+ on the whole PR.

It also sounds like there's some ground to be covered in exposing this implicitly to end-user compiles, which is likely out of scope for this PR but something we shouldn't lose track of.

@pietroalbini
Copy link
Member

r? @Mark-Simulacrum then

@michaelwoerister
Copy link
Member Author

@bors r=Mark-Simulacrum

It also sounds like there's some ground to be covered in exposing this implicitly to end-user compiles, which is likely out of scope for this PR but something we shouldn't lose track of.

Yes, that might make sense for local builds. This PR already adjusts the default for incremental builds.

@bors
Copy link
Contributor

bors commented Jan 2, 2020

📌 Commit 6f57bad has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 2, 2020
@bors
Copy link
Contributor

bors commented Jan 2, 2020

⌛ Testing commit 6f57bad with merge 4ab4a97...

bors added a commit that referenced this pull request Jan 2, 2020
@bors
Auto merge of #67450 - michaelwoerister:bootstrap-import-limit, r=Mar…
4ab4a97 
…k-Simulacrum

Allow for setting a ThinLTO import limit during bootstrap

The benchmarks in #66625 have shown that a lower ThinLTO import limit can be a net win for bootstrap times. This PR:
- exposes the setting to `config.toml`,
- defaults to a lower limit if `incremental = true` in `config.toml`, and
- sets a lower limit for `x86_64-gnu-llvm-7` CI image in order to make the jobs complete more quickly (which remains to be tested).

This setting will affect how the compiler and it's tools are compiled. It will not affect the settings the compiler uses when compiling user code.

r? @pietroalbini
cc @rust-lang/infra
@rust-highfive
Copy link
Collaborator

The job i686-mingw-1 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log. 
2020-01-02T15:15:17.6086619Z 
2020-01-02T15:15:17.6086654Z 
2020-01-02T15:15:17.6783674Z failed to run: D:\a\1\s\build\bootstrap\debug\bootstrap test --exclude src/test/ui --exclude src/test/compile-fail
2020-01-02T15:15:17.6784538Z Build completed unsuccessfully in 1:44:04
2020-01-02T15:15:17.7179994Z make: *** [Makefile:89: ci-mingw-subset-1] Error 1
2020-01-02T15:15:17.7777001Z   local time: Thu Jan  2 15:15:17 CUT 2020
2020-01-02T15:15:18.4348224Z   network time: Thu, 02 Jan 2020 15:15:18 GMT
2020-01-02T15:15:18.4355483Z == end clock drift check ==
2020-01-02T15:15:18.5283267Z 
2020-01-02T15:15:18.5283267Z 
2020-01-02T15:15:18.8637579Z ##[error]Bash exited with code '2'.
2020-01-02T15:15:18.9011554Z ##[section]Starting: Checkout
2020-01-02T15:15:18.9671492Z ==============================================================================
2020-01-02T15:15:18.9671651Z Task         : Get sources
2020-01-02T15:15:18.9671738Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors
Copy link
Contributor

bors commented Jan 2, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 2, 2020
@michaelwoerister
Copy link
Member Author

@bors retry

@rust-lang/infra Seems like a spurious failure?

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 2, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 3, 2020
@JohnTitor
Rollup merge of rust-lang#67450 - michaelwoerister:bootstrap-import-l…
7c404ce 
…imit, r=Mark-Simulacrum

Allow for setting a ThinLTO import limit during bootstrap

The benchmarks in rust-lang#66625 have shown that a lower ThinLTO import limit can be a net win for bootstrap times. This PR:
- exposes the setting to `config.toml`,
- defaults to a lower limit if `incremental = true` in `config.toml`, and
- sets a lower limit for `x86_64-gnu-llvm-7` CI image in order to make the jobs complete more quickly (which remains to be tested).

This setting will affect how the compiler and it's tools are compiled. It will not affect the settings the compiler uses when compiling user code.

r? @pietroalbini
cc @rust-lang/infra
@JohnTitor JohnTitor mentioned this pull request Jan 3, 2020
Merged
bors added a commit that referenced this pull request Jan 3, 2020
@bors
Auto merge of #67828 - JohnTitor:rollup-qmswkkl, r=JohnTitor
30ddb5a 
Rollup of 10 pull requests

Successful merges:

 - #67450 (Allow for setting a ThinLTO import limit during bootstrap)
 - #67595 (Suggest adding a lifetime constraint for opaque type)
 - #67636 (allow rustfmt key in [build] section)
 - #67736 (Less-than is asymmetric, not antisymmetric)
 - #67762 (Add missing links for insecure_time)
 - #67783 (Warn for bindings named same as variants when matching against a borrow)
 - #67796 (Ensure that we process projections during MIR inlining)
 - #67807 (Use drop instead of the toilet closure `|_| ()`)
 - #67816 (Clean up err codes)
 - #67825 (Minor: change take() docs grammar to match other docs)

Failed merges:

r? @ghost
@bors bors merged commit 6f57bad into rust-lang:master Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@michaelwoerister @Mark-Simulacrum @pietroalbini @bors @rust-highfive