Skip to content

Don't cache results of coinductive cycle #96458

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
Merged
merged 1 commit into from
May 7, 2022
Merged

Don't cache results of coinductive cycle #96458

merged 1 commit into from
May 7, 2022

Conversation

Aaron1011
Copy link
Member

No description provided.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 27, 2022
@rust-highfive
Copy link
Contributor

r? @estebank

(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 Apr 27, 2022
@Aaron1011
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 Apr 27, 2022
@bors
Copy link
Collaborator

bors commented Apr 27, 2022

⌛ Trying commit e9e4ab8a5f2bbb6cddd036c62f8339c5232e4b5c with merge c19d1ba0dab84507be11fff585888fa39b00e4b0...

@bors
Copy link
Collaborator

bors commented Apr 27, 2022

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

@rust-timer
Copy link
Collaborator

Queued c19d1ba0dab84507be11fff585888fa39b00e4b0 with parent 082e4ca, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c19d1ba0dab84507be11fff585888fa39b00e4b0): comparison url.

Summary:

  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: no relevant changes found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 0 1 0 1
mean2 N/A N/A -0.6% N/A -0.6%
max N/A N/A -0.6% N/A -0.6%

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 may lead to changes in compiler perf.

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

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 27, 2022
@Aaron1011
Don't cache results of coinductive cycle
40c6d83 
Fixes rust-lang#96319

The logic around handling co-inductive cycles in the evaluation cache
is confusing and error prone. Fortunately, a perf run showed that it
doesn't actually appear to improve performance, so we can simplify
this code (and eliminate a source of ICEs) by just skipping caching
the evaluation results for co-inductive cycle participants.

This commit makes no changes to any of the other logic around
co-inductive cycle handling. Thus, while this commit could
potentially expose latent bugs that were being hidden by
caching, it should not introduce any new bugs.
@Aaron1011 Aaron1011 force-pushed the no-cycle-caching branch from e9e4ab8 to 40c6d83 Compare May 5, 2022 18:03
@Aaron1011 Aaron1011 changed the title [DO NOT MERGE] Don't cache results of coinductive cycle Don't cache results of coinductive cycle May 5, 2022
@Aaron1011
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 May 5, 2022
@bors
Copy link
Collaborator

bors commented May 5, 2022

⌛ Trying commit 40c6d83 with merge 85755f8c91122429d4224fb842a5752cedfff3e9...

@bors
Copy link
Collaborator

bors commented May 5, 2022

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

@rust-timer
Copy link
Collaborator

Queued 85755f8c91122429d4224fb842a5752cedfff3e9 with parent a7d6768, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (85755f8c91122429d4224fb842a5752cedfff3e9): comparison url.

Summary:

  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: no relevant changes found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 1 0 0 0 1
mean2 0.3% N/A N/A N/A 0.3%
max 0.3% N/A N/A N/A 0.3%

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 may lead to changes in compiler perf.

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

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 5, 2022
@Aaron1011
Copy link
Member Author

cc @rust-lang/wg-traits @cjgillot - this PR is now ready for review.

I tested this on the benchmark from #91598, and verified that it didn't result in an exponential blowup. However, it would probably be a good idea to test this PR on some various 'complex' uses of auto traits, to reduce the chance that this leads to a huge regression missed by our perf suite.

@jackh726
Copy link
Member

jackh726 commented May 6, 2022

r=me on the "don't cache" side of things. Not sure if @cjgillot has thoughts.

@cjgillot
Copy link
Contributor

cjgillot commented May 6, 2022

On the incr. comp. side, I agree that this is the best solution for now. Devising a clean scheme to handle cycles is very hard, and probably way more brittle.
We still cache one node in the cycle, a kind of cycle root. This node can be re-discovered by the other participants in the cycle by following the "slow" path. In that case, the cached node would be correct.
As a consequence, the new scheme is more robust than the earlier one, so let's go ahead.

@bors r=jackh726,cjgillot

@bors
Copy link
Collaborator

bors commented May 6, 2022

📌 Commit 40c6d83 has been approved by jackh726,cjgillot

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 6, 2022
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 6, 2022
@lqd
Copy link
Member

lqd commented May 6, 2022

We could add a test from #96319.

@bors
Copy link
Collaborator

bors commented May 6, 2022

⌛ Testing commit 40c6d83 with merge 4799baa...

@bors
Copy link
Collaborator

bors commented May 7, 2022

☀️ Test successful - checks-actions
Approved by: jackh726,cjgillot
Pushing 4799baa to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 7, 2022
@bors bors merged commit 4799baa into rust-lang:master May 7, 2022
@rustbot rustbot added this to the 1.62.0 milestone May 7, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4799baa): comparison url.

Summary:

  • Primary benchmarks: mixed results
  • Secondary benchmarks: 🎉 relevant improvement found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 1 0 1 1 2
mean2 0.3% N/A -0.2% -0.2% 0.0%
max 0.3% N/A -0.2% -0.2% 0.3%

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

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@estebank estebank

Labels
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.62.0
Development

Successfully merging this pull request may close these issues.
9 participants
@Aaron1011 @rust-highfive @rust-timer @bors @jackh726 @cjgillot @lqd @estebank @rustbot