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

cache types during normalization #76928

Merged
merged 1 commit into from
Sep 23, 2020
Merged

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Sep 19, 2020

partially fixes #75992

reduces the following test from 14 to 3 seconds locally.

cc @Mark-Simulacrum would it make sense to add that test to perf?

#![recursion_limit="2048"]
#![type_length_limit="112457564"]

pub async fn h0(v: &String, x: &u64) { println!("{} {}", v, x) }
pub async fn h1(v: &String, x: &u64) { h0(v, x).await }
pub async fn h2(v: &String, x: &u64) { h1(v, x).await }
pub async fn h3(v: &String, x: &u64) { h2(v, x).await }
pub async fn h4(v: &String, x: &u64) { h3(v, x).await }
pub async fn h5(v: &String, x: &u64) { h4(v, x).await }
pub async fn h6(v: &String, x: &u64) { h5(v, x).await }
pub async fn h7(v: &String, x: &u64) { h6(v, x).await }
pub async fn h8(v: &String, x: &u64) { h7(v, x).await }
pub async fn h9(v: &String, x: &u64) { h8(v, x).await }

pub async fn h10(v: &String, x: &u64) { h9(v, x).await }
pub async fn h11(v: &String, x: &u64) { h10(v, x).await }
pub async fn h12(v: &String, x: &u64) { h11(v, x).await }
pub async fn h13(v: &String, x: &u64) { h12(v, x).await }
pub async fn h14(v: &String, x: &u64) { h13(v, x).await }
pub async fn h15(v: &String, x: &u64) { h14(v, x).await }
pub async fn h16(v: &String, x: &u64) { h15(v, x).await }
pub async fn h17(v: &String, x: &u64) { h16(v, x).await }
pub async fn h18(v: &String, x: &u64) { h17(v, x).await }
pub async fn h19(v: &String, x: &u64) { h18(v, x).await }


macro_rules! async_recursive {
    (29, $inner:expr) => { async { async_recursive!(28, $inner) }.await };
    (28, $inner:expr) => { async { async_recursive!(27, $inner) }.await };
    (27, $inner:expr) => { async { async_recursive!(26, $inner) }.await };
    (26, $inner:expr) => { async { async_recursive!(25, $inner) }.await };
    (25, $inner:expr) => { async { async_recursive!(24, $inner) }.await };
    (24, $inner:expr) => { async { async_recursive!(23, $inner) }.await };
    (23, $inner:expr) => { async { async_recursive!(22, $inner) }.await };
    (22, $inner:expr) => { async { async_recursive!(21, $inner) }.await };
    (21, $inner:expr) => { async { async_recursive!(20, $inner) }.await };
    (20, $inner:expr) => { async { async_recursive!(19, $inner) }.await };

    (19, $inner:expr) => { async { async_recursive!(18, $inner) }.await };
    (18, $inner:expr) => { async { async_recursive!(17, $inner) }.await };
    (17, $inner:expr) => { async { async_recursive!(16, $inner) }.await };
    (16, $inner:expr) => { async { async_recursive!(15, $inner) }.await };
    (15, $inner:expr) => { async { async_recursive!(14, $inner) }.await };
    (14, $inner:expr) => { async { async_recursive!(13, $inner) }.await };
    (13, $inner:expr) => { async { async_recursive!(12, $inner) }.await };
    (12, $inner:expr) => { async { async_recursive!(11, $inner) }.await };
    (11, $inner:expr) => { async { async_recursive!(10, $inner) }.await };
    (10, $inner:expr) => { async { async_recursive!(9, $inner) }.await };

    (9, $inner:expr) => { async { async_recursive!(8, $inner) }.await };
    (8, $inner:expr) => { async { async_recursive!(7, $inner) }.await };
    (7, $inner:expr) => { async { async_recursive!(6, $inner) }.await };
    (6, $inner:expr) => { async { async_recursive!(5, $inner) }.await };
    (5, $inner:expr) => { async { async_recursive!(4, $inner) }.await };
    (4, $inner:expr) => { async { async_recursive!(3, $inner) }.await };
    (3, $inner:expr) => { async { async_recursive!(2, $inner) }.await };
    (2, $inner:expr) => { async { async_recursive!(1, $inner) }.await };
    (1, $inner:expr) => { async { async_recursive!(0, $inner) }.await };
    (0, $inner:expr) => { async { h19(&String::from("owo"), &0).await; $inner }.await };
}

async fn f() {
    async_recursive!(14, println!("hello"));
}

fn main() {
    let _ = f();
}

r? @eddyb requires a perf run.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2020
@lcnr
Copy link
Contributor Author

lcnr commented Sep 19, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Sep 19, 2020

⌛ Trying commit 1146c39 with merge 88f354df8a313170036daa0e4156f8e75242ebd6...

@Mark-Simulacrum
Copy link
Member

We'll want to trim it down so that it takes around a second at most on current nightly, but yes adding it definitely seems good to me.

@bors
Copy link
Contributor

bors commented Sep 19, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 88f354df8a313170036daa0e4156f8e75242ebd6 (88f354df8a313170036daa0e4156f8e75242ebd6)

@rust-timer
Copy link
Collaborator

Queued 88f354df8a313170036daa0e4156f8e75242ebd6 with parent 8e9d5db, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (88f354df8a313170036daa0e4156f8e75242ebd6): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@lcnr
Copy link
Contributor Author

lcnr commented Sep 19, 2020

looks mostly neutral, I guess this is ready for review

@eddyb
Copy link
Member

eddyb commented Sep 20, 2020

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Sep 20, 2020
@tmandry
Copy link
Member

tmandry commented Sep 22, 2020

Other than perf (which looks neutral to positive for existing cases), the only possible side effects I see here are

  • Lower recursion levels
  • Fewer duplicate obligations recorded

Which all seem like a good thing!

+1 to adding a case to rustc-perf. I don't think we have to wait on that to merge this, though.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 22, 2020

📌 Commit 1146c39 has been approved by tmandry

@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 Sep 22, 2020
@bors
Copy link
Contributor

bors commented Sep 22, 2020

⌛ Testing commit 1146c39 with merge 6d3acf5...

@bors
Copy link
Contributor

bors commented Sep 23, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: tmandry
Pushing 6d3acf5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 23, 2020
@bors bors merged commit 6d3acf5 into rust-lang:master Sep 23, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 23, 2020
@lcnr lcnr deleted the opaque-types-cache branch September 23, 2020 06:00
@Mark-Simulacrum Mark-Simulacrum added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 3, 2020
@Mark-Simulacrum
Copy link
Member

Along the lines of #72412 (comment), I am nominating this for beta backport. Would someone on the compiler team be willing to approve this? See rationale laid out in that comment.

@Mark-Simulacrum
Copy link
Member

@rust-lang/compiler -- cc on the previous comment, this needs to be beta-approved to go out in the release next week. See rationale in #72412 (comment).

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 3, 2020
…k-Simulacrum

[beta] backports

This backports a number of PRs to beta, not all of which have been approved (yet).

 * Switch to environment files to change the environment on GHA rust-lang#77418
 * cache types during normalization rust-lang#76928
 * Fixing memory exhaustion when formatting short code suggestion rust-lang#76598
 * Issue 72408 nested closures exponential rust-lang#72412

r? `@Mark-Simulacrum`
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.48.0, 1.47.0 Oct 3, 2020
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(lgtm)

let ty = ty.super_fold_with(self);
match *ty.kind() {
let res = (|| match *ty.kind() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this just for outlining?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is mostly so we don't accidentally return here. I kind of like this pattern myself but I do think that it can be somewhat counterintuitive

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using a counterintuitive pattern, please add a comment explaining it. :)

@Mark-Simulacrum Mark-Simulacrum added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Increasingly slow compilation as more levels of async are added in Rust 1.46
10 participants