Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move OnDiskCache to rustc_query_impl. #86698

Merged
merged 2 commits into from
Jul 18, 2021
Merged

Move OnDiskCache to rustc_query_impl. #86698

merged 2 commits into from
Jul 18, 2021

Conversation

cjgillot
Copy link
Contributor

This should be the last remnant of the query implementation that was still in rustc_middle.

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 28, 2021
@cjgillot
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 28, 2021
@bors
Copy link
Contributor

bors commented Jun 28, 2021

⌛ Trying commit 97f5a40109c71a722d059fd1ffac8d86820aeb94 with merge 80ff6e6d03c2115be287fa6ce7b94ba7c1e3dcd7...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 28, 2021

☀️ Try build successful - checks-actions
Build commit: 80ff6e6d03c2115be287fa6ce7b94ba7c1e3dcd7 (80ff6e6d03c2115be287fa6ce7b94ba7c1e3dcd7)

@rust-timer
Copy link
Collaborator

Queued 80ff6e6d03c2115be287fa6ce7b94ba7c1e3dcd7 with parent a435b49, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (80ff6e6d03c2115be287fa6ce7b94ba7c1e3dcd7): comparison url.

Summary: This change led to significant improvements 🎉 in compiler performance.

  • Moderate improvement in instruction counts (up to -4.1% on incr-unchanged builds of ctfe-stress-4-check)

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. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 29, 2021
@bors

This comment has been minimized.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@cjgillot cjgillot force-pushed the modc branch 2 times, most recently from 1b6de32 to 5d1a8ad Compare July 6, 2021 18:51
@rust-log-analyzer

This comment has been minimized.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 14, 2021

☔ The latest upstream changes (presumably #87044) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Jul 14, 2021

☔ The latest upstream changes (presumably #87133) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Jul 17, 2021

☔ The latest upstream changes (presumably #86676) made this pull request unmergeable. Please resolve the merge conflicts.

@estebank
Copy link
Contributor

r=me after rebasing

@cjgillot
Copy link
Contributor Author

@bors r=estebank

@bors
Copy link
Contributor

bors commented Jul 18, 2021

📌 Commit 5b92150 has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 18, 2021
@bors
Copy link
Contributor

bors commented Jul 18, 2021

⌛ Testing commit 5b92150 with merge 1807305...

@bors
Copy link
Contributor

bors commented Jul 18, 2021

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 1807305 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 18, 2021
@bors bors merged commit 1807305 into rust-lang:master Jul 18, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 18, 2021
@cjgillot cjgillot deleted the modc branch July 18, 2021 14:26
@pnkfelix
Copy link
Member

Just a heads up: The perf run that we got after this landed on nightly did not come out quite as rosy as what was predicted from the perf run done locally.

Namely, I see a lot of (small) regressions across several benchmarks, and very few wins at all.

Is there a reason for the different in what we expected from the local run versus what we actually got?

@rylev rylev added the perf-regression Performance regression. label Jul 21, 2021
@Mark-Simulacrum
Copy link
Member

Investigation with cachegrind here is a little difficult: the movement to trait methods (instead of inherent) and a number of other, similar, changes means that the function paths differ sufficiently from run to run that mapping them is hard. That said, some sed'ing and such to adjust seems to point at most of the small regressions here being largely a result of low-level changes in instruction counts scattered across many of the moved functions.

I suspect this is largely due to the movement across different crates creating a slightly different inlining environment and changing optimizer decisions. A diff of one of the moved functions disassembly shows a lot of changes, and the new version is a little longer (-287, +305).

It's also worth noting that the new significance algorithm which more effectively ignores noise on every statistic we collect shows only one benchmark regressing in wall times. This PR also did lower bootstrap time -- overall by 6 seconds -- which is not much, but still a nice improvement. I think that means that the regressions (which are small) are justified.

@rustbot label +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 16, 2021
@tgnottingham
Copy link
Contributor

As with #70951, I assume that some of these regressions would be solved by full program devirtualization (#68262).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.