remove more trivial regions in evaluate_added_goals_and_make_canonical_response - #162032
Conversation
|
Fun fact: the number of outlives constraints grows exactly as fib(2 * depth) :D |
This comment has been minimized.
This comment has been minimized.
0125479 to
bc71d10
Compare
|
@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.
remove more trivial regions in evaluate_added_goals_and_make_canonical_response
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (0be4976): comparison URL. Overall result: no relevant changes - 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 countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)This perf run didn't have relevant results for this metric. CyclesThis perf run didn't have relevant results for this metric. Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: missing data |
|
(Sorry, a rustc-perf failure, looking into it) |
|
Would rerunning with the same artifact make sense? |
|
Yup, I deleted the result from the DB, and will rerun the artifact for you (once the underlying bug is fixed and deployed, which should happen in ~20 minutes). |
Huge thanks! 😄 |
|
@rust-timer build 0be4976 |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (0be4976): comparison URL. Overall result: no relevant changes - 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 countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (primary -1.8%, secondary -2.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 477.635s -> 477.178s (-0.10%) |
bc71d10 to
33ac2c4
Compare
This comment has been minimized.
This comment has been minimized.
33ac2c4 to
3278ea3
Compare
…ke_canonical_response
3278ea3 to
e9ab50c
Compare
|
@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.
remove more trivial regions in evaluate_added_goals_and_make_canonical_response
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (1ca580b): comparison URL. Overall result: no relevant changes - 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 countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (primary -4.0%, secondary 7.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 3.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: 476.686s -> 484.632s (1.67%) |
There was a problem hiding this comment.
Though there's no noticable change in perf, I think having less trivial bounds in the query output is generally good and the other points stated in the PR description make sense.
@bors r+
|
⌛ Testing commit e9ab50c with merge 633abdc... Workflow: https://github.com/rust-lang/rust/actions/runs/34023044567 |
remove more trivial regions in evaluate_added_goals_and_make_canonical_response cc #161575. I don't think it *fixes* the issue per se, since the most principled fix would be to just deny unconstrained lifetime args like we do for types and consts already. At the very least, though, it Makes Things Go Faster. In the example from #161575 (comment), each `'unconstrained` appears only once in the entire response: in the rhs of the `'a: 'unconstrained#N` bound. Since they are mentioned nowhere else and are created only when proving our own nested goals, these outlives constraints are all satisfiable by setting `'unconstrained := 'empty`, which tells us nothing about `'a`. Therefore, (I think) that makes it safe to treat all of these constraints as trivial and drop them entirely, drop all of these requirements entirely, similar to what we already do with reflexive or duplicate region constraints. In other words, if a `ReVar` appears only once in the entire response, and that place is the rhs of an outlives constraint, then it is safe to drop that constraint. I'm a little worried about the perf impact of the visitor on "normal" code, but fwiw even a 100-deep nested version of the reproducer compiles in about 0.05s on my machine. r? lcnr
|
@bors yield |
|
Auto build was cancelled. Cancelled workflows: The next pull request likely to be tested is #162362. |
…uwer Rollup of 4 pull requests Successful merges: - #162032 (remove more trivial regions in evaluate_added_goals_and_make_canonical_response) - #161607 (Add #[track_caller] to signed integer div/rem methods) - #162339 (Revert "bootstrap: Enable rustdoc mergeable CCI for std and internal docs #161716") - #162158 (Revise documentation of `NumBuffer` and `format_into()`.)
Rollup merge of #162032 - sjwang05:more-region-dedup, r=ShoyuVanilla remove more trivial regions in evaluate_added_goals_and_make_canonical_response cc #161575. I don't think it *fixes* the issue per se, since the most principled fix would be to just deny unconstrained lifetime args like we do for types and consts already. At the very least, though, it Makes Things Go Faster. In the example from #161575 (comment), each `'unconstrained` appears only once in the entire response: in the rhs of the `'a: 'unconstrained#N` bound. Since they are mentioned nowhere else and are created only when proving our own nested goals, these outlives constraints are all satisfiable by setting `'unconstrained := 'empty`, which tells us nothing about `'a`. Therefore, (I think) that makes it safe to treat all of these constraints as trivial and drop them entirely, drop all of these requirements entirely, similar to what we already do with reflexive or duplicate region constraints. In other words, if a `ReVar` appears only once in the entire response, and that place is the rhs of an outlives constraint, then it is safe to drop that constraint. I'm a little worried about the perf impact of the visitor on "normal" code, but fwiw even a 100-deep nested version of the reproducer compiles in about 0.05s on my machine. r? lcnr
View all comments
cc #161575. I don't think it fixes the issue per se, since the most principled fix would be to just deny unconstrained lifetime args like we do for types and consts already. At the very least, though, it Makes Things Go Faster.
In the example from #161575 (comment), each
'unconstrainedappears only once in the entire response: in the rhs of the'a: 'unconstrained#Nbound. Since they are mentioned nowhere else and are created only when proving our own nested goals, these outlives constraints are all satisfiable by setting'unconstrained := 'empty, which tells us nothing about'a. Therefore, (I think) that makes it safe to treat all of these constraints as trivial and drop them entirely, drop all of these requirements entirely, similar to what we already do with reflexive or duplicate region constraints. In other words, if aReVarappears only once in the entire response, and that place is the rhs of an outlives constraint, then it is safe to drop that constraint.I'm a little worried about the perf impact of the visitor on "normal" code, but fwiw even a 100-deep nested version of the reproducer compiles in about 0.05s on my machine.
r? lcnr