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

incr. comp.: Methods should have their own dep-graph nodes #36349

Closed
michaelwoerister opened this Issue Sep 8, 2016 · 7 comments

Comments

Projects
None yet
4 participants
@michaelwoerister
Contributor

michaelwoerister commented Sep 8, 2016

Currently there is only one dependency graph node for all items within a given impl. This leads to false positives when detecting changes in methods, that is, changing one method makes the compiler consider the sibling methods to be changed too.

This can be solved by giving each method its own dep-graph node.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Oct 21, 2016

I think the right way to do this is to separate out impl/trait items altogether. I'm going to investigate a bit.

@eddyb

This comment has been minimized.

Member

eddyb commented Oct 21, 2016

FWIW there's some of this already happening on http://github.com/eddyb/rust/commits/lazy-start.
See the test changes in "rustc: introduce TyIncomplete for fine grained handling of type cycles.", which is currently eddyb@2a8aa31#diff-49e88c9171d1c4d781d4922d6870bcc8L36.
On top of that, with a bit of work CollectItem can be removed (as all it does is eagerly request types).

EDIT: updated link after rebase. Also, CollectItem is pointless after eddyb@4c9c8f6.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Nov 1, 2016

I've been working on this. I'm getting close to having a working branch (I'm roughly at the point of getting make check to work). I just noticed @eddyb's comment though and I'm not yet sure how the work I'm doing relates to that. =)

In particular, @eddyb, it seems to me that separating out impl items from the trait is valuable no matter what changes we make, since otherwise changes to the impl item signatures cause the impl's hash to change, no?

bors added a commit that referenced this issue Nov 15, 2016

Auto merge of #37660 - nikomatsakis:incremental-36349, r=eddyb
Separate impl items from the parent impl

This change separates impl item bodies out of the impl itself. This gives incremental more resolution. In so doing, it refactors how the visitors work, and cleans up a bit of the collect/check logic (mostly by moving things out of collect that didn't really belong there, because they were just checking conditions).

However, this is not as effective as I expected, for a kind of frustrating reason. In particular, when invoking `foo.bar()` you still wind up with dependencies on private items. The problem is that the method resolution code scans that list for methods with the name `bar` -- and this winds up touching *all* the methods, even private ones.

I can imagine two obvious ways to fix this:

- separating fn bodies from fn sigs (#35078, currently being pursued by @flodiebold)
- a more aggressive model of incremental that @michaelwoerister has been advocating, in which we hash the intermediate results (e.g., the outputs of collect) so that we can see that the intermediate result hasn't changed, even if a particular impl item has changed.

So all in all I'm not quite sure whether to land this or not. =) It still seems like it has to be a win in some cases, but not with the test cases we have just now. I can try to gin up some test cases, but I'm not sure if they will be totally realistic. On the other hand, some of the early refactorings to the visitor trait seem worthwhile to me regardless.

cc #36349 -- well, this is basically a fix for that issue, I guess

r? @michaelwoerister

NB: Based atop of @eddyb's PR #37402; don't land until that lands.

bors added a commit that referenced this issue Nov 18, 2016

Auto merge of #37660 - nikomatsakis:incremental-36349, r=eddyb
Separate impl items from the parent impl

This change separates impl item bodies out of the impl itself. This gives incremental more resolution. In so doing, it refactors how the visitors work, and cleans up a bit of the collect/check logic (mostly by moving things out of collect that didn't really belong there, because they were just checking conditions).

However, this is not as effective as I expected, for a kind of frustrating reason. In particular, when invoking `foo.bar()` you still wind up with dependencies on private items. The problem is that the method resolution code scans that list for methods with the name `bar` -- and this winds up touching *all* the methods, even private ones.

I can imagine two obvious ways to fix this:

- separating fn bodies from fn sigs (#35078, currently being pursued by @flodiebold)
- a more aggressive model of incremental that @michaelwoerister has been advocating, in which we hash the intermediate results (e.g., the outputs of collect) so that we can see that the intermediate result hasn't changed, even if a particular impl item has changed.

So all in all I'm not quite sure whether to land this or not. =) It still seems like it has to be a win in some cases, but not with the test cases we have just now. I can try to gin up some test cases, but I'm not sure if they will be totally realistic. On the other hand, some of the early refactorings to the visitor trait seem worthwhile to me regardless.

cc #36349 -- well, this is basically a fix for that issue, I guess

r? @michaelwoerister

NB: Based atop of @eddyb's PR #37402; don't land until that lands.
@goffrie

This comment has been minimized.

Contributor

goffrie commented Dec 14, 2016

So... since #37660 landed, is this not fixed? :D

@eddyb

This comment has been minimized.

Member

eddyb commented Dec 14, 2016

@goffrie I think it's still blocked on #37712.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Dec 20, 2016

It's basically fixed, yeah. But we can wait for #37712

@michaelwoerister

This comment has been minimized.

Contributor

michaelwoerister commented Jan 27, 2017

#37712 has landed, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment