Skip to content

save subobligations in the projection cache - #43546

Merged
bors merged 1 commit into
rust-lang:masterfrom
nikomatsakis:issue-43132
Jul 31, 2017
Merged

save subobligations in the projection cache#43546
bors merged 1 commit into
rust-lang:masterfrom
nikomatsakis:issue-43132

Conversation

@nikomatsakis

@nikomatsakisnikomatsakis commented Jul 29, 2017

Copy link
Copy Markdown
Contributor

The projection cache explicitly chose not to "preserve" subobligations for projections, since the fulfillment context ought to have been doing so. But for the trait evaluation scheme that causes problems. This PR reproduces subobligations. This has the potential to slow down compilation, but minimal investigation suggests it does not do so.

One hesitation about this PR: I could not find a way to make a standalone test case for #43132 (but admittedly I did not try very hard).

Fixes#43132.

r? @arielb1

@alexcrichton

alexcrichton commented Jul 29, 2017

Copy link
Copy Markdown
Member

I've "minimized" a test case which at least doesn't depend on the futures crate, but I wouldn't necessarily call it small yet.

@est31

est31 commented Jul 29, 2017

Copy link
Copy Markdown
Member

I've managed to make @alexcrichton 's example a bit smaller (118 lines -> 60 lines).

@arielb1arielb1 added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 30, 2017
@arielb1

arielb1 commented Jul 30, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

beta-nominating because this is a regression - I shuffled things around and made evaluation easier to reach.

r=me with a test.

@bors

bors commented Jul 30, 2017

Copy link
Copy Markdown
Collaborator

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

@alexcrichton

alexcrichton commented Jul 30, 2017

Copy link
Copy Markdown
Member

@bor: r=arielb1

@alexcrichton

alexcrichton commented Jul 31, 2017

Copy link
Copy Markdown
Member

@bors: r=arielb1

@bors

bors commented Jul 31, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit 2574f31 has been approved by arielb1

@bors

bors commented Jul 31, 2017

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 2574f31 with merge f6d7873...

bors added a commit that referenced this pull request Jul 31, 2017
@bors
Auto merge of#43546- nikomatsakis:issue-43132, r=arielb1
f6d7873
save subobligations in the projection cache
The projection cache explicitly chose not to "preserve" subobligations for projections, since the fulfillment context ought to have been doing so. But for the trait evaluation scheme that causes problems. This PR reproduces subobligations. This has the potential to slow down compilation, but minimal investigation suggests it does not do so.
One hesitation about this PR: I could not find a way to make a standalone test case for #43132 (but admittedly I did not try very hard).
Fixes#43132.
r? @arielb1
@bors

bors commented Jul 31, 2017

Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing f6d7873 to master...

@bors
bors merged commit 2574f31 into rust-lang:masterJul 31, 2017
@nikomatsakisnikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jul 31, 2017
@nikomatsakis

nikomatsakis commented Jul 31, 2017

Copy link
Copy Markdown
ContributorAuthor

I'm marking this as beta-accepted -- it's a small and harmless patch.

@arielb1

arielb1 commented Aug 1, 2017

Copy link
Copy Markdown
Contributor

The author of #43132 complains that this caused an exponential case in his code. I can believe that - need to investigate.

@alexcrichton

alexcrichton commented Aug 1, 2017

Copy link
Copy Markdown
Member

It looks like at least one test also regressed on perf.rust-lang.org

@remram44remram44 mentioned this pull request Aug 10, 2017
Closed
@alexcrichtonalexcrichton mentioned this pull request Aug 12, 2017
Merged
@alexcrichtonalexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 12, 2017
bors added a commit that referenced this pull request Aug 13, 2017
@bors
Auto merge of#43831- alexcrichton:beta-next, r=alexcrichton
54279df
Backporting accepted PRs to beta
Backport of:
* #43735
* #43546
* #43513
* #43373
* #43315
@arielb1arielb1 mentioned this pull request Aug 17, 2017
Merged
@TimNNTimNN mentioned this pull request Aug 17, 2017
Closed
@ishitatsuyukiishitatsuyuki mentioned this pull request Sep 29, 2020
Closed
bors pushed a commit to rust-lang-ci/rust that referenced this pull request May 21, 2021
@ishitatsuyuki
Revertrust-lang#43546
a1815d7
This effectively reverts rust-lang#43546 as it seems that it does affect performance more than the PR has anticipated. Follow-up changes from rust-lang#44269 are also reverted.
This also removes the deduplication code from rust-lang#48296 as duplications were primarily coming from rust-lang#43546 and after removing that code it probably doesn't worth paying the cost of using a hash map.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@arielb1arielb1

Labels

beta-acceptedAccepted for backporting to the compiler in the beta channel.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nikomatsakis@alexcrichton@est31@arielb1@bors