Eliminate `ObligationCauseData` by nnethercote · Pull Request #91844 · rust-lang...

Copy link

Contributor

nnethercote commented on Dec 13, 2021

rust-timer commented on Dec 13, 2021

Awaiting bors try build completion.

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

bors commented on Dec 13, 2021

Trying commit ddb96eb with merge 220db1b...

This comment has been hidden.

nnethercote commented on Dec 13, 2021

@bors try

bors commented on Dec 13, 2021

Trying commit 748ccba with merge 498fbcc...

bors commented on Dec 13, 2021

Try build successful - checks-actions
Build commit: 498fbcc (498fbcc6241318ce8d97f067af15d4fba425d18b)

rust-timer commented on Dec 13, 2021

rust-timer commented on Dec 13, 2021

Finished benchmarking commit (498fbcc): comparison url.

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

• Large improvement in instruction counts (up to -3.8% on incr-unchanged builds of deep-vector)
• Small regression in instruction counts (up to 0.7% on incr-full builds of deeply-nested)

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

nnethercote commented on Dec 13, 2021

Lots more wins than losses among the instruction counts, plus it's a tiny reduction in number of lines of code. I think this is worth going ahead with.

r? @Mark-Simulacrum

I agree that these wins exceed the regressions, so this is worth it.

I am wondering, though, if it makes sense to try to go further here. In particular, it seems to me that many ObligationCauseCode's are or could be <= one pointer in "size", in which case Lrc<...> is adding an allocation that's not really necessary.

For example, just by moving the discriminant into the Obligation (one extra byte), we'd be able to represent a lot of the variants inline -- trivially all of the ones that have no data attached, and in theory those which have data that's only usize wide (e.g., a single DefId). I don't off-hand know what the distribution there looks like -- this PR adds a comment suggesting this covers 60% of cases with just MiscObligation, but it's worth noting that we keep adding new obligation cause codes, so presumably that percentage is only going to decrease over time. I'm not sure what the distribution on the rest of the codes is like.

Further optimization seems fine to leave for future PRs, but I would like the comment on the Option discriminant hashing addressed; r=me with that done (another perf run may be in order).

bors commented on Dec 18, 2021

Testing commit 47d6bdf with merge d985d42...

bors commented on Dec 18, 2021

Test failed - checks-actions

This comment has been hidden.

bors commented 29 days ago

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

nnethercote commented 28 days ago

I rebased to fix the conflict. While there, I added a clean_code() function that trivially avoids some additional allocations relating to the parent_code field, that helps with #87012.

@bors r=Mark-simulacrum

bors commented 28 days ago

Commit d7ca962 has been approved by Mark-simulacrum

This comment has been hidden.

nnethercote commented 28 days ago

@bors r=Mark-Simulacrum

bors commented 28 days ago

Commit f09b1fa has been approved by Mark-Simulacrum

bors commented 28 days ago

Testing commit f09b1fa with merge ed7a206...

bors commented 28 days ago

Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing ed7a206 to master...

nnethercote commented 28 days ago

So we could effectively split ObligationCauseCode in half: one part for all the simple variants, one part for all the complex variants.

I did some quick measurements and the potential for improvement here seems very low. The simple variants account for a small fraction (~5% or less) of the occurrences seen in practice (while compiling std).

rust-timer commented 28 days ago

Finished benchmarking commit (ed7a206): comparison url.

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

• Small improvement in instruction counts (up to -0.9% on full builds of coercions)
• Small regression in instruction counts (up to 0.9% on full builds of wg-grammar)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-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 open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression