Skip MIR encoding for cargo check by varkor · Pull Request #49433 · rust-lang/rust

varkor · 2018-03-27T21:55:11Z

Resolves#48662.

leonardo-m · 2018-03-27T22:07:54Z

I think this also works if you use "rustc --emit=metadata" (and you aren't using Cargo).

varkor · 2018-03-27T22:18:13Z

I think this also works if you use "rustc --emit=metadata" (and you aren't using Cargo).

Yes, this is exactly what cargo check uses behind the scenes :) (I simply mentioned Cargo to align with the original issue.)

Mark-Simulacrum · 2018-03-27T22:39:20Z


+ fn metadata_output_only(&self) -> bool {
+ // MIR optimisation can be skipped when we're just interested in the metadata.
+ self.tcx.sess.opts.output_types.keys().count() == 1 &&


Please add a len() impl to OutputTypes in https://github.com/rust-lang/rust/blob/master/src/librustc/session/config.rs and call that instead of the keys(). count().

nagisa · 2018-03-28T19:49:14Z

This solution is debatable. metadata does contain MIR and therefore, it is expected, that with --emit=metadata you get exactly the same object file that would otherwise be included into your final binary (if e.g. emit=link was used).

Basically, it should be possible to reconstruct something resembling a final linked result from component received from --emit=metadata,obj. It perhaps might be worthwhile to simply have a -C flag which disables the optimisations.

shepmaster · 2018-03-30T19:09:44Z

r? @nagisa

michaelwoerister · 2018-04-03T09:50:19Z

I think the solution is fine. We don't specify that --emit=metadata should be exactly the same as the metadata in an RLIB, just that we have enough metadata to compile downstream crates.

Let's see how much of a difference this even makes:
@bors try

bors · 2018-04-03T09:50:30Z

⌛ Trying commit 308f381 with merge a856992...

@michaelwoerister

Skip MIR optimisation for cargo check Resolves#48662. r? @michaelwoerister

michaelwoerister · 2018-04-03T11:11:10Z

After chatting a bit on IRC, @nagisa and I came to the conclusion that it would be better (i.e. more future-proof) to use -Zmir-opt-level for reducing the amount of optimization we do. @nagisa, would you be able to help @varkor with reviewing the optimization passes? I'm not quite sure which of those are actually needed for correctness.

bors · 2018-04-03T13:50:30Z

💥 Test timed out

michaelwoerister · 2018-04-03T14:44:59Z

@bors try

michaelwoerister · 2018-04-03T14:47:48Z

Maybe we should discuss this in @rust-lang/compiler meeting. The PR, as implemented here does not really pose a regression to what one can do with --emit=metadata RLIBs (as @eddyb pointed out on IRC): One can already not compile a regular crate with a metadata-only RLIB dependency. Not computing and storing the MIR does seem like the most efficient approach.

michaelwoerister · 2018-04-03T14:53:15Z

@Mark-Simulacrum, something has gone wrong with the try-build. Could you restart it? I want to get some perf numbers in order to find out if the speedups are even worth the fuss :)

eddyb · 2018-04-03T17:28:10Z

 }

+ pub fn len(&self) -> usize {
+ self.0.keys().count()


Doesn't BTreeMap have a len method?

nagisa · 2018-04-03T18:32:09Z

So a mir-opt-level based solution would end up looking like this for almost every MIR pass.

A number of passes here are all a good target for having their mir-opt-level bumped.

AFAIR currently mir-opt-level = opt-level + 1 specifically to allow checking against mir-opt-level > 0 to almost always run the pass, and to allow specifying -Zmir-opt-level=0 to disable most of the passes that matter.

It might make sense to stabilise this option to some degree and also review which passes are run at what levels.

eddyb · 2018-04-04T09:16:55Z

I don't understand why we need to touch anything about optimizations, instead of the current status of the PR which skips encoding? I expect serializing MIR to potentially be costlier than doing most of the enabled-by-default optimizations.

nagisa · 2018-04-04T10:15:31Z

Oh, that is a fair point, and I had completely missed that, due to how trivial the PR looked and how misleading the title was.

I guess this approach works for me then provided we figure out what exactly would make sure that we still end up querying the "analysed" MIR for everything, so that MIR borrowck and friends are still executed.

cc @nikomatsakis what ensures that "analysed" MIR is queried if "optimised" MIR is not?

ishitatsuyuki · 2018-04-07T02:42:03Z

With the current commits, is MIR emitting suppressed, or is it still emitted unoptimised? It looks like you have changed the title, but I'm not sure if the patch reflects it.

Edit: looks like disabling MIR inlining also suppress MIR encoding.

Also, I'd prefer having this as a flag, similar to what @nagisa said. I have #44587 in mind, and we should not diverge rmeta from what is embedded inside the rlib.

ishitatsuyuki · 2018-04-07T09:43:12Z


+ fn metadata_output_only(&self) -> bool {
+ // MIR optimisation can be skipped when we're just interested in the metadata.
+ self.tcx.sess.opts.output_types.len() == 1 &&


Cargo also includes dep-info as output; we probably need to check if we have anything any output that requires codegen.

Good catch! This method could be implemented as
fnmetadata_output_only(&self) -> bool{// MIR optimisation can be skipped when we're just interested in the metadata. !self.tcx.sess.opts.output_types.should_trans()}

ishitatsuyuki · 2018-04-10T08:21:08Z

How much does this speed up cargo check? I think bors retry will restart the try build.

michaelwoerister · 2018-04-10T11:49:00Z

@bors try

bors · 2018-04-10T11:49:12Z

⌛ Trying commit 5576ce8 with merge f2c12f4...

@michaelwoerister

Skip MIR encoding for cargo check Resolves#48662. r? @michaelwoerister

michaelwoerister · 2018-04-10T11:49:15Z

@ishitatsuyuki Let's take a look.

bors · 2018-04-10T14:16:04Z

☀️ Test successful - status-travis
State: approved= try=True

ishitatsuyuki · 2018-04-10T14:22:05Z

@Mark-Simulacrum Can you please start a perf run?

Mark-Simulacrum · 2018-04-11T12:38:39Z

Perf started.

ishitatsuyuki · 2018-04-12T00:04:07Z

Up to 10% faster on check, others are noise.

https://perf.rust-lang.org/compare.html?start=2018-04-07&end=f2c12f4397efb47adcaa152b1e63e1051b209a7b&stat=instructions%3Au

ishitatsuyuki · 2018-04-15T08:21:31Z

I think we can merge this. Any concerns? @nagisa @michaelwoerister

michaelwoerister · 2018-04-16T08:37:29Z

I'm interpreting @nagisa's comment as him being OK with it.

@bors r+

Thanks again, @varkor!

bors · 2018-04-16T08:37:30Z

📌 Commit 5576ce8 has been approved by michaelwoerister

bors · 2018-04-16T10:31:05Z

⌛ Testing commit 5576ce8 with merge d6a2dd9...

@michaelwoerister

Skip MIR encoding for cargo check Resolves#48662. r? @michaelwoerister

bors · 2018-04-16T13:21:50Z

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing d6a2dd9 to master...

Skip MIR optimisation for cargo check
f15e5c1

Mark-Simulacrum requested changes Mar 27, 2018
View reviewed changes

rust-highfive assigned nagisa Mar 30, 2018

shepmaster added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 30, 2018

bors added a commit that referenced this pull request Apr 3, 2018
Auto merge of#49433- varkor:metadata-skip-mir-opt, r=<try>
a856992
Skip MIR optimisation for cargo check Resolves#48662. r? @michaelwoerister

michaelwoerister added the I-nominated label Apr 3, 2018

eddyb reviewed Apr 3, 2018
View reviewed changes

varkor changed the title ~~Skip MIR optimisation for cargo check~~Skip MIR encoding for cargo checkApr 4, 2018

Add len() method to OutputTypes
8414520

varkor force-pushed the metadata-skip-mir-opt branch from 308f381 to 8414520Compare April 4, 2018 11:52

ishitatsuyuki reviewed Apr 7, 2018
View reviewed changes

Take OutputType::DepInfo into account for metadata_output_only
5576ce8

varkor force-pushed the metadata-skip-mir-opt branch from 005d032 to 5576ce8Compare April 9, 2018 16:18

bors added a commit that referenced this pull request Apr 10, 2018
Auto merge of#49433- varkor:metadata-skip-mir-opt, r=<try>
f2c12f4
Skip MIR encoding for cargo check Resolves#48662. r? @michaelwoerister

rust-lang deleted a comment from TimNN Apr 10, 2018

Mark-Simulacrum approved these changes Apr 11, 2018
View reviewed changes

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 Apr 16, 2018

bors added a commit that referenced this pull request Apr 16, 2018
Auto merge of#49433- varkor:metadata-skip-mir-opt, r=michaelwoerister
d6a2dd9
Skip MIR encoding for cargo check Resolves#48662. r? @michaelwoerister

bors merged commit 5576ce8 into rust-lang:masterApr 16, 2018

This was referenced Apr 16, 2018
Various rustfmt and commenting changes #49969
Merged
The Great Generics Generalisation: Ty Edition #48523
Merged

varkor deleted the metadata-skip-mir-opt branch April 16, 2018 17:09

pnkfelix mentioned this pull request Dec 3, 2018
NLL error on closure, but not on equivalent function #55526
Closed

tmiasko mentioned this pull request Jan 6, 2021
Cannot create local mono-item for DefId #51388
Closed

bjorn3 mentioned this pull request Nov 25, 2025
Target Stages, an improvement of the incremental system rust-lang/rfcs#3881
Open

Uh oh!

Uh oh!

Conversation

varkor commented Mar 27, 2018

Uh oh!

leonardo-m commented Mar 27, 2018

Uh oh!

varkor commented Mar 27, 2018

Uh oh!

Mark-SimulacrumMar 27, 2018

Choose a reason for hiding this comment

Uh oh!

nagisa commented Mar 28, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shepmaster commented Mar 30, 2018

Uh oh!

michaelwoerister commented Apr 3, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Apr 3, 2018

Uh oh!

michaelwoerister commented Apr 3, 2018

Uh oh!

bors commented Apr 3, 2018

Uh oh!

michaelwoerister commented Apr 3, 2018

Uh oh!

michaelwoerister commented Apr 3, 2018

Uh oh!

michaelwoerister commented Apr 3, 2018

Uh oh!

eddybApr 3, 2018

Choose a reason for hiding this comment

Uh oh!

nagisa commented Apr 3, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddyb commented Apr 4, 2018

Uh oh!

nagisa commented Apr 4, 2018

Uh oh!

ishitatsuyuki commented Apr 7, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ishitatsuyukiApr 7, 2018

Choose a reason for hiding this comment

Uh oh!

michaelwoeristerApr 9, 2018

Choose a reason for hiding this comment

Uh oh!

ishitatsuyuki commented Apr 10, 2018

Uh oh!

michaelwoerister commented Apr 10, 2018

Uh oh!

bors commented Apr 10, 2018

Uh oh!

michaelwoerister commented Apr 10, 2018

Uh oh!

bors commented Apr 10, 2018

Uh oh!

ishitatsuyuki commented Apr 10, 2018

Uh oh!

Mark-Simulacrum commented Apr 11, 2018

Uh oh!

ishitatsuyuki commented Apr 12, 2018

Uh oh!

ishitatsuyuki commented Apr 15, 2018

Uh oh!

michaelwoerister commented Apr 16, 2018

Uh oh!

bors commented Apr 16, 2018

Uh oh!

bors commented Apr 16, 2018

Uh oh!

bors commented Apr 16, 2018

Uh oh!

Reviewers

Assignees

Labels

nagisa commented Mar 28, 2018•
edited
Loading

michaelwoerister commented Apr 3, 2018•
edited
Loading

nagisa commented Apr 3, 2018•
edited
Loading

ishitatsuyuki commented Apr 7, 2018•
edited
Loading