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.

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

Middle size, 80 000 lines, project but very slow, 5 hours, compilation #65131

Closed
cheblin opened this issue Oct 5, 2019 · 11 comments · Fixed by #70176
Closed

Middle size, 80 000 lines, project but very slow, 5 hours, compilation #65131

cheblin opened this issue Oct 5, 2019 · 11 comments · Fixed by #70176
Labels
C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@cheblin
Copy link

cheblin commented Oct 5, 2019

This code is the MAVLink  protocol handler, and generated by one of the MAVLink specification. It is  C/Rust  mixed project, so to compile it you will need  RustCC  an any  C  compiler.

To check how slow  rustc  is, check this out , and run  cargo test  in the  GroundControl/demo  dir

Exactly this project is compiled with error, cause the borrow checker complains in one place.

But anyway, the problem is that compilation is unacceptably slow.

Description

/GroundControl/lib   generated API

/GroundControl/demo/src/use_case  is generated sample of using generated API

/GroundControl/demo/src/test  the test

I generate variation of this project, with same functionality, in C, C++, C#.... They are the compiled in seconds !

What wrong with my Rust project? How can I speed up compilation?

My PC

Intel I7-7700K

RAM 32G

SSD Samsung 970 PRO

cargo -V

cargo 1.39.0-nightly (ab6fa8908 2019-09-25)

rustc -V

rustc 1.40.0-nightly (22bc9e1 2019-09-30)

@jonas-schievink
Copy link
Contributor

Please provide build instructions for that code. Running cargo build in GroundControl/lib gives:
fatal error: ../config.h: No such file or directory.

@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 5, 2019
@cheblin
Copy link
Author

cheblin commented Oct 5, 2019

GroundControl/lib gives: gives:.....

But I wrote

and run cargo test in the GroundControl/demo dir

@jonas-schievink
Copy link
Contributor

That gives the same error, since it just has a dependency on the crate in lib...

@cheblin
Copy link
Author

cheblin commented Oct 5, 2019

Small bug fix in the build.rs file.
Please try again.
Thanks.

@jonas-schievink
Copy link
Contributor

That didn't fix it

@cheblin
Copy link
Author

cheblin commented Oct 5, 2019

on my debian and windows 10 build script shown:

cargo:warning=Starting search config.h in....
cargo:warning=/root/InRUST/GroundControl/demo/target/debug/build/black_box-sys-57f37c06735e9d12/out
cargo:warning=/root/InRUST/GroundControl/demo/target/debug/build/black_box-sys-57f37c06735e9d12
cargo:warning=/root/InRUST/GroundControl/demo/target/debug/build
cargo:warning=/root/InRUST/GroundControl/demo/target/debug
cargo:warning=/root/InRUST/GroundControl/demo/target
cargo:warning=/root/InRUST/GroundControl/demo
cargo:warning=<<< Find and use generated config.h in root/InRUST/GroundControl/lib/src/ >>>

@cheblin
Copy link
Author

cheblin commented Oct 5, 2019

made more platform specific fixes.
check now please.

@tanriol
Copy link
Contributor

tanriol commented Oct 6, 2019

I've looked into this code and likely found the problem.

The bright side: it compiles in under half a minute if the errors are removed.

The part of the code causing this problem is a test function that tries to test everything at once and has about 8000 lines. The function contains (as of commit ee653b1a0170) 76 borrow checker errors and the part that takes 5 hours is generating the 3-point errors for them. The time is dominated by the repeated recalculation of dominators for the function body.

@tanriol
Copy link
Contributor

tanriol commented Oct 6, 2019

A minimized reproduction looks like this

fn get_pair(_a: &mut u32, _b: &mut u32) {
}

macro_rules! x10 {
    ($($t:tt)*) => {
        $($t)* $($t)* $($t)* $($t)* $($t)*
        $($t)* $($t)* $($t)* $($t)* $($t)*
    }
}

#[allow(unused_assignments)]
fn main() {
    let mut x = 1;

    get_pair(&mut x, &mut x);

    x10!{ x10!{ x10!{ if x > 0 { x += 2 } else { x += 1 } } } }
}

On my laptop this code requires about 25 seconds to print the error for the 1000 if statements generated above and scales with the cube of the statement count.

tanriol added a commit to tanriol/rust that referenced this issue Oct 6, 2019
This looks like the only place calculating dominators from the MIR body every time instead of using the ones stored on the `MirBorrowckCtxt`. For example, in rust-lang#65131 a big generated function with a number of borrowck errors takes a few hours(!) recalculating the dominators while explaining the errors.

I don't know enough about this part of rustc codebase to know for sure that this change is correct, but no tests seem to fail as a result of this change in local testing.
@jonas-schievink jonas-schievink added C-bug Category: This is a bug. and removed C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Oct 6, 2019
@H2CO3
Copy link

H2CO3 commented Oct 7, 2019

On the bright side, waiting 5 hours for borrowck errors is still faster than trying to debug a memory corruption in a language without borrowck. 😜

(edit: lol, why so serious, this was meant to be humorous and not a complaint about Rust…)

@joshtriplett
Copy link
Member

Thanks to @cheblin for the report and helping us to work with it, and to @tanriol for the quick analysis and fix!

@nagisa nagisa added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Oct 16, 2019
bors added a commit that referenced this issue Oct 16, 2019
… r=nagisa

use precalculated dominators in explain_borrow

This looks like the only place calculating dominators from the MIR body every time instead of using the ones stored on the `MirBorrowckCtxt`. For example, in #65131 a big generated function with a number of borrowck errors takes a few hours(!) recalculating the dominators while explaining the errors.

I don't know enough about this part of rustc codebase to know for sure that this change is correct, but no tests seem to fail as a result of this change in local testing.
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this issue Oct 22, 2019
This looks like the only place calculating dominators from the MIR body every time instead of using the ones stored on the `MirBorrowckCtxt`. For example, in rust-lang#65131 a big generated function with a number of borrowck errors takes a few hours(!) recalculating the dominators while explaining the errors.

I don't know enough about this part of rustc codebase to know for sure that this change is correct, but no tests seem to fail as a result of this change in local testing.
Centril pushed a commit to Centril/rust that referenced this issue Mar 21, 2020
Centril added a commit to Centril/rust that referenced this issue Mar 21, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 21, 2020
Rollup of 16 pull requests

Successful merges:

 - rust-lang#65097 (Make std::sync::Arc compatible with ThreadSanitizer)
 - rust-lang#69033 (Use generator resume arguments in the async/await lowering)
 - rust-lang#69997 (add `Option::{zip,zip_with}` methods under "option_zip" gate)
 - rust-lang#70038 (Remove the call that makes miri fail)
 - rust-lang#70058 (can_begin_literal_maybe_minus: `true` on `"-"? lit` NTs.)
 - rust-lang#70111 (BTreeMap: remove shared root)
 - rust-lang#70139 (add delay_span_bug to TransmuteSizeDiff, just to be sure)
 - rust-lang#70165 (Remove the erase regions MIR transform)
 - rust-lang#70166 (Derive PartialEq, Eq and Hash for RangeInclusive)
 - rust-lang#70176 (Add tests for rust-lang#58319 and rust-lang#65131)
 - rust-lang#70177 (Fix oudated comment for NamedRegionMap)
 - rust-lang#70184 (expand_include: set `.directory` to dir of included file.)
 - rust-lang#70187 (more clippy fixes)
 - rust-lang#70188 (Clean up E0439 explanation)
 - rust-lang#70189 (Abi::is_signed: assert that we are a Scalar)
 - rust-lang#70194 (#[must_use] on split_off())

Failed merges:

r? @ghost
@bors bors closed this as completed in 855eac3 Mar 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants