AliasTerm refactor#155392
Conversation
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy changes to the core type system cc @lcnr HIR ty lowering was modified cc @fmease Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor changes to the core type system cc @lcnr This PR changes rustc_public cc @oli-obk, @celinval, @ouz-a, @makai410 Some changes occurred in compiler/rustc_sanitizers cc @rcvalle |
|
r? BoxyUwU |
|
a bit concerned about PredicateKind size increase, let's do a perf to make sure it's fine @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.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (8206b39): 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 0.5%, secondary 0.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -0.5%)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: 491.618s -> 491.67s (0.01%) |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
I feel a bit unsure about the with_args method, i feel like it makes the callsites which are obligation.predicate.with_args much less clear what's going on. but I guess thats a more general issue that projection predicates have really confusingly fields.
this PR needs a rebase and can you link to the tracking issue in the description. thx
3112204 to
54abf6d
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. |
This comment has been minimized.
This comment has been minimized.
|
Rebased & added the tracking issue to the PR description. |
|
@bors r+ rollup=never p=1 (bitrotty) |
|
threading between rollups @bors p=6 |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing f9988fe (parent) -> 1bfcb28 (this PR) Test differencesShow 23 test diffs23 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 1bfcb284f7a2199ad322daa463e29e708d5bc635 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (1bfcb28): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Our benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression 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 0.8%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeResults (secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 490.294s -> 491.717s (0.29%) |
|
@WaffleLapkin / @BoxyUwU it looks like there were some concerns over the size increases (#155392 (comment)), it looks like those have born out in both pre-merge and after-merge results. Do we think the refactor here is worth the slight regressions in instruction counts? I think it's reasonable to accept them given the lack of cycle counts. |
| goal.param_env, | ||
| ty::UnevaluatedConst::new( | ||
| goal.predicate.alias.def_id.try_into().unwrap(), | ||
| goal.predicate.alias.def_id().try_into().unwrap(), |
There was a problem hiding this comment.
we should pass in the DefId from the match where we decide to call normalize_anon_const here? 🤔
| self.add_goals( | ||
| GoalSource::Misc, | ||
| cx.predicates_of(free_alias.def_id) | ||
| cx.predicates_of(free_alias.def_id()) |
There was a problem hiding this comment.
same here
| cx.type_of(inherent.def_id()).instantiate(cx, inherent_args).skip_norm_wip().into() | ||
| } else { | ||
| cx.const_of_item(inherent.def_id).instantiate(cx, inherent_args).skip_norm_wip().into() | ||
| cx.const_of_item(inherent.def_id()) |
There was a problem hiding this comment.
and here, I guess in general, I feel like most calls of AliasXXX::def_id are meh
There was a problem hiding this comment.
i would expect in the long run we just get rid of this method since not everything will have a DefId anymore
It would probably be good to look into if we can reduce the size of |
View all comments
follow up to #154758
tracking issue: #154941