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.

Sign up for GitHub

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

Jump to bottom

Turn vtable_allocation() into a query #89619

Merged
merged 2 commits into from
Oct 8, 2021
Merged

Turn vtable_allocation() into a query #89619

merged 2 commits into from
Oct 8, 2021

Conversation

michaelwoerister
Copy link
Member

@michaelwoerister michaelwoerister commented Oct 7, 2021 •
edited
Loading

This PR removes the untracked vtable-const-allocation cache from the tcx and turns the vtable_allocation() method into a query.

The change is pretty straightforward and should be backportable without too much effort.

Fixes #89598.

@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(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 Oct 7, 2021
@michaelwoerister michaelwoerister added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Oct 7, 2021
@michaelwoerister
Copy link
Member 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 Oct 7, 2021
@bors
Copy link
Contributor

bors commented Oct 7, 2021

⌛ Trying commit 59a9346bfc4080a60062dd3be0faefef8ec1181e with merge d366508c69d893916d86d7dd00ab980cccf50812...

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 7, 2021
@bors
Copy link
Contributor

bors commented Oct 7, 2021

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

@rust-timer
Copy link
Collaborator

Queued d366508c69d893916d86d7dd00ab980cccf50812 with parent ca8078d, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d366508c69d893916d86d7dd00ab980cccf50812): comparison url.

Summary: This change led to small relevant mixed results 🤷 in compiler performance.

  • Small improvement in instruction counts (up to -0.4% on incr-unchanged builds of helloworld)
  • Small regression in instruction counts (up to 0.5% on incr-unchanged builds of clap-rs)

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 try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 7, 2021
@michaelwoerister
Copy link
Member Author

michaelwoerister commented Oct 7, 2021 •
edited
Loading

I just realized that the test case does not actually do what I expected it to do. It does work -- but only by accident. I'll rewrite it to be more robust.

@apiraino
Copy link
Contributor

apiraino commented Oct 7, 2021

Beta backport accepted as per compiler team on Zulip

Stable backport declined, we are very close to the next stable release.

@rustbot label +beta-accepted -stable-nominated

@rustbot rustbot added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed stable-nominated Nominated for backporting to the compiler in the stable channel. labels Oct 7, 2021
@nagisa
Copy link
Member

nagisa commented Oct 7, 2021

Implementation LGTM; r=me once the test is adjusted as necessary.

@michaelwoerister
Copy link
Member Author

I updated the test case with an explanation of what's actually happening.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@michaelwoerister
Copy link
Member Author

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Oct 7, 2021

📌 Commit b7cc991 has been approved by nagisa

@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 Oct 7, 2021
@michaelwoerister
Copy link
Member Author

michaelwoerister commented Oct 8, 2021 •
edited
Loading

@bors p=1 (since the PR is beta-accepted)

@bors
Copy link
Contributor

bors commented Oct 8, 2021

⌛ Testing commit b7cc991 with merge 44995f7...

@bors
Copy link
Contributor

bors commented Oct 8, 2021

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 44995f7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 8, 2021
@bors bors merged commit 44995f7 into rust-lang:master Oct 8, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 8, 2021
@bors bors mentioned this pull request Oct 8, 2021
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (44995f7): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@rustbot rustbot removed the perf-regression Performance regression. label Oct 8, 2021
@cuviper cuviper mentioned this pull request Oct 13, 2021
Merged
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 13, 2021
@cuviper cuviper modified the milestones: 1.57.0, 1.56.0 Oct 13, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2021
@bors
Auto merge of rust-lang#89854 - cuviper:beta-next, r=Mark-Simulacrum
58268ff 
[beta] backports

- 2229: Consume IfLet expr rust-lang#89282
- Wrapper for -Z gcc-ld=lld to invoke rust-lld with the correct flavor rust-lang#89288
- Fix unsound optimization with explicit variant discriminants rust-lang#89489
- Fix stabilization version for bindings_after_at rust-lang#89605
- Turn vtable_allocation() into a query rust-lang#89619
- Revert "Stabilize Iterator::intersperse()" rust-lang#89638
- Ignore type of projections for upvar capturing rust-lang#89648
- ~~Add Poll::ready and~~ revert stabilization of task::ready! rust-lang#89651
- CI: Use mirror for libisl downloads for more docker dist builds rust-lang#89661
-  Use correct edition for panic in [debug_]assert!(). rust-lang#89622
-  Switch to our own mirror of libisl plus ct-ng oldconfig fixes rust-lang#89599
-  Emit item no type error even if type inference fails rust-lang#89585
-  Revert enum discriminants rust-lang#89884
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 20, 2021
@bors
Auto merge of rust-lang#89619 - michaelwoerister:incr-vtables, r=nagisa
2228c49 
Turn vtable_allocation() into a query

This PR removes the untracked vtable-const-allocation cache from the `tcx` and turns the `vtable_allocation()` method into a query.

The change is pretty straightforward and should be backportable without too much effort.

Fixes rust-lang#89598.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@RalfJung RalfJung RalfJung left review comments

Assignees

@davidtwco davidtwco

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.56.0
Development

Successfully merging this pull request may close these issues.

VTable-related miscompilation with incremental compilation