Refactor Binder to track bound vars

[jackh726]

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]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 4f957e472bde079b127908ca8994e5259654e161 with merge e3ccacbe714c3e189a4895651ada1f854960ddf8...

[bors]

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

[rust-timer]

Queued e3ccacbe714c3e189a4895651ada1f854960ddf8 with parent 285fc7d, future comparison URL.

[rust-timer]

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]

Those are the worst results I've ever seen.

[mark-i-m]

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

[jackh726]

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

[jyn514]

rebind was added that should significantly reduce the work we do

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

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

[bors]

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

[rust-timer]

Queued 13c48570199b4edabb385cc99e6e2a986b1ba938 with parent cbc42a0, future comparison URL.

[jackh726]

@camelid this PR isn't related to compile time

[camelid]

Oh it isn't? Sorry about that :)

Why are you doing a perf run then?

[jyn514]

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

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

[camelid]

Ah, makes sense. Thanks :)

[rust-timer]

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]

⌛ Testing commit 7108918 with merge 4fdac23...

[bors]

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

[rylev]

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

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]

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]

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]

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

[jackh726]

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

[camelid]

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

[jackh726]

ty::Binder::dummy(predicate)