Separate impl items from the parent impl #37660

nikomatsakis · 2016-11-09T02:08:18Z

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 (Separate fn signature from fn body in HIR #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.

nikomatsakis · 2016-11-09T14:40:31Z

Hmm, had a thought last night. Another option that might make sense is to pull the Name for each impl item into the parent impl (it can appear both places). This would allow us to do the method search to find methods of name foo while just reading from the parent impl item. Moreover, it solves the problem better than separate the signature from the body: just doing that would mean that if I change the signature of a private method, we have to recompile callers of other methods, just because we haven't separated those two things.

Obviously, if we pursued the more aggressive strategy of rehashing partial results it sort of subsumes both cases. However, I remain somewhat nervous about that in the shorter term (though longer term it seems good). But also it seems advantageous to be able to determine as much as we can without having to re-run parts of the compiler.

eddyb · 2016-11-09T15:04:52Z

@nikomatsakis Maybe #37402 plus some dep node changes can solve that?
It moves out type information from ty::AssociatedItem so it's less than the signature.

nikomatsakis · 2016-11-09T15:12:07Z

@eddyb well, it seems to me that the current dep information is correct, unless we move the Name out into the impl. That is, if I were to change the name of various inherent methods on Foo, I ought to re-type-check things that called methods on Foo. And, the way things are setup now, that would be reflected in a change to the impl-item -- indistinguishable from other changes.

So fundamentally there are two ways to fix this:

refine the HIR so that we separate out the things of interest:
- this covers both separating sig from body and separating name from impl
- the more precise we get it, the better things work, so doing both is prob good
change our approach to hash intermediate results:
- this compensates for imprecise HIR information

It seems to me that for best results we want both. Having more precise information lets us quickly rule out work without even need to reconfirm. But at a certain point you get diminishing returns. Though perhaps the initial work to recompute signatures and so forth is cheap enough that it doesn't make sense to separate HIR more. I can easily believe that.

I've talked about this with @michaelwoerister a fair amount. I tend to view this as "firewalls". That is, you have points where we hash the intermediate results because we can't (or don't care to) track the edges closely enough. The first example is from the files to the HIR. The second (existing) example is after trans collection. It makes sense to do it in more places, I suspect. Then in between these firewalls you can be conservative and just trace out the impact of a chance and invalidate the work in between said change and the next "firewall" (where you will want to re-hash). This will all fit very nicely with the work you are doing to make the compiler more demand-driven, of course, so I had hoped we could start moving more in that direction as that proceeds, but in short term get the HIR precise enough. (I still believe that.)

michaelwoerister · 2016-11-09T15:17:48Z

Another option that might make sense is to pull the Name for each impl item into the parent impl (it can appear both places).

That seems like a good idea to me.

michaelwoerister · 2016-11-09T15:19:49Z

Obviously, if we pursued the more aggressive strategy of rehashing partial results it sort of subsumes both cases.

I also think that is true and as a consequence we should avoid doing any contortions in HIR representation just to improve dependency tracking. But the examples so far seem to make sense on their on right.

michaelwoerister · 2016-11-09T15:39:31Z

src/librustc_typeck/check/mod.rs

@@ -974,9 +979,11 @@ fn check_impl_items_against_trait<'a, 'tcx>(ccx: &CrateCtxt<'a, 'tcx>,
 let trait_def = tcx.lookup_trait_def(impl_trait_ref.def_id);
 let mut overridden_associated_type = None;

+ let impl_items = || impl_item_ids.iter().map(|&id| ccx.tcx.map.impl_item(id));


Why wrap this in a closure?

michaelwoerister · 2016-11-09T19:28:57Z

So, I did one pass looking through all the commits and I think it should indeed be merged. There are many good refactorings in it that make things cleaner, especially with respect to what's going on in ty::collect. Also making the different levels for HIR visitors more explicit is a good thing.

nikomatsakis · 2016-11-09T21:55:37Z

@michaelwoerister so I just pushed a new commit intended to make the "nested" pattern a little more future-proof. This was mildly more tedious than I thought because it requires having better lifetime annotations to make it work (most visitors are written to accept a much wider range of lifetimes than is actually needed, since it winds up needing less explicit use of 'tcx). But it seems like an improvement regardless. Take a look.

bors · 2016-11-09T23:14:46Z

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

michaelwoerister · 2016-11-10T15:23:45Z

I just pushed a new commit intended to make the "nested" pattern a little more future-proof.

I like it. It's a bit unfortunate with the additional lifetime annotations but not really a problem.
It's a bit subtle that overriding nested_visit_map() will turn the visitor into a nested one. Maybe a name like nesting_enabled for the method would improve that?

nikomatsakis · 2016-11-11T00:55:22Z

@michaelwoerister

It's a bit subtle that overriding nested_visit_map() will turn the visitor into a nested one. Maybe a name like nesting_enabled for the method would improve that?

Sure, I can rename it. I'd prefer to leverage specialization to make a NestedVisitor trait or something but we can't quite do that nicely.

Also, I just pushed a PR that separates the name into the impl, which actually fixes the test cases that were blocked on this.

michaelwoerister · 2016-11-11T14:48:21Z

src/librustc_typeck/coherence/overlap.rs

- let name_and_namespace = |def_id| {
- let item = self.tcx.associated_item(def_id);
+ let name_and_namespace = |r: &ty::AssociatedItemRef| {
+ let item = self.tcx.associated_item(r.def_id);


This is one of the cases where it would make sense to also store the kind of associated item in the ref.

I think I agree. I'd almost go as far as having associated_item use the DefId of its container for the dep node, and then just use associated_items everywhere.

michaelwoerister · 2016-11-11T14:50:59Z

Also, I just pushed a PR that separates the name into the impl, which actually fixes the test cases that were blocked on this.

I just looked through the new commits. Very nice :)

nikomatsakis · 2016-11-11T23:41:27Z

@michaelwoerister @eddyb ready for re-review; I removed AssociatedItemRef and consolidated everything into ImplItemRef (the same should be done for traits, but I'd prefer to leave it for later).

I think there is one thing missing from this PR before it can land though: we are not hashing all of the things in the ImplItemRef along with the impl, I suspect, so we need to do that, and I need to make some ICH tests for it.

nikomatsakis · 2016-11-12T00:38:43Z

Oh, I should add -- it didn't quite work as well as the old approach across crates, but I think that's a separate issue really. I filed #37720 describing it.

bors · 2016-11-12T12:16:21Z

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

eddyb · 2016-11-12T12:22:55Z

src/librustc_typeck/collect.rs

+ // for details.
+ ctp::setup_constraining_predicates(&mut ty_predicates.predicates,
+ trait_ref,
+ &mut ctp::parameters_for_impl(selfty, trait_ref));


Lovely! This might actually solve my trouble with this ordering thing, looks like it's self-contained now.

eddyb · 2016-11-12T12:25:10Z

r=me with a rebase.

(Unrelated to this PR series)

This sidesteps problems with long paths because the canonical path includes the "magic long path prefix" on Windows.

nikomatsakis · 2016-11-17T18:45:03Z

@bors r+

bors · 2016-11-17T18:45:07Z

📌 Commit ab79438 has been approved by nikomatsakis

nikomatsakis · 2016-11-17T18:45:10Z

@bors r=eddyb

bors · 2016-11-17T18:45:12Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Support useing externally defined macros behind #![feature(use_extern_macros)] #37732

bors · 2016-11-17T18:45:12Z

📌 Commit ab79438 has been approved by eddyb

nikomatsakis · 2016-11-17T20:13:35Z

@bors r=eddyb

bors · 2016-11-17T20:13:36Z

📌 Commit c938007 has been approved by eddyb

bors · 2016-11-18T01:31:02Z

⌛ Testing commit c938007 with merge 35e8924...

@flodiebold

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 · 2016-11-18T04:56:01Z

The `visit_fn` code mutates its surrounding context. Between *items*, this was saved/restored, but between impl items it was not. This meant that we wound up with `CallSiteScope` entries with two parents (or more!). As far as I can tell, this is harmless in actual type-checking, since the regions you interact with are always from at most one of those branches. But it can slow things down. Before, the effect was limited, since it only applied to impl items within an impl. After rust-lang#37660, impl items are visisted all together at the end, and hence this could create a very messed up hierarchy. Isolating impl item properly solves both issues. I cannot come up with a way to unit-test this; for posterity, however, you can observe the messed up hierarchies with a test as simple as the following, which would create a callsite scope with two parents both before and after ``` struct Foo { } impl Foo { fn bar(&self) -> usize { 22 } fn baz(&self) -> usize { 22 } } fn main() { } ``` Fixes rust-lang#37864.

@michaelwoerister

in region, treat current (and future) item-likes alike The `visit_fn` code mutates its surrounding context. Between *items*, this was saved/restored, but between impl items it was not. This meant that we wound up with `CallSiteScope` entries with two parents (or more!). As far as I can tell, this is harmless in actual type-checking, since the regions you interact with are always from at most one of those branches. But it can slow things down. Before, the effect was limited, since it only applied to impl items within an impl. After #37660, impl items are visisted all together at the end, and hence this could create a very messed up hierarchy. Isolating impl item properly solves both issues. I cannot come up with a way to unit-test this; for posterity, however, you can observe the messed up hierarchies with a test as simple as the following, which would create a callsite scope with two parents both before and after ``` struct Foo { } impl Foo { fn bar(&self) -> usize { 22 } fn baz(&self) -> usize { 22 } } fn main() { } ``` Fixes #37864. r? @michaelwoerister cc @pnkfelix -- can you think of a way to make a regr test?

@michaelwoerister

in region, treat current (and future) item-likes alike The `visit_fn` code mutates its surrounding context. Between *items*, this was saved/restored, but between impl items it was not. This meant that we wound up with `CallSiteScope` entries with two parents (or more!). As far as I can tell, this is harmless in actual type-checking, since the regions you interact with are always from at most one of those branches. But it can slow things down. Before, the effect was limited, since it only applied to impl items within an impl. After #37660, impl items are visisted all together at the end, and hence this could create a very messed up hierarchy. Isolating impl item properly solves both issues. I cannot come up with a way to unit-test this; for posterity, however, you can observe the messed up hierarchies with a test as simple as the following, which would create a callsite scope with two parents both before and after ``` struct Foo { } impl Foo { fn bar(&self) -> usize { 22 } fn baz(&self) -> usize { 22 } } fn main() { } ``` Fixes #37864. r? @michaelwoerister cc @pnkfelix -- can you think of a way to make a regr test?

@michaelwoerister

in region, treat current (and future) item-likes alike The `visit_fn` code mutates its surrounding context. Between *items*, this was saved/restored, but between impl items it was not. This meant that we wound up with `CallSiteScope` entries with two parents (or more!). As far as I can tell, this is harmless in actual type-checking, since the regions you interact with are always from at most one of those branches. But it can slow things down. Before, the effect was limited, since it only applied to impl items within an impl. After #37660, impl items are visisted all together at the end, and hence this could create a very messed up hierarchy. Isolating impl item properly solves both issues. I cannot come up with a way to unit-test this; for posterity, however, you can observe the messed up hierarchies with a test as simple as the following, which would create a callsite scope with two parents both before and after ``` struct Foo { } impl Foo { fn bar(&self) -> usize { 22 } fn baz(&self) -> usize { 22 } } fn main() { } ``` Fixes #37864. r? @michaelwoerister cc @pnkfelix -- can you think of a way to make a regr test?

rust-highfive assigned michaelwoerister Nov 9, 2016

michaelwoerister reviewed Nov 9, 2016

View reviewed changes

alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 10, 2016

nikomatsakis force-pushed the incremental-36349 branch from 75a5c98 to bd0a191 Compare November 11, 2016 02:24

michaelwoerister reviewed Nov 11, 2016

View reviewed changes

This was referenced Nov 11, 2016

Separate trait items from trait #37712

Closed

Separate foreign items in HIR #37713

Closed

nikomatsakis force-pushed the incremental-36349 branch 4 times, most recently from acfac99 to 4fb31ca Compare November 11, 2016 23:39

eddyb reviewed Nov 12, 2016

View reviewed changes

nikomatsakis force-pushed the incremental-36349 branch 3 times, most recently from 15209aa to 9364fc2 Compare November 14, 2016 23:53

nikomatsakis added 3 commits November 17, 2016 13:44

fix oversight in closure translation

b8116da

(Unrelated to this PR series)

fix change_private_impl_method_cc test

4e844ad

canonicalize base incremental path on windows

ab79438

This sidesteps problems with long paths because the canonical path includes the "magic long path prefix" on Windows.

nikomatsakis force-pushed the incremental-36349 branch from ec345a3 to ab79438 Compare November 17, 2016 18:44

add test for hashing trait impls

c938007

bors merged commit c938007 into rust-lang:master Nov 18, 2016

This was referenced Nov 18, 2016

ICH: Handle MacroDef HIR instances. #37787

Merged

instantiate closures on demand #37797

Merged

Mark-Simulacrum mentioned this pull request Nov 18, 2016

Compile-Time Performance Regression #37864

Closed

dtolnay mentioned this pull request Nov 19, 2016

Does not compile on rustc 1.15.0-nightly (ac635aa95 2016-11-18) rust-lang/rust-clippy#1350

Closed

nikomatsakis mentioned this pull request Nov 21, 2016

in region, treat current (and future) item-likes alike #37920

Merged

goffrie mentioned this pull request Dec 14, 2016

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

Closed

nikomatsakis deleted the incremental-36349 branch April 14, 2017 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate impl items from the parent impl #37660

Separate impl items from the parent impl #37660

nikomatsakis commented Nov 9, 2016 •

edited

Loading

nikomatsakis commented Nov 9, 2016

eddyb commented Nov 9, 2016

nikomatsakis commented Nov 9, 2016

michaelwoerister commented Nov 9, 2016

michaelwoerister commented Nov 9, 2016

michaelwoerister Nov 9, 2016

michaelwoerister commented Nov 9, 2016

nikomatsakis commented Nov 9, 2016

bors commented Nov 9, 2016

michaelwoerister commented Nov 10, 2016

nikomatsakis commented Nov 11, 2016

michaelwoerister Nov 11, 2016

eddyb Nov 11, 2016

michaelwoerister commented Nov 11, 2016

nikomatsakis commented Nov 11, 2016

nikomatsakis commented Nov 12, 2016

bors commented Nov 12, 2016

eddyb Nov 12, 2016

eddyb commented Nov 12, 2016

nikomatsakis commented Nov 17, 2016

bors commented Nov 17, 2016

nikomatsakis commented Nov 17, 2016

bors commented Nov 17, 2016

bors commented Nov 17, 2016

nikomatsakis commented Nov 17, 2016

bors commented Nov 17, 2016

bors commented Nov 18, 2016

bors commented Nov 18, 2016

Separate impl items from the parent impl #37660

Separate impl items from the parent impl #37660

Conversation

nikomatsakis commented Nov 9, 2016 • edited Loading

nikomatsakis commented Nov 9, 2016

eddyb commented Nov 9, 2016

nikomatsakis commented Nov 9, 2016

michaelwoerister commented Nov 9, 2016

michaelwoerister commented Nov 9, 2016

michaelwoerister Nov 9, 2016

Choose a reason for hiding this comment

michaelwoerister commented Nov 9, 2016

nikomatsakis commented Nov 9, 2016

bors commented Nov 9, 2016

michaelwoerister commented Nov 10, 2016

nikomatsakis commented Nov 11, 2016

michaelwoerister Nov 11, 2016

Choose a reason for hiding this comment

eddyb Nov 11, 2016

Choose a reason for hiding this comment

michaelwoerister commented Nov 11, 2016

nikomatsakis commented Nov 11, 2016

nikomatsakis commented Nov 12, 2016

bors commented Nov 12, 2016

eddyb Nov 12, 2016

Choose a reason for hiding this comment

eddyb commented Nov 12, 2016

nikomatsakis commented Nov 17, 2016

bors commented Nov 17, 2016

nikomatsakis commented Nov 17, 2016

bors commented Nov 17, 2016

bors commented Nov 17, 2016

nikomatsakis commented Nov 17, 2016

bors commented Nov 17, 2016

bors commented Nov 18, 2016

bors commented Nov 18, 2016

nikomatsakis commented Nov 9, 2016 •

edited

Loading