Repository navigation
Clean up and speed up inference variable resolving code - #160913
jdonszelmann wants to merge 5 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.
| /// 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.
| ty::TyVar(_) => { | ||
| unreachable!() | ||
| } |
|
talked about this in person @rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
3cbad4a to
218b4f5
Compare
This comment has been minimized.
This comment has been minimized.
218b4f5 to
f31f698
Compare
This comment has been minimized.
This comment has been minimized.
|
In-person, @lcnr and I discussed that the |
This comment has been minimized.
This comment has been minimized.
…er a shallow_resolve
f31f698 to
dc4ea60
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. |
|
The job Click to see the possible cause of the failure (guessed by this bot)Important For more information how to resolve CI failures of this job, visit this link. |
|
r? project-trait-system-refactor (lcnr has told me in-person they're not available for review for a while) |
|
@rustbot review |
|
@rustbot author |
View all comments
r? @lcnr
As part of #162126, I found some small performance wins resulting from the fact that
shallow_resolvenow returns root vids.The last commit also makes it so calls to
shallow_resolveneed to explicitly handle the possibility of reusing the existing interned value.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.