Clean up and speed up inference variable resolving code - #160913
Clean up and speed up inference variable resolving code#160913jdonszelmann wants to merge 8 commits into
Conversation
|
@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.
Clean up and speed up resolving code
This comment has been minimized.
This comment has been minimized.
3dde145 to
b5fe24d
Compare
This comment has been minimized.
This comment has been minimized.
| match self.inner.borrow_mut().type_variables().probe(vid) { | ||
| TypeVariableValue::Known { value } => Ok(value), | ||
| match value { | ||
| TypeVariableValue::Known { value } => Ok(self.shallow_resolve_non_recursive(value)), |
There was a problem hiding this comment.
this method (and the one below) now also does the recursive resolving shallow_resolve already did. No tests change here.
| // Cold because the case in which a tyvar resolves to an intvar which resolves to a type is | ||
| // quite rare. It's way more common for `shallow_resolve_non_recursive` to return ty. | ||
| #[cold] | ||
| fn shallow_resolve_infer_non_recursive(&self, infer: InferTy, ty: Ty<'tcx>) -> Ty<'tcx> { |
There was a problem hiding this comment.
the non-recursive caase helps ~0.5% on local benchmarks. Not much, but still a bit.
| } | ||
|
|
||
| ty::Infer(ty::IntVar(vid)) => { | ||
| let nt = self.infcx.unwrap().opportunistic_resolve_int_var(vid); |
There was a problem hiding this comment.
renames all the opportunistic_* functions with simply shallow_resolve_*. I've done this in many places. I've added a lot of docs to all the resolve methods, which I think makes it super clear that resolving is always an opportunistic process that doesn't necessarily resolve all variables, simply because it can't always.
From the perspective of a new contributor, they'll see a resolve_* function for the first time, go to its docs, and learn that the purpose of all resolve_* methods is to opportunistically resolve variables.
Since the behavior is the same for all the resolve_* functions I think that will actually make things clearer than randomly calling some of them "opportunistic" even when the others are inherently also opportunistic.
| /// In cases where we do, this can aid performance. | ||
| #[inline(always)] | ||
| fn shallow_resolve_ty_var(&self, v: TyVid, ty: Ty<'tcx>) -> Ty<'tcx> { | ||
| fn shallow_resolve_ty_var_with_ty(&self, v: TyVid, ty: Option<Ty<'tcx>>) -> Ty<'tcx> { |
There was a problem hiding this comment.
By taking an Option here, we can merge more methods' implementations. Doing this has 0 performance overhead, #[inline(always)] makes sure the callsites that always call with Some get optimized properly.
There was a problem hiding this comment.
could we instead change this function to not take a ty and return and Option instead?
This comment has been minimized.
This comment has been minimized.
|
💔 Test for 7e8d4ec failed: CI. Failed job:
|
b5fe24d to
6b88c26
Compare
|
@bors try (failed due to missing rustdoc changes now applied) |
|
Unknown argument "(failed". Did you mean to use |
This comment has been minimized.
This comment has been minimized.
|
@bors try |
This comment has been minimized.
This comment has been minimized.
Clean up and speed up resolving code
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (d881e22): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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. @bors rollup=never rustc-perf 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 5.7%, secondary -0.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.5%, secondary 0.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: 463.191s -> 456.07s (-1.54%) |
This comment has been minimized.
This comment has been minimized.
|
that seems worth it :3 |
…er a shallow_resolve
6b88c26 to
06dd748
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. |
|
@rustbot review |
| Ty::new_int_var(self.tcx, inner.int_unification_table().find(vid)) | ||
| } | ||
| } | ||
| pub fn shallow_resolve_int_var(&self, vid: ty::IntVid) -> Ty<'tcx> { |
There was a problem hiding this comment.
if we return None if the inner function doesn't make progress, could we always be explicit about reconstructing the type in the caller to make it explicit where we (likely unnecessarily) do so?
| .borrow_mut() | ||
| .unwrap_region_constraints() | ||
| .opportunistic_resolve_var(canonicalizer.tcx, vid); | ||
| .shallow_resolve_region_var(canonicalizer.tcx, vid); |
There was a problem hiding this comment.
we don't reuse r here if the root doesn't change 🤔 feels like doing so would be good for perf 🤔
|
so we have the following now
|
View all comments
r? @lcnr
Clean up the
resolve*family of functions inInferCtxt, renaming various functions to have more consistent and descriptive names. Adds a lot of documentation, and even wins some performance using the fact that shallow_resolve now returns root vids.Reviewable commit by commit: some are large renames across the board, which are separated from the perf wins and small renames in other commits to hopefully make more sense.
Note
I've not used an LLM for any part of this PR, or any other PR I make. This includes any related work like research.