Remove interior mutability from TraitDef by turning fields into queries by michaelwoerister · Pull Request #41911 · rust-lang/rust

michaelwoerister · 2017-05-11T14:51:17Z

This PR gets rid of anything std::cell in TraitDef by

moving the global list of trait impls from TraitDef into a query,
moving the list of trait impls relevent for some self-type from TraitDef into a query
moving the specialization graph of trait impls into a query, and
moving TraitDef::object_safety into a query.

I really like how querifying things not only helps with incremental compilation and on-demand, but also just plain makes the code cleaner :)

There are also some smaller fixes in the PR. Commits can be reviewed separately.

r? @eddyb or @nikomatsakis

eddyb

I like the overall approach but have some doubts on the details.

eddyb · 2017-05-11T22:15:07Z

src/librustc/traits/mod.rs

Since they're identical I'd use the same function twice.

You think so? It's just a coincidence that they are identical at the moment.

maybe have provide() call provide_extern()? I imagine that anything we provide for extern crates, we would also provide for locals, but maybe not the other way around

That makes sense.

eddyb · 2017-05-11T22:16:43Z

src/librustc/traits/project.rs

How does this work? Do we produce cycle errors in more cases?

I'm not sure what you mean exactly. I don't see a potential cycle here.

The !is_complete case is a fallback for cycles, that is, when normalization of e.g. <T as Foo>::Assoc is needed to compute Foo's specialization graph.

cc @aturon We may be lacking tests here.

Oh, so assoc_ty_def() might be called while still building the specialization graph?

It needs to be used in the specialization graph of the same trait.

It needs to be used in the specialization graph of the same trait.

Yes, that makes sense. And I thought the following example might trigger the condition, but it doesn't :(

#![feature(specialization)] trait Assoc { type Output; } impl<T> Assoc for T { default type Output = u8; } impl Assoc for u32 { type Output = u32; } impl Assoc for u64 { // Shouldn't this cause a reentrant call? type Output = <u32 as Assoc>::Output; } fn main() { let _x: <u32 as Assoc>::Output = 0; }

In the impl header - only Self/params are checked by coherence/specialization.

So, the following gives me the error overflow evaluating the requirement <u32 as Assoc>::Output regardless of whether cycle detection is enabled:

#![feature(specialization)] trait Assoc { type Output; } impl Assoc for u32 { type Output = u64; } impl Assoc for <u32 as Assoc>::Output { type Output = u64; } fn main() { let _x: <u64 as Assoc>::Output = 0; }

So that's not really what we are looking for, I guess?

Hmm, I don't really expect you to be able to project <u32 as Assoc>::Output in an impl for Assoc. What I expect could work is if there is a DAG between the traits, sort of like this:

#![feature(specialization)] use std::vec; trait Assoc { type Output; } impl Assoc for u32 { type Output = u64; } impl Assoc for <vec::IntoIter<u64> as Iterator>::Item { type Output = u64; } fn main() { }

Interestingly, coherence rejects this example, presumably because of RFC 1214 concerns. I'm not sure that's right, but this example does work today:

#![feature(specialization)] use std::vec; trait Assoc { type Output; } impl Assoc for u32 { type Output = u64; } impl Assoc for <u32 as Assoc2>::Output { type Output = u64; } trait Assoc2 { type Output; } impl Assoc2 for u32 { type Output = u64; } fn main() { }

That said, I think that would also work in @michaelwoerister's branch, no?

eddyb · 2017-05-11T22:18:53Z

src/librustc/ty/maps.rs

Wait, where are all the blanket impls?

They are part of the list.

eddyb · 2017-05-11T22:23:17Z

src/librustc/ty/trait_def.rs

This is potentially very bad, as N blanket impls and M non-blanket now use N*M space.

Hm, OK, let me think about that. Maybe we could share the list of blanket impls?

bors · 2017-05-13T07:51:20Z

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

michaelwoerister · 2017-05-15T13:38:18Z

@eddyb I added the cycle check back in (40a6734) and blanket impl lists are now shared (742ebc1).

But I didn't manage to come up with a test case that would cause a cycle (see #41911 (comment)), unfortunately.

eddyb · 2017-05-15T14:52:12Z

@michaelwoerister Yeah, I'm waiting on @aturon and/or @nikomatsakis for that.

nikomatsakis · 2017-05-16T00:30:05Z

src/librustc/hir/map/mod.rs


    pub fn trait_impls(&self, trait_did: DefId) -> &'hir [NodeId] {
-        self.dep_graph.read(DepNode::TraitImpls(trait_did));
+        self.dep_graph.read(DepNode::AllLocalTraitImpls);


I'm curious why you converted this into a single dep-node, rather than keeping it per-trait. It seems like this is losing quite a bit of precision, no?

(That is, if any trait adds an impl, we will consider them all to have changed.)

Mostly for ease of implementation and symmetry with the situation in metadata. DepNode::TraitImpls has been re-purposed to represent local and remote impls and, with red-green, will take care of stopping invalidation short.

Without red-green though you are right, it's losing precision. Want me to make it per-item?

Probably not a big deal.

nikomatsakis · 2017-05-16T00:40:19Z

src/librustc/traits/project.rs

Hmm, I don't really expect you to be able to project <u32 as Assoc>::Output in an impl for Assoc. What I expect could work is if there is a DAG between the traits, sort of like this:

#![feature(specialization)] use std::vec; trait Assoc { type Output; } impl Assoc for u32 { type Output = u64; } impl Assoc for <vec::IntoIter<u64> as Iterator>::Item { type Output = u64; } fn main() { }

Interestingly, coherence rejects this example, presumably because of RFC 1214 concerns. I'm not sure that's right, but this example does work today:

#![feature(specialization)] use std::vec; trait Assoc { type Output; } impl Assoc for u32 { type Output = u64; } impl Assoc for <u32 as Assoc2>::Output { type Output = u64; } trait Assoc2 { type Output; } impl Assoc2 for u32 { type Output = u64; } fn main() { }

That said, I think that would also work in @michaelwoerister's branch, no?

eddyb · 2017-05-16T09:43:19Z

src/librustc/ty/trait_def.rs

+            let impl_trait_ref = tcx.impl_trait_ref(impl_def_id).unwrap();
+            let impl_simple_self_ty = fast_reject::simplify_type(tcx,
+                                                                 impl_trait_ref.self_ty(),
+                                                                 false).unwrap();


I just realized, this is stricter than it has to be. it should use tcx.type_of(impl_def_id) instead.

eddyb · 2017-05-16T09:43:29Z

src/librustc/ty/trait_def.rs

+                         .map(|&node_id| tcx.hir.local_def_id(node_id));
+
+     for impl_def_id in local_impls.chain(remote_impls.into_iter()) {
+        let impl_trait_ref = tcx.impl_trait_ref(impl_def_id).unwrap();


Same here, I suppose.

eddyb · 2017-05-16T13:43:11Z

src/test/compile-fail/coherence-inherited-assoc-ty-cycle-err.rs

+
+// Formerly this ICEd with the following message:
+// Tried to project an inherited associated type during coherence checking,
+// which is currently not supported.


Where is that message in the source? Could that codepath be removed?

It here:

rust/src/librustc/traits/project.rs

Line 989 in 4d09a0e

span_bug!(obligation.cause.span,

But it has this nice, long comment which made me reluctant to remove it. I could move the comment to assoc_ty_def(), make its return value non-optional, and then remove the branch though.

Yeah, moving the comment should be fine.

… in relevant_impls_for().

michaelwoerister · 2017-05-16T15:33:21Z

@eddyb I'm not entirely sure if that last commit (08660af) is an improvement.

nikomatsakis · 2017-05-16T17:51:21Z

src/librustc/traits/project.rs

-                    span_bug!(obligation.cause.span,
-                              "Tried to project an inherited associated type during \
-                               coherence checking, which is currently not supported.");
+                    None


Why did you remove this comment etc? Do we think this code-path is dead, or not?

If we think it's unreachable, I'd rather see bug!(...), probably with an updated comment explaining that we expect this scenario to be prevented by cycle-errors.

The diff is a bit confusion here because indentation has changed. The None belongs to let new_candidate = if !is_default {, for which nothing should have changed, I believe.

👍 I see now

nikomatsakis · 2017-05-17T13:03:24Z

@bors r+

bors · 2017-05-17T13:03:25Z

📌 Commit 08660af has been approved by nikomatsakis

bors · 2017-05-17T21:44:15Z

⌛ Testing commit 08660af with merge 4640e18...

@eddyb

…akis Remove interior mutability from TraitDef by turning fields into queries This PR gets rid of anything `std::cell` in `TraitDef` by - moving the global list of trait impls from `TraitDef` into a query, - moving the list of trait impls relevent for some self-type from `TraitDef` into a query - moving the specialization graph of trait impls into a query, and - moving `TraitDef::object_safety` into a query. I really like how querifying things not only helps with incremental compilation and on-demand, but also just plain makes the code cleaner `:)` There are also some smaller fixes in the PR. Commits can be reviewed separately. r? @eddyb or @nikomatsakis

bors · 2017-05-18T00:35:55Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 4640e18 to master...

A change in rust-lang#41911 had made `for_all_relevant_impls` do a linear scan over all impls, instead of using an HashMap. Use an HashMap again to avoid quadratic blowup when there is a large number of structs with impls. I think this fixes rust-lang#43141 completely, but I want better measurements in order to be sure. As a perf patch, please don't roll this up.

@eddyb

make `for_all_relevant_impls` O(1) again A change in #41911 had made `for_all_relevant_impls` do a linear scan over all impls, instead of using an HashMap. Use an HashMap again to avoid quadratic blowup when there is a large number of structs with impls. I think this fixes #43141 completely, but I want better measurements in order to be sure. As a perf patch, please don't roll this up. r? @eddyb beta-nominating because regression

A change in rust-lang#41911 had made `for_all_relevant_impls` do a linear scan over all impls, instead of using an HashMap. Use an HashMap again to avoid quadratic blowup when there is a large number of structs with impls. I think this fixes rust-lang#43141 completely, but I want better measurements in order to be sure. As a perf patch, please don't roll this up.

rust-highfive assigned eddyb May 11, 2017

alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 11, 2017

eddyb reviewed May 11, 2017

View reviewed changes

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2017

michaelwoerister added 6 commits May 15, 2017 15:27

ICH: Hash lists of local trait impls as part of the HIR.

c2d9b4e

Make incr. comp. test case dependent on specific ICH instead of SVH

513cc6d

Fix instability in GlobalMetadata::Impls ICH.

77b7df3

Remove interior mutability from TraitDef by turning fields into queries.

8da2fe8

Re-introduce cycle-check in assoc. item resolution.

40a6734

Share lists of blanket impls in results of relevant_impls_for() query.

742ebc1

michaelwoerister force-pushed the querify_trait_def branch from 3abfdbe to 742ebc1 Compare May 15, 2017 13:28

michaelwoerister added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 15, 2017

nikomatsakis reviewed May 16, 2017

View reviewed changes

eddyb reviewed May 16, 2017

View reviewed changes

michaelwoerister added 2 commits May 16, 2017 15:03

Don't use queries::try_get() in assoc_ty projection

d731b04

Add test cases for cyclic specialization graph construction

0a77a58

eddyb reviewed May 16, 2017

View reviewed changes

michaelwoerister added 2 commits May 16, 2017 17:12

Use tcx.type_of(impl) instead of TraitRef::self_ty() for getting Self…

42051ce

… in relevant_impls_for().

Remove unreachable branches in traits::project

08660af

nikomatsakis reviewed May 16, 2017

View reviewed changes

bors merged commit 08660af into rust-lang:master May 18, 2017

This was referenced May 18, 2017

remove interior mutability of type-flags #42015

Merged

Enable cross-crate incremental compilation by default. #42055

Merged

Rollup of 3 pull requests #42074

Closed

Mark-Simulacrum mentioned this pull request Jul 12, 2017

Compile time regression #43141

Closed

arielb1 mentioned this pull request Aug 7, 2017

make for_all_relevant_impls O(1) again #43723

Merged

eddyb mentioned this pull request Mar 27, 2020

Non-deterministic perf.rust-lang.org runs (e.g. syn-opt). #69060

Open

Uh oh!

Conversation

michaelwoerister commented May 11, 2017

Uh oh!

eddyb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented May 13, 2017

Uh oh!

michaelwoerister commented May 15, 2017

Uh oh!

eddyb commented May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelwoerister commented May 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb May 12, 2017 •

edited

Loading

eddyb May 12, 2017 •

edited

Loading

eddyb commented May 15, 2017 •

edited

Loading