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

Refactor Binder to track bound vars #76814

Merged
merged 12 commits into from
Mar 31, 2021
Merged

Conversation

jackh726
Copy link
Member

c.c. @rust-lang/wg-traits

This is super early (and might just get closed at some point), but want to get at least an initial idea of the perf impact.

r? @ghost

@jyn514
Copy link
Member

jyn514 commented Sep 16, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Sep 16, 2020

⌛ Trying commit 4f957e472bde079b127908ca8994e5259654e161 with merge e3ccacbe714c3e189a4895651ada1f854960ddf8...

@jyn514 jyn514 added A-traits Area: Trait system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 16, 2020
@bors
Copy link
Contributor

bors commented Sep 17, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: e3ccacbe714c3e189a4895651ada1f854960ddf8 (e3ccacbe714c3e189a4895651ada1f854960ddf8)

@rust-timer
Copy link
Collaborator

Queued e3ccacbe714c3e189a4895651ada1f854960ddf8 with parent 285fc7d, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (e3ccacbe714c3e189a4895651ada1f854960ddf8): 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 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

@jackh726
Copy link
Member Author

image

Those are the worst results I've ever seen.

@mark-i-m
Copy link
Member

Lol, you should have seen some of the NRVO attempts: #71003 (comment)

@jackh726
Copy link
Member Author

I've rebased this on master, and these changes might change perf, so want to retest that.

@jyn514
Copy link
Member

jyn514 commented Oct 18, 2020

rebind was added that should significantly reduce the work we do

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 18, 2020

⌛ Trying commit 3172ff568d95d27daa25e4d3c20ae3a7ea412358 with merge 13c48570199b4edabb385cc99e6e2a986b1ba938...

@camelid camelid added the I-compiletime Issue: Problems and improvements with respect to compile times. label Oct 18, 2020
@bors
Copy link
Contributor

bors commented Oct 18, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 13c48570199b4edabb385cc99e6e2a986b1ba938 (13c48570199b4edabb385cc99e6e2a986b1ba938)

@rust-timer
Copy link
Collaborator

Queued 13c48570199b4edabb385cc99e6e2a986b1ba938 with parent cbc42a0, future comparison URL.

@jackh726
Copy link
Member Author

@camelid this PR isn't related to compile time

@jyn514 jyn514 removed the I-compiletime Issue: Problems and improvements with respect to compile times. label Oct 18, 2020
@camelid
Copy link
Member

camelid commented Oct 18, 2020

Oh it isn't? Sorry about that :)

Why are you doing a perf run then?

@jyn514
Copy link
Member

jyn514 commented Oct 18, 2020

@jackh726 FYI you can modify labels yourself with rustbot modify labels: https://forge.rust-lang.org/platforms/zulip/triagebot.html#apply-labels-to-issues

@jyn514
Copy link
Member

jyn514 commented Oct 18, 2020

@camelid this is a feature that may have a perf impact, as opposed to a PR that's only for reducing compile time.

@camelid
Copy link
Member

camelid commented Oct 18, 2020

Ah, makes sense. Thanks :)

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (13c48570199b4edabb385cc99e6e2a986b1ba938): 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 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

@bors
Copy link
Contributor

bors commented Mar 31, 2021

⌛ Testing commit 7108918 with merge 4fdac23...

@bors
Copy link
Contributor

bors commented Mar 31, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 4fdac23 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 31, 2021
@bors bors merged commit 4fdac23 into rust-lang:master Mar 31, 2021
@rustbot rustbot added this to the 1.53.0 milestone Mar 31, 2021
@jackh726 jackh726 deleted the binder-refactor branch March 31, 2021 20:39
@rylev
Copy link
Member

rylev commented Apr 1, 2021

@jackh726 @nikomatsakis this change caused a moderate performance regression which seems to be directly related to the changes made. I see the last performance run was never really discussed before this was merged. Is there something we can do to address the performance regressions? This is not severe enough to warrant discussions of reverting the changes, but the regression is severe enough that it should be addressed.

@jackh726
Copy link
Member Author

jackh726 commented Apr 1, 2021

So, first, this PR is important longer term in order enable the Chalk integration (without expensive conversions).
However, a few things that can be done to in order of easiest/soonest to longer term:

  • Remove Binder::bind and only use Binder::dummy (I expect this will us back a significant % of lost perf)
  • Refactor rustc_resolve::late::lifetimes to not duplicate trait headers or to run per-owner (not technically this PR, but related)
  • Refactor pretty printing code to allow removing BoundRegionKind from BoundRegion, which will remove the need to "anonymize" regions
  • Chalk integration

Ultimately though, the performance regression here is unavoidable in the short-term. We do strictly more work because we have to track bound vars. Furthermore, I didn't spend a bunch of effort pursuing specific optimizations (maybe there's a clever way to revert the size increase, for example) since that would be out of scope here.

@camelid
Copy link
Member

camelid commented Apr 1, 2021

Remove Binder::bind and only use Binder::dummy (I expect this will us back a significant % of lost perf)

Do you think this would be approachable enough for a newcomer to the trait system like me?

@Dylan-DPC-zz
Copy link

Do you think this would be approachable enough for a newcomer to the trait system like me?

@camelid there's one way to find out ;)

@jackh726
Copy link
Member Author

jackh726 commented Apr 1, 2021

@camelid I think almost all of them should be fairly easy

@jackh726
Copy link
Member Author

jackh726 commented Apr 1, 2021

I managed to be able to remove all but one I think, in testing

@camelid
Copy link
Member

camelid commented Apr 1, 2021

Each call seems to look like ty::Binder::bind(predicate, tcx). What code would I replace the Binder::bind calls with?

@jackh726
Copy link
Member Author

jackh726 commented Apr 1, 2021

ty::Binder::dummy(predicate)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-traits Area: Trait system 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. WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804
Projects
None yet
Development

Successfully merging this pull request may close these issues.