-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Only compute specializes
query if (min)specialization is enabled in the crate of the specializing impl
#126139
Conversation
…ate of the specialized impl
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Only compute `specializes` query if (min)specialization is enabled in the crate of the specializing impl Fixes (after backport) rust-lang#125197 ### What rust-lang#122791 makes it so that inductive cycles are no longer hard errors. That means that when we are testing, for example, whether these impls overlap: ```rust impl PartialEq<Self> for AnyId { fn eq(&self, _: &Self) -> bool { todo!() } } impl<T: Identifier> PartialEq<T> for AnyId { fn eq(&self, _: &T) -> bool { todo!() } } ``` ...given... ```rust pub trait Identifier: Display + 'static {} impl<T> Identifier for T where T: PartialEq + Display + 'static {} ``` Then we try to see if the second impl holds given `T = AnyId`. That requires `AnyId: Identifier`, which requires that `AnyId: PartialEq`, which is satisfied by these two impl candidates... The `PartialEq<T>` impl is a cycle, and we used to winnow it when we used to treat inductive cycles as errors. However, now that we don't winnow it, this means that we *now* try calling `candidate_should_be_dropped_in_favor_of`, which tries to check whether one of the impls specializes the other: the `specializes` query. In that query, we currently bail early if the impl is local: However, in a foreign crate, we try to compute if the two impls specialize each other by doing trait solving. This may itself lead to the same situation where we call `specializes`, which will lead to a query cycle. ### How does this fix the problem We now record whether specialization is enabled in foreign crates, and extend this early-return behavior to foreign impls too. This means that we can only encounter these cycles if we truly have a specializing impl from a crate with specialization enabled. ----- r? `@oli-obk` or `@lcnr`
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (20182a9): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResults (secondary 0.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: missing data |
@bors r+ |
As 1.79 artifacts are being prepared right now, this PR could be stable-nominated in the future. Maybe we'll just keep a look at open issues to see whether people encounter #125197 or similar in the wild. There wasn't many crater regressions (nor did it seem issues were opened about this on nightly?), and maybe a stable backport won't be needed. |
☀️ Test successful - checks-actions |
Finished benchmarking commit (336e6ab): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (secondary -4.8%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (secondary 0.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 677.913s -> 675.77s (-0.32%) |
Also suitable for a stable backport in case a point release is planned. @rustbot label +stable-accepted |
Adding the nomination so it comes up in our queries. @rustbot label +stable-nominated |
[beta] backports - Only compute `specializes` query if (min)specialization is enabled in the crate of the specializing impl rust-lang#126139 - Add pub struct with allow(dead_code) into worklist rust-lang#126315 - ci: Update centos:7 to use vault repos rust-lang#126352 r? cuviper
Clearing stable backport labels since we didn't ship a 1.79 point release, and this is already in current stable 1.80.0. @rustbot label -stable-nominated -stable-accepted |
Fixes (after backport) #125197
What
#122791 makes it so that inductive cycles are no longer hard errors. That means that when we are testing, for example, whether these impls overlap:
...given...
Then we try to see if the second impl holds given
T = AnyId
. That requiresAnyId: Identifier
, which requires thatAnyId: PartialEq
, which is satisfied by these two impl candidates... ThePartialEq<T>
impl is a cycle, and we used to winnow it when we used to treat inductive cycles as errors.However, now that we don't winnow it, this means that we now try calling
candidate_should_be_dropped_in_favor_of
, which tries to check whether one of the impls specializes the other: thespecializes
query. In that query, we currently bail early if the impl is local.However, in a foreign crate, we try to compute if the two impls specialize each other by doing trait solving. This may itself lead to the same situation where we call
specializes
, which will lead to a query cycle.How does this fix the problem
We now record whether specialization is enabled in foreign crates, and extend this early-return behavior to foreign impls too. This means that we can only encounter these cycles if we truly have a specializing impl from a crate with specialization enabled.
r? @oli-obk or @lcnr