Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upMIR CopyPropagation is slow #36673
Comments
This comment has been minimized.
This comment has been minimized.
|
I figured out how to install older nightly versions with rustup - I'm going to try searching for the problem |
This comment has been minimized.
This comment has been minimized.
|
Is the code available somewhere (or a smaller reproduction of the bug)? Can you try running with |
This comment has been minimized.
This comment has been minimized.
|
I can definitely send the code in private, but unfortunately I can't make it publicly available at this point. I'd be more than happy to send you a targz (or some other format, if you prefer) I'll try a few different versions to see where it happens, and measure time-passes of the affected version(s) |
This comment has been minimized.
This comment has been minimized.
And here's the timing pass on the last version: I don't quite understand the output.. it seems like the timing pass describes almost none of the walltime, and yet rustc is pegged at 100% CPU for about 22 seconds before it even gets to the first line of the time output. I don't know if this is because it's summarized only after completion? Otherwise, something weird seems to be going on.. |
This comment has been minimized.
This comment has been minimized.
|
Here's something else interesting: This means about 94% of the time is spent on the parsing step (or the steps leading up to it). I'm guessing this means my macros are too heavy? |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
Oops, you're quite right. This is that: |
This comment has been minimized.
This comment has been minimized.
|
cc @pcwalton |
This comment has been minimized.
This comment has been minimized.
|
Is there anything I can do to help? |
This comment has been minimized.
This comment has been minimized.
|
Nominated to discuss priority. Marked as regression. |
pnkfelix
added
regression-from-stable-to-beta
P-high
and removed
regression-from-stable-to-nightly
I-nominated
labels
Sep 29, 2016
This comment has been minimized.
This comment has been minimized.
|
@chrivers You said that you were not willing to post the code publicly, but that you are willing to send it via some private channel? If you have some reduced test case to share, are you willing to share it with all the members of the rust compiler team? Or is there some non-disclosure agreement issues in play? |
This comment has been minimized.
This comment has been minimized.
|
I haven't really been able to figure out what causes this, but I believe it could be because I'm building a large-ish structure in memory. I actually ended up creating a template compiler just for this (kind of) use case: https://github.com/chrivers/transwarp It's nothing super secret at all, and sharing it with the rust language team is certainly fine. Just as long as it doesn't hit google, that's no problem :) There are some licensing terms I have to clear, before putting it on github. But nothing that prevents me from sharing it with a closed group of experts, certainly. @pnkfelix can you email me? I can send you a tarball |
This comment has been minimized.
This comment has been minimized.
|
Nominating to discuss how to fix this in compiler team mtg. |
nikomatsakis
added
the
I-nominated
label
Oct 6, 2016
This comment has been minimized.
This comment has been minimized.
|
Please assign someone. |
This comment has been minimized.
This comment has been minimized.
|
I think we should probably comment out the copy-propagation fix at least for beta:
But we should separately be moving on improving the performance/correctness of CopyPropagation, which is definitely an important thing to have. |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
@arielb1 ah ok I was under the impression it did not do that yet. I have to go re-read the code. |
This comment has been minimized.
This comment has been minimized.
|
Conclusion from meeting: Proper fix then for nightly would be to try to cache work here, but for the beta branch an easy short-term fix is to disable copy-prop. I'll assign myself the beta PR. |
nikomatsakis
removed
the
I-nominated
label
Oct 6, 2016
nikomatsakis
self-assigned this
Oct 6, 2016
This comment has been minimized.
This comment has been minimized.
|
@pnkfelix I've sent you and @nikomatsakis a source tarball which reproduces the issue, and I've mentioned the exact version of at least one rustc version with this problem in the mail. I'm very interested in any updates - and let me know if I can do anything else :) |
This comment has been minimized.
This comment has been minimized.
|
@nikomatsakis @pnkfelix still waiting on a fix to beta. There's about 2 weeks left to get it in. |
This comment has been minimized.
This comment has been minimized.
|
I have been in private communication with @chrivers ; I was able to replicate the regression injected between 9/13 and 9/21, it appears (based on my informal measurements) to have was resolved sometime between 9/22 and 9/27. |
This comment has been minimized.
This comment has been minimized.
|
(I want to verify that @chrivers is observing the same outcome on their end, but have not yet heard back.) |
This comment has been minimized.
This comment has been minimized.
|
Compiler team meeting: waiting to hear back from @chrivers before we decide whether to close or what. We're a bit curious how this got resolved. =) |
This comment has been minimized.
This comment has been minimized.
|
Hey everybody. Can confirm that I have received the emails! However, I'm in a summer house at the moment, and there is only exactly enough internet connection to send you this message :) I'll be back on monday, then I'll look into the different versions. I'd say there's a chance that the bug got resolved after my initial report, but before the testing. I'm not sure yet, however. I'll keep you posted |
This comment has been minimized.
This comment has been minimized.
|
After discussion at compiler team mtg last night, I decided to double-check the -Z time-passes output for the crate in question at three dates:
a gist with the output is here: I think this data confirms that the compiler got faster between Sept 22 and Sept 27, and the difference between Sept 9 and Sept 27 is probably in the noise. of particular interest: CopyPropagation went down between Sept 22 and Sept 27 sept 22: time: 8.683 CopyPropagation |
This comment has been minimized.
This comment has been minimized.
|
Thanks for following up @pnkfelix! |
This comment has been minimized.
This comment has been minimized.
|
@pnkfelix think you can test with the current beta? we're not sure whether the "fix" (whatever it was!) made it to beta. Separately, it would prob be worth trying to find out what has changed, also. It's confusing why it's faster... |
This comment has been minimized.
This comment has been minimized.
|
Monday is probably the cutoff for backporting to beta if this isn't in already. |
This comment has been minimized.
This comment has been minimized.
|
The test crate in question does not compile out of the box with the beta branch itself (it uses features only available on nightly). However, good news: The point in my experiments where the speed became acceptable again was the nightly from sept 27, which is this commit: d0623cf That commit, if I understand the output of |
This comment has been minimized.
This comment has been minimized.
|
triage: P-medium It is still a mystery why this is a problem, but not a burning issue at this point. |
rust-highfive
added
P-medium
and removed
P-high
labels
Nov 3, 2016
This comment has been minimized.
This comment has been minimized.
|
Here is a small test case adapted from a vast reduction of the code that @chrivers sent. (In this reduction, I have left in some seemingly unrelated details, such as repeated calls to // one_file.rs
fn bit() -> Option<bool> { unimplemented!() }
macro_rules! def_thing_fields {
( { $($field:ident,)* } )
=>
{
pub struct Thing { $(pub $field: (),)* }
pub fn go()
{
Thing
{
$(
$field:
{
println!("Thing");
println!("{}", "Thing");
println!("{}", stringify!($field));
let b = match bit() {
Some(x) => x,
None => return,
};
if b {
let result = match bit() {
Some(x) => x,
None => return,
};
println!("next_bit -> {:?}", result);
}
}
),*
};
}
}
}
fn main() {
}
def_thing_fields! {
{
a00, a01, a02, a03, a04, a05, a06, a07, a08, a09,
a10, a11, a12, a13, a14, a15, a16, a17, a18, a19,
a20, a21, a22, a23, a24, a25, a26, a27, a28, a29,
a30, a31, a32, a33, a34, a35, a36, a37, a38, a39,
a40, a41, a42, a43, a44, a45, a46, a47, a48, a49,
a50, a51, a52, a53, a54, a55, a56, a57, a58, a59,
a60, a61, a62, a63, a64, a65, a66, a67, a68, a69,
a70, a71, a72, a73, a74, a75, a76, a77, a78, a79,
a80, a81, a82, a83, a84, a85, a86, a87, a88, a89,
a90, a91, a92, a93, a94, a95, a96, a97, a98, a99,
}
}Here are timing results showing that the same compilers I tested above show an analogous change in speed for this test case: |
This comment has been minimized.
This comment has been minimized.
|
Ah! Someone mentioned in the meeting that the time spent in Here's data that reflects this: (and with that knowledge, it seems obvious now why the problem "went away" -- pcwalton's commit 79cb2db deliberately avoids running the |
This comment has been minimized.
This comment has been minimized.
|
@brson the short version of all of this that I do not think there is anything that needs to be backported to beta at this point. |
arielb1
changed the title
Significant compile-speed regression in nightly
MIR CopyPropagation is slow
Nov 10, 2016
arielb1
removed
the
regression-from-stable-to-beta
label
Nov 10, 2016
This comment has been minimized.
This comment has been minimized.
|
We untagged this as a regression but kept the issue open since we would like to eventually make copy-prop fast |
This comment has been minimized.
This comment has been minimized.
|
FYI: copy propagation has moved to -Zmir-opt-level=2 (default is 1) |
Mark-Simulacrum
added
the
C-enhancement
label
Jul 26, 2017
oyvindln
referenced this issue
Oct 23, 2017
Open
likely/unlikely intrinsics fail to propagate through inline functions #45440
oli-obk
referenced this issue
Jan 5, 2019
Merged
Don't actually create a full MIR stack frame when not needed #57351
bors
added a commit
that referenced
this issue
Jan 5, 2019
bors
added a commit
that referenced
this issue
Jan 10, 2019
bors
added a commit
that referenced
this issue
Jan 10, 2019
pietroalbini
added a commit
to pietroalbini/rust
that referenced
this issue
Jan 12, 2019
Centril
added a commit
to Centril/rust
that referenced
this issue
Jan 13, 2019
Centril
added a commit
to Centril/rust
that referenced
this issue
Jan 13, 2019
chrivers commentedSep 23, 2016
Hello everyone
I've tried searching for a github issue that reports this, but I couldn't find anything, so I hope this isn't a duplicate. I'm on nightly, since my project uses type macros. Overall, there is a good amount of macro expansion going on, since I build parsers for binary data based on structs.
Compilation time for a 5500 LOC project has been around 7-9 seconds on my Core i7-4770, going a bit up and down with changes in the project, and changes in nightly.
However, today this changed very drastically:
[chrivers@helios]~/git/artemis/oxide #cargo build
Compiling oxide v0.1.0 (file:///home/chrivers/git/artemis/oxide)
Finished debug [unoptimized + debuginfo] target(s) in 21.94 secs
This is more than twice as slow as before! Something pretty significant must have happened.
This version is definitely affected:
rustc 1.13.0-nightly (4f9812a59 2016-09-21)Honestly, I'm not quite sure when it worked, but I can say that the version I used yesterday, which was probably no more than 1 week old, worked much better.