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

Fix exponential projection complexity on nested types #48296

Merged
merged 4 commits into from Feb 25, 2018

Conversation

@ishitatsuyuki
Copy link
Contributor

@ishitatsuyuki ishitatsuyuki commented Feb 17, 2018

This implements solution 1 from #38528 (comment).

The code quality was extremely poor, but it has improved during review.

Blocking issues:

  • we probably don't want a quadratic deduplication for obligations.
  • is there an alternative to deduplication?

Based on #48315.

Needs changelog. Noticable improvement on compile time is expected.

Fix #38528
Close #39684
Close #43757

@rust-highfive
Copy link
Collaborator

@rust-highfive rust-highfive commented Feb 17, 2018

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@ishitatsuyuki ishitatsuyuki changed the title [wip] (PoC) Fix exponential blowup on nested types [wip] (PoC) Fix exponential projection complexity on nested types Feb 17, 2018
@ishitatsuyuki ishitatsuyuki force-pushed the ishitatsuyuki:exp-unblow branch from 0b84c09 to 91d7792 Feb 17, 2018
Copy link
Contributor Author

@ishitatsuyuki ishitatsuyuki left a comment

Here are some self-reminders.

@@ -818,55 +822,74 @@ fn project_type<'cx, 'gcx, 'tcx>(
&obligation_trait_ref,
&mut candidates);

let decide_commit = |candidates: &mut ProjectionTyCandidateSet<'tcx>| {

This comment has been minimized.

@ishitatsuyuki

ishitatsuyuki Feb 17, 2018
Author Contributor

I have no way but to reorder this before the error handling. It looks non intuitive.

});
debug!("resulting candidate set: {:?}", candidates.vec);
if candidates.vec.len() != 1 {
candidates.ambiguous = true;

This comment has been minimized.

@ishitatsuyuki

ishitatsuyuki Feb 17, 2018
Author Contributor

This is nearly a hack.

}
};

match vtable {
match vtable.clone() {

This comment has been minimized.

@ishitatsuyuki

ishitatsuyuki Feb 17, 2018
Author Contributor

This copy can be avoided.

@ishitatsuyuki ishitatsuyuki force-pushed the ishitatsuyuki:exp-unblow branch from 91d7792 to 365bf21 Feb 18, 2018
@ishitatsuyuki ishitatsuyuki changed the title [wip] (PoC) Fix exponential projection complexity on nested types [wip] Fix exponential projection complexity on nested types Feb 18, 2018
@ishitatsuyuki
Copy link
Contributor Author

@ishitatsuyuki ishitatsuyuki commented Feb 18, 2018

The other part of the blowup (typeck related) is yet to be solved, but I think the code is ready for a round of review. I have removed whitespace changes and clarified the description.

@ishitatsuyuki ishitatsuyuki force-pushed the ishitatsuyuki:exp-unblow branch from 365bf21 to 3d81698 Feb 18, 2018
@ishitatsuyuki
Copy link
Contributor Author

@ishitatsuyuki ishitatsuyuki commented Feb 18, 2018

@kennytm A reminder for you to change the review labels.

@kennytm
Copy link
Member

@kennytm kennytm commented Feb 18, 2018

@bors try

Let’s do a perf check while we’re waiting for review :)

@bors
Copy link
Contributor

@bors bors commented Feb 18, 2018

Trying commit 3d81698 with merge e35a327...

bors added a commit that referenced this pull request Feb 18, 2018
[wip] Fix exponential projection complexity on nested types

This implements solution 1 from #38528 (comment).

The code quality is currently extremely poor, but we can improve them during review.

Based on #48315.

Needs changelog. Noticable improvement on compile time is expected.

Fix #38528 (in progress, another exponential blowup exist)
Close #39684
Close #43757
@kennytm
Copy link
Member

@kennytm kennytm commented Feb 18, 2018

@ishitatsuyuki please remove the [wip] from the PR title if it is ready for review.

@ishitatsuyuki
Copy link
Contributor Author

@ishitatsuyuki ishitatsuyuki commented Feb 18, 2018

Well, I'm currently tackling another part of the exponential issue (so this is WIP), but I need comments on improving the code organization.

@bors
Copy link
Contributor

@bors bors commented Feb 18, 2018

☀️ Test successful - status-travis
State: approved= try=True

@ishitatsuyuki ishitatsuyuki changed the title [wip] Fix exponential projection complexity on nested types Fix exponential projection complexity on nested types Feb 18, 2018
@ishitatsuyuki
Copy link
Contributor Author

@ishitatsuyuki ishitatsuyuki commented Feb 18, 2018

By deduplicating the obligations the issue is completely resolved. I'm not sure if this approach is the best way though, and I copied the dumb deduplication becuase I didn't want to implement Ord everywhere. I will fix them later.

@ishitatsuyuki
Copy link
Contributor Author

@ishitatsuyuki ishitatsuyuki commented Feb 18, 2018

@kennytm I need another try.

@Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Feb 18, 2018

@bors try

@bors
Copy link
Contributor

@bors bors commented Feb 18, 2018

Trying commit 4a40c55 with merge 4ac2425...

@ishitatsuyuki
Copy link
Contributor Author

@ishitatsuyuki ishitatsuyuki commented Feb 22, 2018

r? @nikomatsakis

Please change the label...

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Looks great =)

} else {
Err(())
}
});

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 22, 2018
Contributor

I like the change to push errors into the candidate set. This is less confusing this way.

This comment has been minimized.

@ishitatsuyuki

ishitatsuyuki Feb 22, 2018
Author Contributor

I'm not sure if I understand. I already handle SelectionError above, and these Ok and Err is the return value to commit_if_ok.

// unknown. What we know is that the deduplication avoids exponential
// amount of predicates being propogated when processing deeply nested
// types.
let mut seen = FxHashSet();

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 22, 2018
Contributor

Oh, it seems like duplicates could come from many sources -- in particular, normalization might introduce the same obligation a few times.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Feb 22, 2018

@bors r+

@ishitatsuyuki -- it'd be great to add at least one of these exponential blow-up cases to the perf website so we can monitor for regressions. Perhaps @Mark-Simulacrum can walk you through it.

@bors
Copy link
Contributor

@bors bors commented Feb 22, 2018

📌 Commit 5a2bec9 has been approved by nikomatsakis

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 24, 2018
…sakis

Fix exponential projection complexity on nested types

This implements solution 1 from rust-lang#38528 (comment).

The code quality is currently extremely poor, but we can improve them during review.

Blocking issues:

- we probably don't want a quadratic deduplication for obligations.
- is there an alternative to deduplication?

Based on rust-lang#48315.

Needs changelog. Noticable improvement on compile time is expected.

Fix rust-lang#38528
Close rust-lang#39684
Close rust-lang#43757
bors added a commit that referenced this pull request Feb 24, 2018
Rollup of 15 pull requests

- Successful merges: #47689, #48110, #48197, #48296, #48386, #48392, #48404, #48415, #48441, #48448, #48452, #48481, #48490, #48499, #48503
- Failed merges:
bors added a commit that referenced this pull request Feb 25, 2018
Rollup of 15 pull requests

- Successful merges: #47689, #48110, #48197, #48296, #48386, #48392, #48404, #48415, #48441, #48448, #48452, #48481, #48490, #48499, #48503
- Failed merges:
@bors bors merged commit 5a2bec9 into rust-lang:master Feb 25, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ishitatsuyuki ishitatsuyuki deleted the ishitatsuyuki:exp-unblow branch Mar 1, 2018
ishitatsuyuki added a commit to ishitatsuyuki/rust that referenced this pull request Sep 29, 2020
This effectively reverts rust-lang#43546 as it seems that it does affect performance more than the PR has anticipated.

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.
ishitatsuyuki added a commit to ishitatsuyuki/rust that referenced this pull request Sep 29, 2020
This effectively reverts rust-lang#43546 as it seems that it does affect performance more than the PR has anticipated.

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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 29, 2020
Revert rust-lang#43546

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

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.

Interestingly, the regression tests introduced in rust-lang#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.

![image](https://user-images.githubusercontent.com/12389383/94511110-64b42200-0253-11eb-9dec-1f2632783719.png)

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.
ishitatsuyuki added a commit to ishitatsuyuki/rust that referenced this pull request Sep 30, 2020
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.
ishitatsuyuki added a commit to ishitatsuyuki/rust that referenced this pull request Sep 30, 2020
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.
ishitatsuyuki added a commit to ishitatsuyuki/rust that referenced this pull request Oct 17, 2020
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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants