Fix TypeOutlives fast-path - #163576
Fix TypeOutlives fast-path#163576
TypeOutlives fast-path#163576Conversation
|
@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.
Do not stall outlives goals on infer vars in fast path
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (ff34d4b): 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 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)This perf run didn't have relevant results for this metric. CyclesResults (primary -2.7%, secondary -20.1%)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: 487.961s -> 491.627s (0.75%) |
|
@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.
Do not stall outlives goals on infer vars in fast path
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (5b8f953): comparison URL. Overall result: ❌✅ regressions and improvements - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -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 (secondary -4.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 4.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.1%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 490.07s -> 491.114s (0.21%) |
94684a2 to
9270acb
Compare
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
TypeOutlives fast-path
|
this seems right, cool the remaining perf impact seems like noise @bors r+ rollup |
…=lcnr Fix `TypeOutlives` fast-path > Oh, the fast path is scuffed here. So, I feel fairly confident that we can encounter cases where stalled_on is wrong due to region vars without there being a bug, as in, your change is correct and desirable, but this specific test is just a bug in the fast path: > > `<MaybeVerify as Service<?infer>>::Future: 'static` should simply not be considered stalled. What's the cost of either entirely removing this trivially_stalled_on fast path or fixing it to bail when encountering non-rigid aliases > > This change is correct. Please separately do a PR to fix the type-outlives fastpath :blush: _Originally posted by @lcnr in rust-lang#162782 (comment) The first perf-run result is for always returning `Outcome::NoFastPath` on any infer var and the second one is for the current HEAD. We shouldn't stall the outlives goal if the goal contains a non-rigid alias even though the goal contains a non-region infer. Non-fast path will normalize that non-rigid alias and that make the evaluation progress, and I think in theory the fast path shouldn't make observable difference outside the solver. I'm not entirely sure on disabling fast path only in the presence of non-rigid opaques instead of disabling it entirely for non-region infer, but.. - It's no-less-correct than the status quo - It roughly matches the actual non-fast path: https://github.com/rust-lang/rust/blob/dba8825fe50879b22129271fb865944e384f7cce/compiler/rustc_next_trait_solver/src/solve/mod.rs#L128-L129 - The later has some perf impact hard to ignore for `typenum` I couldn't conjure up any case fixed by this PR other than the one in rust-lang#162782 😅 r? lcnr
…=lcnr Fix `TypeOutlives` fast-path > Oh, the fast path is scuffed here. So, I feel fairly confident that we can encounter cases where stalled_on is wrong due to region vars without there being a bug, as in, your change is correct and desirable, but this specific test is just a bug in the fast path: > > `<MaybeVerify as Service<?infer>>::Future: 'static` should simply not be considered stalled. What's the cost of either entirely removing this trivially_stalled_on fast path or fixing it to bail when encountering non-rigid aliases > > This change is correct. Please separately do a PR to fix the type-outlives fastpath :blush: _Originally posted by @lcnr in rust-lang#162782 (comment) The first perf-run result is for always returning `Outcome::NoFastPath` on any infer var and the second one is for the current HEAD. We shouldn't stall the outlives goal if the goal contains a non-rigid alias even though the goal contains a non-region infer. Non-fast path will normalize that non-rigid alias and that make the evaluation progress, and I think in theory the fast path shouldn't make observable difference outside the solver. I'm not entirely sure on disabling fast path only in the presence of non-rigid opaques instead of disabling it entirely for non-region infer, but.. - It's no-less-correct than the status quo - It roughly matches the actual non-fast path: https://github.com/rust-lang/rust/blob/dba8825fe50879b22129271fb865944e384f7cce/compiler/rustc_next_trait_solver/src/solve/mod.rs#L128-L129 - The later has some perf impact hard to ignore for `typenum` I couldn't conjure up any case fixed by this PR other than the one in rust-lang#162782 😅 r? lcnr
…uwer Rollup of 13 pull requests Successful merges: - #163317 (Fix incremental compilation for fat LTO) - #163582 (Reapply "bootstrap: Enable rustdoc mergeable CCI for std and internal docs") - #151793 (Add mul_add_relaxed methods for floating-point types) - #162782 (Fix rustdoc ICE caused by mishandling of ambiguity errors) - #163010 (Miri can do dirfd now) - #163535 (Improve `DocStrings` perf) - #163576 (Fix `TypeOutlives` fast-path) - #163587 (Several small span improvements) - #163612 (fix `ValidateBoundVars`) - #163632 (bump rustc-build-sysroot) - #163635 (Revert note about signum of NaN) - #163644 (Add mailmap entry) - #163651 (Remove variants from `feature-gate-autodiff-use` test)
…uwer Rollup of 14 pull requests Successful merges: - #163582 (Reapply "bootstrap: Enable rustdoc mergeable CCI for std and internal docs") - #151793 (Add mul_add_relaxed methods for floating-point types) - #162782 (Fix rustdoc ICE caused by mishandling of ambiguity errors) - #163010 (Miri can do dirfd now) - #163535 (Improve `DocStrings` perf) - #163576 (Fix `TypeOutlives` fast-path) - #163587 (Several small span improvements) - #163603 (Reorganise reflection intrinsics) - #163612 (fix `ValidateBoundVars`) - #163632 (bump rustc-build-sysroot) - #163635 (Revert note about signum of NaN) - #163644 (Add mailmap entry) - #163647 (Rename `rustc_driver::run_compiler` to `compiler_entrypoint`) - #163651 (Remove variants from `feature-gate-autodiff-use` test)
Rollup merge of #163576 - ShoyuVanilla:outlives-fast-path, r=lcnr Fix `TypeOutlives` fast-path > Oh, the fast path is scuffed here. So, I feel fairly confident that we can encounter cases where stalled_on is wrong due to region vars without there being a bug, as in, your change is correct and desirable, but this specific test is just a bug in the fast path: > > `<MaybeVerify as Service<?infer>>::Future: 'static` should simply not be considered stalled. What's the cost of either entirely removing this trivially_stalled_on fast path or fixing it to bail when encountering non-rigid aliases > > This change is correct. Please separately do a PR to fix the type-outlives fastpath :blush: _Originally posted by @lcnr in #162782 (comment) The first perf-run result is for always returning `Outcome::NoFastPath` on any infer var and the second one is for the current HEAD. We shouldn't stall the outlives goal if the goal contains a non-rigid alias even though the goal contains a non-region infer. Non-fast path will normalize that non-rigid alias and that make the evaluation progress, and I think in theory the fast path shouldn't make observable difference outside the solver. I'm not entirely sure on disabling fast path only in the presence of non-rigid opaques instead of disabling it entirely for non-region infer, but.. - It's no-less-correct than the status quo - It roughly matches the actual non-fast path: https://github.com/rust-lang/rust/blob/dba8825fe50879b22129271fb865944e384f7cce/compiler/rustc_next_trait_solver/src/solve/mod.rs#L128-L129 - The later has some perf impact hard to ignore for `typenum` I couldn't conjure up any case fixed by this PR other than the one in #162782 😅 r? lcnr
…uwer Rollup of 14 pull requests Successful merges: - rust-lang/rust#163582 (Reapply "bootstrap: Enable rustdoc mergeable CCI for std and internal docs") - rust-lang/rust#151793 (Add mul_add_relaxed methods for floating-point types) - rust-lang/rust#162782 (Fix rustdoc ICE caused by mishandling of ambiguity errors) - rust-lang/rust#163010 (Miri can do dirfd now) - rust-lang/rust#163535 (Improve `DocStrings` perf) - rust-lang/rust#163576 (Fix `TypeOutlives` fast-path) - rust-lang/rust#163587 (Several small span improvements) - rust-lang/rust#163603 (Reorganise reflection intrinsics) - rust-lang/rust#163612 (fix `ValidateBoundVars`) - rust-lang/rust#163632 (bump rustc-build-sysroot) - rust-lang/rust#163635 (Revert note about signum of NaN) - rust-lang/rust#163644 (Add mailmap entry) - rust-lang/rust#163647 (Rename `rustc_driver::run_compiler` to `compiler_entrypoint`) - rust-lang/rust#163651 (Remove variants from `feature-gate-autodiff-use` test)
Originally posted by @lcnr in #162782 (comment)
The first perf-run result is for always returning
Outcome::NoFastPathon any infer var and the second one is for the current HEAD.We shouldn't stall the outlives goal if the goal contains a non-rigid alias even though the goal contains a non-region infer. Non-fast path will normalize that non-rigid alias and that make the evaluation progress, and I think in theory the fast path shouldn't make observable difference outside the solver.
I'm not entirely sure on disabling fast path only in the presence of non-rigid opaques instead of disabling it entirely for non-region infer, but..
rust/compiler/rustc_next_trait_solver/src/solve/mod.rs
Lines 128 to 129 in dba8825
typenumI couldn't conjure up any case fixed by this PR other than the one in #162782 😅
r? lcnr