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

Bad regression in coherence checking in winapi #32499

Closed
brson opened this issue Mar 26, 2016 · 5 comments
Closed

Bad regression in coherence checking in winapi #32499

brson opened this issue Mar 26, 2016 · 5 comments
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@brson
Copy link
Contributor

brson commented Mar 26, 2016

Coherence checking from winapi went from negligible time to 126 seconds.

From @retep998 on IRC.

22:28 < WindowsBunnies> acrichto: brson: um, I'm experiencing a bit of a coherence checking regression
22:28 < WindowsBunnies> 126 seconds on coherence checking for winapi using rustc 1.9.0-nightly (21922e1f4 2016-03-21)
22:29 < WindowsBunnies> aturon: I'm poking you because eddyb suspects it was specialization that caused it and you did specialization
22:29  * WindowsBunnies updates to latest nightly to check whether it still occurs
22:38 < WindowsBunnies> Okay, I can confirm that it occurs with latest nightly multirust-rs could get me which is rustc 1.9.0-nightly (d7a71687e 2016-03-24)
22:39 < WindowsBunnies> Here is the -Ztime-passes https://gist.github.com/retep998/d8a276e41112ec4ff539
22:40 < WindowsBunnies> for the record, here is what compile times used to look like some months ago https://gist.github.com/retep998/0e9aaeb53037bb6b89ae
22:42 < WindowsBunnies> time: 147.409; rss: 333MB       coherence checking

I can't offer further information now. cc @rust-lang/lang.

@brson brson added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Mar 26, 2016
@aturon
Copy link
Member

aturon commented Mar 26, 2016

cc me

@nikomatsakis
Copy link
Contributor

Certainly specialization seems like a likely candidate. I wonder if some simple hashing using simplified types could help here? Have to go review what the code does.

@aturon
Copy link
Member

aturon commented Mar 26, 2016

At the moment, specialization does essentially no caching, relying instead on selection caching. However, I believe that the coherence part of specialization (where the graph is built) is not able to use this cache as currently written. There are lots of easy opportunities to do better, if this is indeed the culprit.

@aturon
Copy link
Member

aturon commented Mar 29, 2016

I confirmed that this is due to specialization. I think, in particular, it's stemming from the move away from using simplified types.

Amusingly, the initial version of the code I wrote for the specialization graph actually did keep the simplified type distinction, but I dropped it because I doubted it was worthwhile here. Looks like I was wrong! I'll work to get a patch together in the next couple of days.

aturon added a commit to aturon/rust that referenced this issue Apr 5, 2016
The initial implementation of specialization did not use the
`fast_reject` mechanism when checking for overlap, which caused a
serious performance regression in some cases.

This commit modifies the specialization graph to use simplified types
for fast rejection when possible, and along the way refactors the logic
for building the specialization graph.

Closes rust-lang#32499
@aturon
Copy link
Member

aturon commented Apr 5, 2016

Fix here: #32748

Manishearth added a commit to Manishearth/rust that referenced this issue Apr 6, 2016
Reinstate fast_reject for overlap checking

The initial implementation of specialization did not use the
`fast_reject` mechanism when checking for overlap, which caused a
serious performance regression in some cases.

This commit modifies the specialization graph to use simplified types
for fast rejection when possible, and along the way refactors the logic
for building the specialization graph.

Closes rust-lang#32499

r? @nikomatsakis
Manishearth added a commit to Manishearth/rust that referenced this issue Apr 7, 2016
Reinstate fast_reject for overlap checking

The initial implementation of specialization did not use the
`fast_reject` mechanism when checking for overlap, which caused a
serious performance regression in some cases.

This commit modifies the specialization graph to use simplified types
for fast rejection when possible, and along the way refactors the logic
for building the specialization graph.

Closes rust-lang#32499

r? @nikomatsakis
Manishearth added a commit to Manishearth/rust that referenced this issue Apr 7, 2016
Reinstate fast_reject for overlap checking

The initial implementation of specialization did not use the
`fast_reject` mechanism when checking for overlap, which caused a
serious performance regression in some cases.

This commit modifies the specialization graph to use simplified types
for fast rejection when possible, and along the way refactors the logic
for building the specialization graph.

Closes rust-lang#32499

r? @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants