Uh oh!
There was an error while loading. Please reload this page.
Compute generator saved locals on MIR - #101692
Conversation
jyn514
commented
Sep 11, 2022
I don't currently have time to review a PR this large but I think this work is super exciting, thank you for tackling it ❤️ |
ee1c144 to
2941ec7Compare
This comment has been minimized.
This comment has been minimized.
cjgillot
commented
Sep 11, 2022
@bors try @rust-timer queue |
rust-timer
commented
Sep 11, 2022
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
bors
commented
Sep 11, 2022
⌛ Trying commit 6c9ec2af2224dcbfba350ffdd69a27e5030000c1 with merge 8ea250bdb76176ecb0225baa34aa634b024ae0e2... |
This comment has been minimized.
This comment has been minimized.
bors
commented
Sep 11, 2022
☀️ Try build successful - checks-actions |
rust-timer
commented
Sep 11, 2022
Queued 8ea250bdb76176ecb0225baa34aa634b024ae0e2 with parent 0d56e34, future comparison URL. |
rust-timer
commented
Sep 12, 2022
Finished benchmarking commit (8ea250bdb76176ecb0225baa34aa634b024ae0e2): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Footnotes |
cjgillot
commented
Sep 12, 2022
@craterbot check |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
craterbot
commented
Sep 15, 2022
🎉 Experiment
|
36cbe4d to
f2506c0Compare
This comment has been minimized.
This comment has been minimized.
d39d4d0 to
b134e71Comparecjgillot
commented
Jan 27, 2023
I investigated those failures quite a long time ago, so I can't remember exactly. I'll have to perform a new crater check before stabilisation, so the issue will definitely reappear then. Simple rebase: |
bors
commented
Jan 27, 2023
bors
commented
Jan 28, 2023
bors
commented
Jan 28, 2023
☀️ Test successful - checks-actions |
1 similar comment
bors
commented
Jan 28, 2023
☀️ Test successful - checks-actions |
rust-timer
commented
Jan 28, 2023
Finished benchmarking commit (6cd6bad): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
cjgillot
commented
Jan 29, 2023
Mark-Simulacrum
commented
Jan 31, 2023
Thanks! That seems sufficient to me, marking as triaged. |
cjgillot
commented
Jan 31, 2023
#107443 has removed the remaining 1%. |
Generators are currently type-checked by introducing a
witnesstype variable, which is unified with aGeneratorWitness(captured types)whose purpose is to ensure that the auto traits correctly migrate from the captured types to thewitnesstype. This requires computing the captured types on HIR during type-checking, only to re-do it on MIR later.This PR proposes to drop the HIR-based computation, and only keep the MIR one. This is done in 3 steps.
witnesstype variable is never unified. This allows to stall all the obligations that depend on it until the end of type-checking. Then, the stalled obligations are marked as successful, and saved into the typeck results for later verification.witnessis replaced byGeneratorWitnessMIR(def_id, substs). From this point on, all trait selection involvingGeneratorWitnessMIRwill fetch the MIR-computed locals, similar to what opaque types do. There is no lifetime to be preserved here: we consider all the lifetimes appearing in this witness type to be higher-ranked.check_generator_obligationsquery. If any obligation was wrongly marked as fulfilled in step 1, it should be reported here.There are still many issues:
I am not too happy having to filter out some locals from the checked bounds, I think this is MIR building that introduces raw pointers polluting the analysis;solved by a check specific to static variables.cc @eholk@jyn514 for the drop-tracking work
r? @oli-obk as you warned me of potential unsoundness