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

Stop allocating vtable entries for non-object-safe methods #88552

Merged
merged 2 commits into from
Sep 5, 2021

Conversation

nbdd0121
Copy link
Contributor

@nbdd0121 nbdd0121 commented Sep 1, 2021

Current a vtable entry is allocated for all associated fns, even if the method is not object-safe: https://godbolt.org/z/h7vx6f35T

As a result, each vtable for Iterator' currently consumes 74 usizes. This PR stops allocating vtable entries for those methods, reducing vtable size of each Iterator vtable to 7 usizes.

Note that this PR introduces will cause more invocations of is_vtable_safe_method. So a perf run might be needed. If result isn't favorable then we might need to query-ify is_vtable_safe_method.

@rust-highfive
Copy link
Collaborator

r? @nagisa

(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 Sep 1, 2021
@jonas-schievink jonas-schievink added the I-heavy Issue: Problems and improvements with respect to binary size of generated code. label Sep 1, 2021
@Aaron1011
Copy link
Member

@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 Sep 1, 2021
@bors
Copy link
Contributor

bors commented Sep 1, 2021

⌛ Trying commit bb74667e52e83b465fb47c2d32634fdc7ace0637 with merge f884054025e1df9c3b9c6e59123af3d8014e085d...

@bors
Copy link
Contributor

bors commented Sep 1, 2021

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

@rust-timer
Copy link
Collaborator

Queued f884054025e1df9c3b9c6e59123af3d8014e085d with parent 00ce166, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (f884054025e1df9c3b9c6e59123af3d8014e085d): comparison url.

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

  • Small improvement in instruction counts (up to -0.4% on full builds of inflate)
  • Large regression in instruction counts (up to 2.8% on incr-unchanged builds of webrender)

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 @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 Sep 1, 2021
@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Sep 1, 2021

Introduced a query. Please do another perf run

@Aaron1011
Copy link
Member

@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 Sep 1, 2021
@bors
Copy link
Contributor

bors commented Sep 1, 2021

⌛ Trying commit 6c7644deb04e51894c7abf0da8abb0fcaeaac15f with merge f4005c89228ae80fe5d43005600154074957d538...

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@bors
Copy link
Contributor

bors commented Sep 4, 2021

⌛ Trying commit 83c5dab94c62142956dd2e86455afe37303ee36e with merge 4e5b7bbe1459e4b0667320310649719962750f5a...

@bors
Copy link
Contributor

bors commented Sep 4, 2021

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

@rust-timer
Copy link
Collaborator

Queued 4e5b7bbe1459e4b0667320310649719962750f5a with parent 226e181, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (4e5b7bbe1459e4b0667320310649719962750f5a): comparison url.

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

  • Small improvement in instruction counts (up to -0.5% on incr-unchanged builds of encoding)
  • Small regression in instruction counts (up to 0.4% on full builds of inflate)

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 @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

@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Sep 5, 2021

Done (I manually edited the file so I don't have to recompile everything)

@nagisa
Copy link
Member

nagisa commented Sep 5, 2021

@bors delegate=nbdd0121

@bors
Copy link
Contributor

bors commented Sep 5, 2021

✌️ @nbdd0121 can now approve this pull request

@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Sep 5, 2021

Race condition :)

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Sep 5, 2021

📌 Commit 7c2031082fa92f345ec377e9f91c64da9ce51321 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 Sep 5, 2021
@rust-log-analyzer

This comment has been minimized.

@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Sep 5, 2021

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 5, 2021
@camelid
Copy link
Member

camelid commented Sep 5, 2021

@bors r-

CI is failing again; please wait for CI to pass before re-approving the PR (otherwise it slows down the bors queue) :)

@camelid
Copy link
Member

camelid commented Sep 5, 2021

Heh, another race condition :)

@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Sep 5, 2021

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Sep 5, 2021

📌 Commit 97214ee 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 5, 2021
@bors
Copy link
Contributor

bors commented Sep 5, 2021

⌛ Testing commit 97214ee with merge e30b683...

@bors
Copy link
Contributor

bors commented Sep 5, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 5, 2021
@bors bors merged commit e30b683 into rust-lang:master Sep 5, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 5, 2021
@nbdd0121 nbdd0121 deleted the vtable branch September 6, 2021 17:34
@Mark-Simulacrum
Copy link
Member

This had mixed results on landing, generally, with no significant cycle count results. Marking as triaged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-heavy Issue: Problems and improvements with respect to binary size of generated code. 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.