Uh oh!
There was an error while loading. Please reload this page.
Refactor query forcing - #78780
Conversation
rust-highfive
commented
Nov 5, 2020
r? @lcnr (rust_highfive has picked a reviewer for you, use r? to override) |
jyn514
commented
Nov 5, 2020
@cjgillot does this need a perf run? |
cjgillot
commented
Nov 5, 2020
Should be perf-neutral, but a perf run can't hurt. |
jyn514
commented
Nov 5, 2020
Let's hold off then, it will get a perf run when it's merged and whoever does perf triage will see it if it's significant. |
Mark-Simulacrum
commented
Nov 5, 2020
Queries are generally quite hot, and it won't hurt - @bors try @rust-timer queue |
rust-timer
commented
Nov 5, 2020
Awaiting bors try build completion |
bors
commented
Nov 5, 2020
⌛ Trying commit 7dc39e324baf46e4990667269aa4dacd5655fd5f with merge bfb3e3391674a1abc7127c7552b0afd98fcc000f... |
bors
commented
Nov 5, 2020
☀️ Try build successful - checks-actions |
rust-timer
commented
Nov 5, 2020
Queued bfb3e3391674a1abc7127c7552b0afd98fcc000f with parent b1d9f31, future comparison URL. |
rust-timer
commented
Nov 5, 2020
Finished benchmarking try commit (bfb3e3391674a1abc7127c7552b0afd98fcc000f): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
JohnCSimon
commented
Dec 7, 2020
ping from triage |
bjorn3
commented
Dec 7, 2020
@cjgillot is the PR author, not me. Also all my review comments have been resolved. |
bors
commented
Dec 9, 2020
☔ The latest upstream changes (presumably #79721) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels: |
jyn514
commented
Dec 9, 2020
ping @lcnr - are you a good reviewer for this? If not, can you suggest someone, maybe @wesleywiser? |
lcnr
commented
Dec 9, 2020
yeah, would prefer to hand this off 😅 this ended up slipping under my radar, sry r? @wesleywiser feel free to reassign if you also can't review this rn |
wesleywiser
commented
Dec 15, 2020
Apologies, it's been a busy week for me and I haven't had time to look at this. It will probably be a few more days before I can take an in-depth look so if you don't want to wait that long, please feel free to re-assign; I don't want to hold this up 🙂 I do see some regressions in the performance data. Would it be feasible to try to narrow down which changes are causing that? |
bors
commented
Dec 22, 2020
☔ The latest upstream changes (presumably #80177) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels: |
cjgillot
commented
Aug 24, 2021
@bors try @rust-timer queue |
rust-timer
commented
Aug 24, 2021
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
bors
commented
Aug 24, 2021
⌛ Trying commit 31330bf with merge 95378b6041ecf80226a5ce93362511521c868f20... |
bors
commented
Aug 24, 2021
☀️ Try build successful - checks-actions |
rust-timer
commented
Aug 24, 2021
Queued 95378b6041ecf80226a5ce93362511521c868f20 with parent 47ab5f7, future comparison URL. |
rust-timer
commented
Aug 24, 2021
Finished benchmarking try commit (95378b6041ecf80226a5ce93362511521c868f20): comparison url. Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking 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 led to changes in compiler perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @bors rollup=never |
cjgillot
commented
Aug 25, 2021
Last perf run shows no runtime modification, and -8% on rustc_query_impl compile time. |
Mark-Simulacrum
commented
Sep 11, 2021
Thanks! I looked over each commit, and while the code is definitely still not easily readable I think this is moving us in the right direction for sure, great work. Bootstrap time improvement is also nice to see. I also think that the perf-regression label can be considered acceptable - the vast majority of benchmarks are showing slight improvements, with only a few of the smaller benchmarks showing regressions (helloworld, await-call-tree), and those are quite small too. @rustbot label: +perf-regression-triaged @bors r+ |
Mark-Simulacrum
commented
Sep 11, 2021
@bors r+ (I think bors missed my earlier comment...) |
Mark-Simulacrum
commented
Sep 11, 2021
@bors r+ |
bors
commented
Sep 11, 2021
📌 Commit 31330bf has been approved by |
Mark-Simulacrum
commented
Sep 11, 2021
@bors rollup=never |
bors
commented
Sep 11, 2021
bors
commented
Sep 11, 2021
☀️ Test successful - checks-actions |
rust-timer
commented
Sep 12, 2021
Finished benchmarking commit (8c2b6ea): comparison url. Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression |
Mark-Simulacrum
commented
Sep 14, 2021
@rustbot label: +perf-regression-triaged The results are as expected before merging, with the majority being slightly positive, and the negative results limited to coercions-debug (typically noisy, though this may be a real data point) and helloworld (small, not very representative of normal code). Generally speaking, it seems like the improvements in bootstrap time and code quality are sufficient to accept these small regressions (and overall mixed results, given the wide swath of small improvements). |
The control flow in those functions was very complex, with several layers of continuations.
I tried to simplify the implementation, while keeping essentially the same logic.
Now, all code paths go through
try_execute_queryfor the actual query execution.Communication with the
dep_graphand the live caches are the only difference between query getting/ensuring/forcing.