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

Test polymorphic_recursion.rs takes a long time to compile with a debug-assertions=true compiler #121121

Open
Zalathar opened this issue Feb 15, 2024 · 4 comments
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-mir-opt Area: MIR optimizations A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@Zalathar
Copy link
Contributor

#120912 reported that tests/mir-opt/inline/polymorphic_recursion.rs became very slow after #120584. A workaround for that slowness was added by a hack in #120942.

However, the test is still very slow when the compiler is built with [rust] debug-assertions=true.

On my system (Apple M2 Pro):

  • Before the original change, it was fast enough that I never noticed it being slow when running x test mir-opt.
  • After the original change but before the hack, it appeared to hang indefinitely (though perhaps it was just running very very slowly).
  • After the hack, it still takes about 45 seconds to complete.
// skip-filecheck
// Make sure that the MIR inliner does not loop indefinitely on polymorphic recursion.
// compile-flags: --crate-type lib

// Randomize `def_path_hash` by defining them under a module with different names
macro_rules! emit {
    ($($m:ident)*) => {$(
        pub mod $m {
            pub trait Tr { type Next: Tr; }

            pub fn hoge<const N: usize, T: Tr>() {
                inner::<N, T>();
            }

            #[inline(always)]
            fn inner<const N: usize, T: Tr>()
            {
                inner::<N, T::Next>();
                inner::<N, T::Next>();
            }
        }
    )*};
}

// Increase the chance of triggering the bug
emit!(m00 m01 m02 m03 m04 m05 m06 m07 m08 m09 m10 m11 m12 m13 m14 m15 m16 m17 m18 m19);
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 15, 2024
@Zalathar
Copy link
Contributor Author

I'm not sure what should be done here.

Obviously I can work around this issue, by just avoiding that specific test.

But I don't like the idea of just leaving this test lying around in its current state, if it becomes catastrophically slow in an otherwise-reasonable (dev) build configuration.

@compiler-errors
Copy link
Member

:(

I wonder why it still takes so long. Perhaps it's just because of extremely deep type substitution? Let me try a few more hacks, I guess.

@jieyouxu jieyouxu added A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-mir-opt Area: MIR optimizations A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself T-types Relevant to the types team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 15, 2024
@compiler-errors
Copy link
Member

compiler-errors commented Feb 15, 2024

#121123 will help; let's see if i can get that into a state that's landable though.

bors added a commit to rust-lang-ci/rust that referenced this issue Mar 21, 2024
…oli-obk

Split an item bounds and an item's super predicates

This is the moral equivalent of rust-lang#107614, but instead for predicates this applies to **item bounds**. This PR splits out the item bounds (i.e. *all* predicates that are assumed to hold for the alias) from the item *super predicates*, which are the subset of item bounds which share the same self type as the alias.

## Why?

Much like rust-lang#107614, there are places in the compiler where we *only* care about super-predicates, and considering predicates that possibly don't have anything to do with the alias is problematic. This includes things like closure signature inference (which is at its core searching for `Self: Fn(..)` style bounds), but also lints like `#[must_use]`, error reporting for aliases, computing type outlives predicates.

Even in cases where considering all of the `item_bounds` doesn't lead to bugs, unnecessarily considering irrelevant bounds does lead to a regression (rust-lang#121121) due to doing extra work in the solver.

## Example 1 - Trait Aliases

This is best explored via an example:

```
type TAIT<T> = impl TraitAlias<T>;

trait TraitAlias<T> = A + B where T: C;
```

The item bounds list for `Tait<T>` will include:
* `Tait<T>: A`
* `Tait<T>: B`
* `T: C`

While `item_super_predicates` query will include just the first two predicates.

Side-note: You may wonder why `T: C` is included in the item bounds for `TAIT`? This is because when we elaborate `TraitAlias<T>`, we will also elaborate all the predicates on the trait.

## Example 2 - Associated Type Bounds

```
type TAIT<T> = impl Iterator<Item: A>;
```

The `item_bounds` list for `TAIT<T>` will include:
* `Tait<T>: Iterator`
* `<Tait<T> as Iterator>::Item: A`

But the `item_super_predicates` will just include the first bound, since that's the only bound that is relevant to the *alias* itself.

## So what

This leads to some diagnostics duplication just like rust-lang#107614, but none of it will be user-facing. We only see it in the UI test suite because we explicitly disable diagnostic deduplication.

Regarding naming, I went with `super_predicates` kind of arbitrarily; this can easily be changed, but I'd consider better names as long as we don't block this PR in perpetuity.
@Zalathar
Copy link
Contributor Author

As of #121123 this test now takes about 4s for me, which is a lot less than 45 seconds, but still several times longer than the rest of mir-opt put together.

flip1995 pushed a commit to flip1995/rust that referenced this issue Apr 4, 2024
…oli-obk

Split an item bounds and an item's super predicates

This is the moral equivalent of rust-lang#107614, but instead for predicates this applies to **item bounds**. This PR splits out the item bounds (i.e. *all* predicates that are assumed to hold for the alias) from the item *super predicates*, which are the subset of item bounds which share the same self type as the alias.

## Why?

Much like rust-lang#107614, there are places in the compiler where we *only* care about super-predicates, and considering predicates that possibly don't have anything to do with the alias is problematic. This includes things like closure signature inference (which is at its core searching for `Self: Fn(..)` style bounds), but also lints like `#[must_use]`, error reporting for aliases, computing type outlives predicates.

Even in cases where considering all of the `item_bounds` doesn't lead to bugs, unnecessarily considering irrelevant bounds does lead to a regression (rust-lang#121121) due to doing extra work in the solver.

## Example 1 - Trait Aliases

This is best explored via an example:

```
type TAIT<T> = impl TraitAlias<T>;

trait TraitAlias<T> = A + B where T: C;
```

The item bounds list for `Tait<T>` will include:
* `Tait<T>: A`
* `Tait<T>: B`
* `T: C`

While `item_super_predicates` query will include just the first two predicates.

Side-note: You may wonder why `T: C` is included in the item bounds for `TAIT`? This is because when we elaborate `TraitAlias<T>`, we will also elaborate all the predicates on the trait.

## Example 2 - Associated Type Bounds

```
type TAIT<T> = impl Iterator<Item: A>;
```

The `item_bounds` list for `TAIT<T>` will include:
* `Tait<T>: Iterator`
* `<Tait<T> as Iterator>::Item: A`

But the `item_super_predicates` will just include the first bound, since that's the only bound that is relevant to the *alias* itself.

## So what

This leads to some diagnostics duplication just like rust-lang#107614, but none of it will be user-facing. We only see it in the UI test suite because we explicitly disable diagnostic deduplication.

Regarding naming, I went with `super_predicates` kind of arbitrarily; this can easily be changed, but I'd consider better names as long as we don't block this PR in perpetuity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-mir-opt Area: MIR optimizations A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants