-Znext-solver Less normalizes-to janks#156619
Conversation
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor changes to cc @lcnr This PR modifies |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
`-Znext-solver` Less normalizes-to janks
|
I haven't done the nested goals' recursion depth part yet 😅 I'm gonna work on it if the overall design here pan out to be feasible |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (d4c528b): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.5%, secondary 8.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 26.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 510.626s -> 509.733s (-0.17%) |
|
Oh, the perf regressions are insane 😅 I'll look into them |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
`-Znext-solver` Less normalizes-to janks
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (52311d2): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.7%, secondary 8.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.3%, secondary 22.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 513.812s -> 509.975s (-0.75%) |
6bd6498 to
5927eca
Compare
This comment has been minimized.
This comment has been minimized.
|
If my observation is correct, this would fix the perf regression 🤔 |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (bf09da4): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.4%, secondary 6.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 3.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 514.669s -> 514.548s (-0.02%) |
|
@lcnr I've done available depth hacks as well. Could this be reviewed or would you like me to completely remove all the remaining |
This comment has been minimized.
This comment has been minimized.
5c1e975 to
44d974b
Compare
This comment has been minimized.
This comment has been minimized.
|
Re-measuring perf regressions on top of #156187 @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
`-Znext-solver` Less normalizes-to janks
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (4d1f2a6): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary -1.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -4.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 523.273s -> 517.628s (-1.08%) |
| /// Because of this we return any ambiguous nested goals from `NormalizesTo` to the | ||
| /// caller when then adds these to its own context. The caller is always an `AliasRelate` | ||
| /// caller when then adds these to its own context. The caller is always an `Projection` | ||
| /// goal so this never leaks out of the solver. |
There was a problem hiding this comment.
can you more completely rewrite this comment and also state that NormalizesTo is just an implementation detail of Projection for associated terms
I feel like NormalizesTois just somewhat of a bad name xx
It should be something like
ProjectionComputeAssocTermCandidate or NormalizeAssocTermInternal or whatever :>
There was a problem hiding this comment.
Sure. Would you like to change both this variant and the NormalizesTo goal's names into one of those more proper ones?
There was a problem hiding this comment.
I just changed the name of CurrentGoalKind::NormalizesTo for now and left PredicateKind::NormalizesTo as a FIXME for now 😅
| self.normalization_goal_source, | ||
| Goal::new(self.cx(), self.param_env, normalizes_to), | ||
| Goal::new(self.cx(), self.param_env, projection), | ||
| false, |
There was a problem hiding this comment.
hmm, I guess it's fine for now. Can you add a FIXME that this whole folder should be removed in favor of proper normalization, cc #156742
| normalizes_to, | ||
| None, | ||
| IncreaseDepthForNested::No, | ||
| )?; |
There was a problem hiding this comment.
comments please :3
- why do we use a separate goal only for associated terms
- why do we not increase the depth for nested goals
- edge case that we don't reevaluate the
NormalizesToif equating theunconstrained_termafterwards reesults in inference constraints in the impl headerl, and how that should be fine as we will reevaluate the outerProjectiongoal in that case
There was a problem hiding this comment.
I added comments but I'm not totally sure my understandings on those questions are correct 😅
We had never stepped into recursive replacement with `AliasRelate` goals when replacing aliases because `AliasRelate` goals don't allow normalization
44d974b to
ed986a6
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
ed986a6 to
1dad181
Compare
1dad181 to
8cdfcda
Compare
View all comments
cc rust-lang/trait-system-refactor-initiative#223 (comment)
r? lcnr