obligations_for_self_ty: skip irrelevant goals (recompute sub_root from stalled_vars)#156187
obligations_for_self_ty: skip irrelevant goals (recompute sub_root from stalled_vars)#156187inq wants to merge 4 commits intorust-lang:mainfrom
Conversation
|
changes to cc @lcnr Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
inq
force-pushed
the
obligations-self-ty-recompute-sub-root
branch
from
90c0853 to
596f7e9
Compare
| .filter(|(_, stalled_on)| { | ||
| let Some(stalled_on) = stalled_on else { return true }; | ||
| stalled_on.stalled_vars.iter().filter_map(|arg| arg.as_type()).any(|ty| { | ||
| matches!(*ty.kind(), ty::Infer(ty::TyVar(tv)) if infcx.sub_unification_table_root_var(tv) == vid) |
There was a problem hiding this comment.
please shallow_resolve the stalled_var here and if it is not an inference variable, always return true for now.
There was a problem hiding this comment.
also, please add a comment explaining that we are conservative here and want to handle cases where a goal is no longer stalled
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
…=<try> obligations_for_self_ty: skip irrelevant goals (recompute sub_root from stalled_vars)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
r? BoxyUwU given that it's largely my PR, let's give it to somebody else to review |
|
Finished benchmarking commit (1a8e5b7): 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 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 -67.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -25.7%)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: 495.518s -> 495.667s (0.03%) |
rustbot
removed
the
S-waiting-on-perf
6 participants
Supersedes #146759. Since the original PR is having conflict now, I've rebased.
This includes 2 commit
Why:
cached sub_roots can be stale if there is a sub-unification merge after stalling (lcnr proposed on the original PR)
so re-evaluating when reads ---> so more conservative
Testing:
-Zdisable-fast-pathsin trait solving #156172 with -Zdisable-fast-pathsStyle question:
In my preference, inside filter, I used filter_map - matches, but do you prefer if-let chain?
Original author: lcnr #146759
r? @lcnr