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

Always produce sub-obligations when using cached projection result #85382

Merged
merged 1 commit into from
May 21, 2021

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented May 16, 2021

See #85360

When we skip adding the sub-obligations to the obligation list, we can affect whether or not the final result is EvaluatedToOk or EvaluatedToOkModuloObligations. This creates problems for incremental compilation, since the projection cache is untracked shared state.

To solve this issue, we unconditionally process the sub-obligations. Surprisingly, this is a slight performance win in many cases.

@rust-highfive
Copy link
Collaborator

r? @varkor

(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 May 16, 2021
@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 16, 2021
@bors
Copy link
Contributor

bors commented May 16, 2021

⌛ Trying commit 8657bb2 with merge c3a39b612f192447636019e943fd635f36375fb6...

@bors
Copy link
Contributor

bors commented May 16, 2021

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

@rust-timer
Copy link
Collaborator

Queued c3a39b612f192447636019e943fd635f36375fb6 with parent 7dc9ff5, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (c3a39b612f192447636019e943fd635f36375fb6): 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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

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

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned varkor May 17, 2021
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 20, 2021

📌 Commit 8657bb2 has been approved by nikomatsakis

@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 May 20, 2021
@nikomatsakis
Copy link
Contributor

Well, can't argue with the benchmark results!

@bors
Copy link
Contributor

bors commented May 21, 2021

⌛ Testing commit 8657bb2 with merge 6f5a198...

@bors
Copy link
Contributor

bors commented May 21, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 6f5a198 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 21, 2021
@bors bors merged commit 6f5a198 into rust-lang:master May 21, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants