Revert #43546 "save subobligations in the projection cache" #77325

ishitatsuyuki · 2020-09-29T04:03:25Z

This effectively reverts #43546 as it seems that it does affect performance more than the PR has anticipated.

This also removes the deduplication code from #48296 as duplications were primarily coming from #43546 and after removing that code it probably doesn't worth paying the cost of using a hash map.

Interestingly, the regression tests introduced in #43546 didn't fail after reverting, but some other tests had breakage. For now I just applied --bless, but I'd like to figure out if it could be a problem later on.

Preliminary perf results confirms that we no longer need deduplication to avoid exponential behavior.

This PR will be draft until I figure out the potential breakage.

r? @ghost

Need a perf run. A crater run will help as well to identify potential breakage.

jyn514 · 2020-09-29T05:36:31Z

@bors try @rust-timer queue

rust-timer · 2020-09-29T05:36:33Z

Awaiting bors try build completion

bors · 2020-09-29T05:36:43Z

⌛ Trying commit cb7a9c6ac5b49777171397997012564810c86a14 with merge f1391937758145f22ee8b89aba5b57d9766029a6...

jyn514 · 2020-09-29T05:37:14Z

compiler/rustc_trait_selection/src/traits/project.rs

- // This is the hottest path in this function.
- //
- // If we find the value in the cache, then return it along
- // with the obligations that went along with it. Note
- // that, when using a fulfillment context, these
- // obligations could in principle be ignored: they have
- // already been registered when the cache entry was
- // created (and hence the new ones will quickly be
- // discarded as duplicated). But when doing trait
- // evaluation this is not the case, and dropping the trait
- // evaluations can causes ICEs (e.g., #43132).


Any reason you removed the comment too?

I dropped it because I also removed the relevant code; that said, I think I deleted too much and some of them still serve as an explanation, so I will restore them later.

bors · 2020-09-29T06:23:10Z

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

rust-timer · 2020-09-29T06:23:12Z

Queued f1391937758145f22ee8b89aba5b57d9766029a6 with parent 48cab67, future comparison URL.

rust-timer · 2020-09-29T11:19:13Z

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

ishitatsuyuki · 2020-09-29T11:33:15Z

Perf summary: improvements on deeply-nested, around -1% for certain (probably assoc-type heavy) cases, and no change for vast of the benchmarks.

I'll continue to try to figure out what might break...

ishitatsuyuki · 2020-09-30T11:24:09Z

I've concluded that the changes in UI test stderr can be justified, as it's not a recursive obligation but just a (unresolvable) self-reference, and such cyclic definitions are unlikely in real world use cases to warrant a specific diagnostic. I did some additional cleanup to fully revert the related changes.

As for potential breakages, I feel it's unlikely to happen given all the tests are passing. Maybe we could get a crater run though.

r? @nikomatsakis since you authored the original changes

nikomatsakis · 2020-10-05T22:48:40Z

@ishitatsuyuki the primary motivation here is performance, or something else?

ishitatsuyuki · 2020-10-06T01:12:26Z

The primary motivation is not performance, probably.

I was looking at the debug! traces of the trait resolver in order to understand something else, but I end up discovering that seemingly simple predicates can resolve to a large number of obligations due to the duplicates, like the case below:

AssocTypeNormalizer: depth=0 normalized <T as num::dec2flt::rawfp::RawFloat>::Bits to <T as num::dec2flt::rawfp::RawFloat>::Bits, now with 20 obligations

So then I spent some time figuring out why does this happen, and the result is this PR. The motivation is not that strong, I just wanted to get rid of these redundant log entries.

bors · 2020-10-06T15:16:18Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

ishitatsuyuki · 2020-10-08T11:40:08Z

Status: get_paranoid_cache_value_obligation is gone with #73905 and I've got a real test failure now, so I need some time to figure out a fix (or to close this if I can't fix it).

ishitatsuyuki · 2020-10-12T09:55:21Z

New discovery: a side effect of this PR is that #76179 would be fixed. I don't know if this is the only way to fix it, though.

ishitatsuyuki · 2020-10-17T12:22:29Z

Turns out the (what I think are) regressions was actually caused by #76179, so I filed #78049. Not sure whether the fix of #76179 is correct or accidental.

jackh726 · 2021-05-21T16:31:56Z

Looks like crater queue for this isn't correct. Both the older check and a newer "run" are queued. Should only be a new "check"

Dylan-DPC-zz · 2021-05-21T16:36:54Z

@craterbot abort name=pr-77325

craterbot · 2021-05-21T16:36:55Z

🗑️ Experiment pr-77325 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Dylan-DPC-zz · 2021-05-21T16:38:14Z

@craterbot retry (just to ensure it's in the right state)

craterbot · 2021-05-21T16:38:15Z

🚨 Error: failed to parse the command

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Dylan-DPC-zz · 2021-05-21T16:39:19Z

@jackh726 removed the older one now :) thanks

jackh726 · 2021-05-21T17:01:15Z

We don't need a test, just a check

@craterbot abort name=pr-77325-1

@craterbot check

craterbot · 2021-05-21T17:01:18Z

🗑️ Experiment pr-77325-1 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

jackh726 · 2021-05-21T17:03:01Z

@craterbot check

craterbot · 2021-05-21T17:03:05Z

👌 Experiment pr-77325 created and queued.
🤖 Automatically detected try build 7c3a44cf5f95fa5cf164a312bdcf845157af7093
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Dylan-DPC-zz · 2021-05-21T17:03:57Z

aah cool :)

nikomatsakis · 2021-06-02T13:23:35Z

r? @nikomatsakis

craterbot · 2021-06-09T07:07:45Z

🚧 Experiment pr-77325 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2021-06-12T13:06:57Z

🎉 Experiment pr-77325 is completed!
📊 61 regressed and 11 fixed (163710 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

ishitatsuyuki · 2021-06-13T07:30:21Z

It seems that this indeed introduces problems with trait resolution in some cases:

Variant 1
Variant 2

Something similar happened when I updated the test cases, although all of them were compile-fail tests so I thought that fine and went with a bless. Turns out it isn't OK.

I probably will close this PR, unless there turns out to be an easy enough solution to tackle the regressions above.

jackh726 · 2021-07-01T13:19:21Z

Maybe we should see if #84944 suffers from the same problems as this PR

jyn514 added I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 29, 2020

jyn514 reviewed Sep 29, 2020

View reviewed changes

jyn514 added the A-traits Area: Trait system label Sep 29, 2020

ishitatsuyuki force-pushed the nodup branch 2 times, most recently from 0abd9d7 to 914ec15 Compare September 30, 2020 11:19

ishitatsuyuki marked this pull request as ready for review September 30, 2020 11:19

rust-highfive assigned nikomatsakis Sep 30, 2020

jyn514 changed the title ~~Revert #43546~~ Revert #43546 "save subobligations in the projection cache" Sep 30, 2020

ishitatsuyuki added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 5, 2020

ishitatsuyuki added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 14, 2020

ishitatsuyuki force-pushed the nodup branch from 914ec15 to 1fda149 Compare October 17, 2020 12:21

ishitatsuyuki added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 17, 2020

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 21, 2021

Dylan-DPC-zz added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2021

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 21, 2021

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2021

rust-highfive assigned nikomatsakis and unassigned jackh726 Jun 2, 2021

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jun 12, 2021

ishitatsuyuki closed this Jul 1, 2021

jackh726 mentioned this pull request Dec 6, 2021

Re-introduce concept of projection cache 'completion' #89831

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert #43546 "save subobligations in the projection cache" #77325

Revert #43546 "save subobligations in the projection cache" #77325

ishitatsuyuki commented Sep 29, 2020

jyn514 commented Sep 29, 2020

rust-timer commented Sep 29, 2020

bors commented Sep 29, 2020

jyn514 Sep 29, 2020

ishitatsuyuki Sep 29, 2020

bors commented Sep 29, 2020

rust-timer commented Sep 29, 2020

rust-timer commented Sep 29, 2020

ishitatsuyuki commented Sep 29, 2020 •

edited

Loading

ishitatsuyuki commented Sep 30, 2020

nikomatsakis commented Oct 5, 2020

ishitatsuyuki commented Oct 6, 2020

bors commented Oct 6, 2020

ishitatsuyuki commented Oct 8, 2020 •

edited

Loading

ishitatsuyuki commented Oct 12, 2020

ishitatsuyuki commented Oct 17, 2020

jackh726 commented May 21, 2021

Dylan-DPC-zz commented May 21, 2021

craterbot commented May 21, 2021

Dylan-DPC-zz commented May 21, 2021

craterbot commented May 21, 2021

Dylan-DPC-zz commented May 21, 2021

jackh726 commented May 21, 2021

craterbot commented May 21, 2021

jackh726 commented May 21, 2021

craterbot commented May 21, 2021

Dylan-DPC-zz commented May 21, 2021

nikomatsakis commented Jun 2, 2021

craterbot commented Jun 9, 2021

craterbot commented Jun 12, 2021

ishitatsuyuki commented Jun 13, 2021

jackh726 commented Jul 1, 2021

Revert #43546 "save subobligations in the projection cache" #77325

Revert #43546 "save subobligations in the projection cache" #77325

Conversation

ishitatsuyuki commented Sep 29, 2020

jyn514 commented Sep 29, 2020

rust-timer commented Sep 29, 2020

bors commented Sep 29, 2020

jyn514 Sep 29, 2020

Choose a reason for hiding this comment

ishitatsuyuki Sep 29, 2020

Choose a reason for hiding this comment

bors commented Sep 29, 2020

rust-timer commented Sep 29, 2020

rust-timer commented Sep 29, 2020

ishitatsuyuki commented Sep 29, 2020 • edited Loading

ishitatsuyuki commented Sep 30, 2020

nikomatsakis commented Oct 5, 2020

ishitatsuyuki commented Oct 6, 2020

bors commented Oct 6, 2020

ishitatsuyuki commented Oct 8, 2020 • edited Loading

ishitatsuyuki commented Oct 12, 2020

ishitatsuyuki commented Oct 17, 2020

jackh726 commented May 21, 2021

Dylan-DPC-zz commented May 21, 2021

craterbot commented May 21, 2021

Dylan-DPC-zz commented May 21, 2021

craterbot commented May 21, 2021

Dylan-DPC-zz commented May 21, 2021

jackh726 commented May 21, 2021

craterbot commented May 21, 2021

jackh726 commented May 21, 2021

craterbot commented May 21, 2021

Dylan-DPC-zz commented May 21, 2021

nikomatsakis commented Jun 2, 2021

craterbot commented Jun 9, 2021

craterbot commented Jun 12, 2021

ishitatsuyuki commented Jun 13, 2021

jackh726 commented Jul 1, 2021

ishitatsuyuki commented Sep 29, 2020 •

edited

Loading

ishitatsuyuki commented Oct 8, 2020 •

edited

Loading