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

check_mod_item_types query times regressed bigly #65147

Closed
tmandry opened this issue Oct 6, 2019 · 3 comments · Fixed by #65293 or rust-lang/rustc-perf#508
Closed

check_mod_item_types query times regressed bigly #65147

tmandry opened this issue Oct 6, 2019 · 3 comments · Fixed by #65293 or rust-lang/rustc-perf#508
Assignees
Labels
A-async-await Area: Async & Await AsyncAwait-Polish Async-await issues that are part of the "polish" area AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. I-compiletime Issue: Problems and improvements with respect to compile times. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tmandry
Copy link
Member

tmandry commented Oct 6, 2019

There is a crate inside of Fuchsia whose compile times increased from ~500ms to ~120s in the last month of Rust compiler updates. It hasn't been easy so far to provide a minimal example, but I will keep working at it.

Some bisecting revealed that it actually consisted of two performance regressions, both affecting the time spent in check_mod_item_types:

Commit hash Item Self time % of total time Item count Cache hits Blocked time Incremental load time
87b0c90 check_mod_item_types 34.92ms 7.120 6 0 0.00ns 0.00ns
34e82a7 check_mod_item_types 21.92s 97.786 6 0 0.00ns 0.00ns
66bf391 check_mod_item_types 21.76s 97.838 6 0 0.00ns 0.00ns
0221e26 check_mod_item_types 133.53s 99.526 6 0 0.00ns 0.00ns

The first regression occurred in the rollup #64354, and the second occurred sometime in the two weeks between changes 66bf391..0221e26.

There is heavy use of async/await in the code, and I've identified a couple places where taking out .await makes the compile finish right away. This makes me suspect #64292 for the first regression, but I'm not sure.

I'll update the issue as I have more information.

@tmandry tmandry changed the title check_mod_item_types compile times regressed bigly check_mod_item_types query times regressed bigly Oct 6, 2019
@tmandry
Copy link
Member Author

tmandry commented Oct 6, 2019

There is heavy use of async/await in the code, and I've identified a couple places where taking out .await makes the compile finish right away.

Interestingly, removing .await from either of these places makes the compile much faster. If I leave both in, it's slow, but taking one or both out makes it faster. Maybe there's some kind of cycle being created when both are left in?

In fact, switching on RUSTC_LOG=debug actually causes a cycle error to be reported when there wasn't one before. This is really weird to me, but it may be a red herring. Removing one or both of the .await calls (or calls to the methods that create the futures) doesn't resolve the cycle error, even though it resolves the slowdown. Here's a sample of the error, for completeness.

@nagisa nagisa added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. I-compiletime Issue: Problems and improvements with respect to compile times. A-async-await Area: Async & Await regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Oct 6, 2019
@jonas-schievink jonas-schievink added C-bug Category: This is a bug. I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 6, 2019
@tmandry
Copy link
Member Author

tmandry commented Oct 8, 2019

I don't have a lot more information to put here, except that the compile time seems to increase with the size of a particular call graph of async fns (it's not binary slow/fast, but different gradations of slow). This continues to make it hard to minimize.

I did some profiling, and noticed a lot of time being spent collecting items in SmallVec::iter. Unfortunately I couldn't see what was calling that function. There was some speculation that it could be from this code (cc @Mark-Simulacrum who suggested this).

See this chat thread for more details, and some flame graphs of the profiles I ran.

@nikomatsakis nikomatsakis added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Oct 8, 2019
@nikomatsakis nikomatsakis added AsyncAwait-Polish Async-await issues that are part of the "polish" area and removed AsyncAwait-OnDeck labels Oct 8, 2019
@pnkfelix
Copy link
Member

pnkfelix commented Oct 10, 2019

visited for T-compiler triage. Assigning P-high based on labels applied by niko as part of async-await triage. Removing nomination label too.

@pnkfelix pnkfelix added P-high High priority and removed I-nominated labels Oct 10, 2019
bors added a commit that referenced this issue Oct 14, 2019
Optimize `try_expand_impl_trait_type`

A lot of time was being spent expanding some large `impl Future` types in fuchsia. This PR takes the number of types being visited in one expansion from >3 billion to about a thousand, and eliminates the compile time regression in #65147 (in fact, compile times are better than they were before).

Thanks to @matthewjasper for suggesting this change.

Fixes #65147.
r? @matthewjasper,@nikomatsakis
tmandry added a commit to tmandry/rust that referenced this issue Oct 15, 2019
…sper

Optimize `try_expand_impl_trait_type`

A lot of time was being spent expanding some large `impl Future` types in fuchsia. This PR takes the number of types being visited in one expansion from >3 billion to about a thousand, and eliminates the compile time regression in rust-lang#65147 (in fact, compile times are better than they were before).

Thanks to @Mark-Simulacrum for helping identify the issue and to @matthewjasper for suggesting this change.

Fixes rust-lang#65147.
r? @matthewjasper,@nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await AsyncAwait-Polish Async-await issues that are part of the "polish" area AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. I-compiletime Issue: Problems and improvements with respect to compile times. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
5 participants