fix a few errant `Krate` edges #35960

nikomatsakis · 2016-08-24T12:15:45Z

Exploring the effect of small changes on syntex reuse, I discovered the following sources of unnecessary edges from Krate

r? @michaelwoerister

nagisa · 2016-08-24T14:51:33Z

src/librustc/dep_graph/graph.rs

+
+    /// Tracks nodes that are forbidden to read/write; see
+    /// `DepGraph::forbid`. Used for debugging only.
+    forbidden: RefCell<Vec<DepNode<DefId>>>,


Should this field also be #[cfg(debug_assertions)]-ed?

michaelwoerister · 2016-08-24T21:30:30Z

Good catches both!
r=me with the nits and addressed.

arielb1 · 2016-08-25T19:39:35Z

That said, the pattern that is used for the inherent impls map
is generally an anti-pattern (that is, holding the borrow lock for the
duration of using the contents)

I remember a similar place in coherence where I intentionally held the lock because someone (i.e., metadata loading) was inserting stuff to the list behind my back and I wanted to prevent that problem from recurring.

If nobody is supposed to mutate a list behind your back, holding a lock is the "safe" option - it can lead to more ICEs, but it can make your assumptions more visible.

bors · 2016-08-27T13:05:08Z

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

nikomatsakis · 2016-08-31T19:28:09Z

@bors r=mw

bors · 2016-08-31T19:28:10Z

📌 Commit 30c6f12 has been approved by mw

nikomatsakis · 2016-09-01T01:41:17Z

@bors r-

nikomatsakis · 2016-09-01T01:41:24Z

Warnings from travis look legit, even though I didn't see anything like that locally. Maybe I didn't run full tests on the latest commit or something.

nikomatsakis · 2016-09-01T21:56:25Z

@bors r=mw

bors · 2016-09-01T21:56:26Z

📌 Commit d287b7a has been approved by mw

nikomatsakis · 2016-09-02T08:45:02Z

@bors r- // again the travis issue looks legit, and again I wonder why I am not seeing this locally!

nikomatsakis · 2016-09-02T09:02:08Z

@nagisa @michaelwoerister

Should this field also be #[cfg(debug_assertions)]-ed?

The answer is sort of yes, but I wound up (see most recent commit) taking a different approach. In particular, the number of things that must have #[cfg] applied to them seemed to be growing at a kind of ridiculous rate, and moreover making a field #[cfg] is kind of annoying since you can't apply #[cfg] to the field in the struct literals. I really find code with #[cfg] sprinkled all about in random places quite hard to read -- I like it to be contained at one or two "pinch points" or not used at all, so in the most recent commit I removed the #[cfg(debug_assertions)] on the forbid method and replaced it with an assert!. But I'm not totally happy with that.

I'm not sure what's a better way to formulate the code here. It occurs to me now I could maybe make a mod forbid that is different if debug-assertions are disabled, so that at least all the cond code can be contained in one place, but it still feels like a lot for such a small thing.

nikomatsakis · 2016-09-02T09:04:09Z

again the travis issue looks legit, and again I wonder why I am not seeing this locally!

Oh, I am dumb, it's because I'm building with debug-assertions enabled, of course!

nikomatsakis · 2016-09-02T11:49:51Z

OK, so, I wound up implementing a better mechanism altogether. I'll push a few more commits shortly.

nikomatsakis · 2016-09-02T15:56:46Z

r? @michaelwoerister -- this fixes one add'l bug, and revamps the forbidden stuff into something more powerful.

bors · 2016-09-05T00:19:02Z

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

The goal here is to avoid writing to the `inherent_impls` map from within the general `Coherence` task, and instead write to it as we visit. Writing to it from the Coherence task is actually an information leak; it happened to be safe because Coherence read from `DepNode::Krate`, but that was very coarse. I removed the `Rc` here because, upon manual inspection, nobody clones the data in this table, and it meant that we can accumulate the data in place. That said, the pattern that is used for the inherent impls map is *generally* an anti-pattern (that is, holding the borrow lock for the duration of using the contents), so it'd probably be better to clone (and I doubt that would be expensive -- how many inherent impls does a typical type have?).

We touch the krate to adjust the maps, but we don't expose that data widely.

It is useful to track down an errant edge that is being added. This is not a perfect mechanism, since it doesn't consider (e.g.) if we are in an ignored task, but it's helpful enough for now.

It was duplicating information available elsewhere.

The shadow graph supercedes the existing code that checked for reads/writes without an active task and now adds the ability to filter for specific edges.

Across crates only, converting a def-id into its def-key or def-path was considered a read. This caused spurious reads when computing the symbol name for some item.

supplanted by RUST_FORBID_DEP_GRAPH_EDGE

I also added an `opt_def_path` so that we can deal with DefIds that are missing a `DefPath` entry.

add licenses to shadow.rs

michaelwoerister · 2016-09-06T15:39:35Z

src/librustc/dep_graph/README.md

+
+That first edge looks suspicious to you. So you set
+`RUST_FORBID_DEP_GRAPH_EDGE` to `Hir&foo -> Collect&bar`, re-run, and
+then observe the backtrace. Voila, bug fixed!


michaelwoerister · 2016-09-12T22:26:42Z

@bors r+

bors · 2016-09-12T22:26:44Z

📌 Commit 9ca5786 has been approved by michaelwoerister

bors · 2016-09-13T00:15:26Z

⌛ Testing commit 9ca5786 with merge fa9d8cc...

@michaelwoerister

…oerister fix a few errant `Krate` edges Exploring the effect of small changes on `syntex` reuse, I discovered the following sources of unnecessary edges from `Krate` r? @michaelwoerister

bors · 2016-09-13T03:33:45Z

rust-highfive assigned michaelwoerister Aug 24, 2016

nagisa reviewed Aug 24, 2016
View reviewed changes

nikomatsakis force-pushed the incr-comp-krate-edges branch from e08d4fb to 30c6f12 Compare August 31, 2016 19:19

nikomatsakis force-pushed the incr-comp-krate-edges branch from 30c6f12 to 386abe1 Compare September 1, 2016 10:08

nikomatsakis force-pushed the incr-comp-krate-edges branch from d287b7a to 3b4e587 Compare September 2, 2016 08:55

nikomatsakis force-pushed the incr-comp-krate-edges branch 2 times, most recently from 54f8bdf to cf7fd6b Compare September 2, 2016 15:55

nikomatsakis force-pushed the incr-comp-krate-edges branch from 2d893b3 to bf27870 Compare September 4, 2016 11:55

nikomatsakis added 4 commits September 6, 2016 11:17

ignore dep-graph when loading inlined HIR

c6363b8

We touch the krate to adjust the maps, but we don't expose that data widely.

add a debugging mechanism to forbid edges

753590f

It is useful to track down an errant edge that is being added. This is not a perfect mechanism, since it doesn't consider (e.g.) if we are in an ignored task, but it's helpful enough for now.

allow testing DepNode::Krate edges directly

2a84449

nikomatsakis added 7 commits September 6, 2016 11:18

remove the "misc-items" from meta-data

4c2f3ff

It was duplicating information available elsewhere.

implement a debugging "shadow graph"

2f91ba0

The shadow graph supercedes the existing code that checked for reads/writes without an active task and now adds the ability to filter for specific edges.

expanding a def-id is not a read

fe6557e

Across crates only, converting a def-id into its def-key or def-path was considered a read. This caused spurious reads when computing the symbol name for some item.

kill the forbidden code

07df812

supplanted by RUST_FORBID_DEP_GRAPH_EDGE

kill extra use

2446e25

always print def-path in Debug impl for DefId

dadce25

I also added an `opt_def_path` so that we can deal with DefIds that are missing a `DefPath` entry.

pacify the mercilous tidy

c2ffa2f

add licenses to shadow.rs

nikomatsakis force-pushed the incr-comp-krate-edges branch from bf27870 to c2ffa2f Compare September 6, 2016 15:28

michaelwoerister reviewed Sep 6, 2016
View reviewed changes

check stack discipline of tasks

9ca5786

bors merged commit 9ca5786 into rust-lang:master Sep 13, 2016

This was referenced Sep 13, 2016

ICH: Add ability to test the ICH of exported metadata items. #36370

Merged

Remove some obsolete code from the compiler #36393

Merged

nikomatsakis deleted the incr-comp-krate-edges branch October 3, 2016 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix a few errant `Krate` edges #35960

fix a few errant `Krate` edges #35960

nikomatsakis commented Aug 24, 2016

nagisa Aug 24, 2016

michaelwoerister commented Aug 24, 2016

arielb1 commented Aug 25, 2016 •

edited

Loading

bors commented Aug 27, 2016

nikomatsakis commented Aug 31, 2016

bors commented Aug 31, 2016

nikomatsakis commented Sep 1, 2016

nikomatsakis commented Sep 1, 2016 •

edited

Loading

nikomatsakis commented Sep 1, 2016

bors commented Sep 1, 2016

nikomatsakis commented Sep 2, 2016

nikomatsakis commented Sep 2, 2016

nikomatsakis commented Sep 2, 2016

nikomatsakis commented Sep 2, 2016

nikomatsakis commented Sep 2, 2016

bors commented Sep 5, 2016

michaelwoerister Sep 6, 2016

michaelwoerister commented Sep 12, 2016

bors commented Sep 12, 2016

bors commented Sep 13, 2016

bors commented Sep 13, 2016

fix a few errant Krate edges #35960

fix a few errant Krate edges #35960

Conversation

nikomatsakis commented Aug 24, 2016

nagisa Aug 24, 2016

Choose a reason for hiding this comment

michaelwoerister commented Aug 24, 2016

arielb1 commented Aug 25, 2016 • edited Loading

bors commented Aug 27, 2016

nikomatsakis commented Aug 31, 2016

bors commented Aug 31, 2016

nikomatsakis commented Sep 1, 2016

nikomatsakis commented Sep 1, 2016 • edited Loading

nikomatsakis commented Sep 1, 2016

bors commented Sep 1, 2016

nikomatsakis commented Sep 2, 2016

nikomatsakis commented Sep 2, 2016

nikomatsakis commented Sep 2, 2016

nikomatsakis commented Sep 2, 2016

nikomatsakis commented Sep 2, 2016

bors commented Sep 5, 2016

michaelwoerister Sep 6, 2016

Choose a reason for hiding this comment

michaelwoerister commented Sep 12, 2016

bors commented Sep 12, 2016

bors commented Sep 13, 2016

bors commented Sep 13, 2016

fix a few errant `Krate` edges #35960

fix a few errant `Krate` edges #35960

arielb1 commented Aug 25, 2016 •

edited

Loading

nikomatsakis commented Sep 1, 2016 •

edited

Loading