-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
some more rustc_borrowck cleanups #151374
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
base: main
Are you sure you want to change the base?
Conversation
| if let Err(_) = | ||
| self.eq_types(a, b, Locations::All(span), ConstraintCategory::BoringNoLocation) | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i assume removing this "fast path" is perf sensitive so will do a perf run 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like 0.2% in a few secondary benchmarks #151374 (comment)
I don't think a simple cleanup of yeeting this is worth it given those results?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you instead do
if infcx.next_Trait_solver() {
return eq_types();
}and then keep the old body with updated comments, rn the formatting is kinda ass here
| @@ -1,4 +1,7 @@ | |||
| //@ build-pass | |||
| //@ revisions: current next | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all of these are tests that fail in the old solver if you remove the normalization in equate_normalized_input_or_output
|
@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.
some more rustc_borrowck cleanups
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (97360bc): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @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.0%, secondary 5.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 473.058s -> 474.427s (0.29%) |
|
@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.
some more rustc_borrowck cleanups
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.
e322fbf to
f330bf8
Compare
f330bf8 to
df106b6
Compare
|
Finished benchmarking commit (e28fa5a): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary -2.0%, secondary 1.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 2.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 473.092s -> 474.982s (0.40%) |
| "there should be no incoming region assumptions = {pre_assumptions:#?}", | ||
| ); | ||
| { | ||
| // Scope these variables so it's clear they're not used later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seemed kinda scary to me when i was reading it :> but i think this would have made it not seem so scary
| ); | ||
| } | ||
|
|
||
| // Return types are a bit more complex. They may contain opaque `impl Trait` types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing something here? The actual logic here is no more complex than any of the previous relations we do.
| // Note: if we have to introduce new placeholders during normalization above, then we | ||
| // won't have added those universes to the universe info, which we would want in | ||
| // `relate_tys`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am missing something here. This comment would seem to imply we do things differently to avoid calling relate_types (note taht there is no such thing as relate_tys) but eq_types is a trivial wrapper around relate_tys.
I guess the point is that there will be universes made during the normalization call which this eq_types is unaware of the origin for? I don't quite get why this is worth calling out, but maybe it's worth keeping this :>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at minimum I think I'd want to move this comment to the actual normalize call as that's where the source of the "weirdness" is? but also we don't have a similar comment in other places we normalize and maybe we should if we do want this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also do not get this comment. I don't think it's relevant 🤔
| // | ||
| // I am not sure of a test case where this actually matters. There is a similar | ||
| // hack in `equate_inputs_and_outputs` which does have associated test cases. | ||
| let mir_ty = match self.infcx.tcx.next_trait_solver_globally() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| let mir_ty = match self.infcx.tcx.next_trait_solver_globally() { | |
| let mir_ty = match self.infcx.next_trait_solver() { |
| { | ||
| // Scope these variables so it's clear they're not used later | ||
| let pre_obligations = infcx.take_registered_region_obligations(); | ||
| assert!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why have these as variables at all, we could just do
assert_eq!(
infcx.take_registered_region_obligations(),
vec![],
"there should be no incoming region obligations"
);🤔 this assert is hard to read/noisy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oo smart
r? lcnr