Address ICEs running w/ incremental compilation and building glium by nikomatsakis · Pull Request #35166 · rust-lang/rust

nikomatsakis · 2016-08-01T18:18:59Z

Fixes for various ICEs I encountered trying to build glium with incremental compilation enabled. Building glium now works. Of the 4 ICEs, I have test cases for 3 of them -- I didn't isolate a test for the last commit and kind of want to go do other things -- most notably, figuring out why incremental isn't saving much effort.

But if it seems worthwhile and I can come back and try to narrow down the problem.

r? @michaelwoerister

Fixes#34991
Fixes#32015

bors · 2016-08-01T22:30:28Z

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

michaelwoerister · 2016-08-02T08:10:10Z

TyCtxt::crate_name() should already take care of the local/extern crate switch.

michaelwoerister · 2016-08-02T08:39:10Z

I'm not sure that I like how the Hir/Metadata problem is solved here via the graph post-processing step. It means that during the current compilation session, we still have a graph missing edges, right? Shouldn't you run into the def_id.is_local() assertion HirMap::hir_hash() when computing the Metadata hash for exported things depending on inlined HIR? And if you don't, isn't that fishy?

The way we do HIR inlining introduces reads of the "Hir" into the graph, but this Hir in fact belongs to other crates, so when we try to load later, we ICE because the Hir nodes in question don't belond to the crate (and we haven't done inlining yet). This pass rewrites those HIR nodes to the metadata from which the inlined HIR was loaded.

The biggest problem, actually, is krate numbers being removed entirely, which can lead to array-index-out-of-bounds errors. cc rust-lang#35123 -- not a complete fix, since really we ought to "map" the old crate numbers to the new ones, not just detect changes.

The reads will occur naturally as the HIR/MIR is fetched from the tracked tables, and this winds up adding reads to the hir of foreign def-ids somehow.

When we hash the inputs to a MetaData node, we have to hash them in a consistent order. We achieve this by sorting the stringfied `DefPath` entries. Also, micro-optimie by cache more results across the saving process.

It's nice to get a rough idea of how much work we're saving.

nikomatsakis · 2016-08-02T14:24:38Z

It means that during the current compilation session, we still have a graph missing edges, right?

As we said on IRC, I will investigate the alternate approach of "detecting" inlined ids and remapping more eagerly.

We now detect inlined id's earlier (in the HIR map) and rewrite a read of them to be a read of the metadata for the associated item.

nikomatsakis · 2016-08-02T20:40:15Z

@michaelwoerister see last commit, which takes a different strategy that also seems to work.

I cannot figure out how to write a test for this, but I observed incorrect edges as a result of not using memoized pattern here (e.g., LateLintCheck -> SizedConstraint).

this can actually be expensive!

michaelwoerister · 2016-08-05T20:23:42Z

+}
+
+#[derive(Debug, RustcEncodable, RustcDecodable)]
+pub struct KrateInfo {


Why KrateInfo instead of CrateInfo?

Why KrateInfo instead of CrateInfo?
Because crate is a keyword... but yeah I realize now we often use C when we can (personally I would just always use K...).

michaelwoerister · 2016-08-05T20:56:20Z

Looks good to me now! I think the new metadata tracking strategy is a clear improvement over the graph post processing.

nikomatsakis · 2016-08-06T09:37:09Z

@michaelwoerister do you want me to rename the uses of KrateInfo to CrateInfo (there are also some other cases, like krate_info(), in that file, so I guess I should do them all)?

~~(Also, I see travis failed, but I couldn't quite see why -- trying a full make check locally.)~~ found it

michaelwoerister · 2016-08-08T12:53:03Z

do you want me to rename the uses of KrateInfo to CrateInfo (there are also some other cases, like krate_info(), in that file, so I guess I should do them all)?

Yes, please.

Per the discussion on rust-lang#34765, we make one `DepNode::Mir` variant and use it to represent both the MIR tracking map as well as passes that operate on MIR. We also track loads of cached MIR (which naturally comes from metadata). Note that the "HAIR" pass adds a read of TypeckItemBody because it uses a myriad of tables that are not individually tracked.

it now carries a def-id; supply a dummy

nikomatsakis · 2016-08-09T01:41:10Z

@michaelwoerister ok, to fixup tests, I wound up fixing #34765 -- care to review last few commits :)

The new `Predecessors` type computes a set of interesting targets and their HIR predecessors, and discards everything in between.

Fixesrust-lang#35292.

Produces a deterministic hash, at least for a single platform / compiler-version.

This massively speeds up serialization. It also seems to produce deterministic metadata hashes (before I was seeing inconsistent results). Fixesrust-lang#35232.

michaelwoerister · 2016-08-09T14:02:59Z

@@ -538,8 +551,7 @@ impl<'b, 'tcx> SharedCrateContext<'b, 'tcx> {

 pub fn get_mir(&self, def_id: DefId) -> Option<CachedMir<'b, 'tcx>> {


This could probably use the memoize functions.

michaelwoerister · 2016-08-09T14:05:05Z

@bors r+

bors · 2016-08-09T14:05:07Z

📌 Commit 82b6dc2 has been approved by michaelwoerister

nikomatsakis · 2016-08-09T14:38:11Z

@bors r=mw

bors · 2016-08-09T14:38:12Z

📌 Commit 76eecc7 has been approved by mw

bors · 2016-08-09T15:52:20Z

⌛ Testing commit 76eecc7 with merge d7012c1...

nikomatsakis · 2016-08-09T17:00:38Z

@bors r=mw force

bors · 2016-08-09T17:00:47Z

📌 Commit e0b82d5 has been approved by mw

bors · 2016-08-09T17:00:55Z

⌛ Testing commit e0b82d5 with merge e1d2bc2...

@michaelwoerister

Address ICEs running w/ incremental compilation and building glium Fixes for various ICEs I encountered trying to build glium with incremental compilation enabled. Building glium now works. Of the 4 ICEs, I have test cases for 3 of them -- I didn't isolate a test for the last commit and kind of want to go do other things -- most notably, figuring out why incremental isn't saving much *effort*. But if it seems worthwhile and I can come back and try to narrow down the problem. r? @michaelwoeristerFixes#34991Fixes#32015

bors · 2016-08-09T21:04:52Z

rust-highfive assigned michaelwoerister Aug 1, 2016

hash foreign items too
9294f8e

michaelwoerister reviewed Aug 2, 2016
View reviewed changes

nikomatsakis added 5 commits August 2, 2016 05:31

remove register_reads
2797b2a
The reads will occur naturally as the HIR/MIR is fetched from the tracked tables, and this winds up adding reads to the hir of foreign def-ids somehow.

make metadata hashes determinstic
2e7df80
When we hash the inputs to a MetaData node, we have to hash them in a consistent order. We achieve this by sorting the stringfied `DefPath` entries. Also, micro-optimie by cache more results across the saving process.

dump statistics about re-use w/ -Z time-passes
903142a
It's nice to get a rough idea of how much work we're saving.

nikomatsakis force-pushed the incr-comp-ice-34991-2 branch from a11a4e2 to 903142aCompare August 2, 2016 09:45

replace graph rewriting with detecting inlined ids
94acff1
We now detect inlined id's earlier (in the HIR map) and rewrite a read of them to be a read of the metadata for the associated item.

nikomatsakis added 4 commits August 2, 2016 19:11

improve log when something no longer exists
b13d504

use memoized pattern for SizedConstraint
54595ec
I cannot figure out how to write a test for this, but I observed incorrect edges as a result of not using memoized pattern here (e.g., LateLintCheck -> SizedConstraint).

skip assert-dep-graph unless unit testing
bfbfe63
this can actually be expensive!

rustfmt save.rs
a6a97a9

michaelwoerister reviewed Aug 5, 2016
View reviewed changes

nikomatsakis added 3 commits August 8, 2016 18:41

rename KrateInfo to CrateInfo
88b2e9a

fixup tests for new def'n of InlinedItem
82b6dc2
it now carries a def-id; supply a dummy

nikomatsakis added 8 commits August 9, 2016 08:26

isolate predecessor computation
0e97240
The new `Predecessors` type computes a set of interesting targets and their HIR predecessors, and discards everything in between.

make DepNode PartialOrd
a92b1a7

replace Name with InternedString in DefPathData
571010b
Fixesrust-lang#35292.

add a -Z incremental-info flag
d4bd054

add a deterministic_hash method to DefPath
8150494
Produces a deterministic hash, at least for a single platform / compiler-version.

generalize BitMatrix to be NxM and not just NxN
9978cbc

use preds to serialize just what we need
02a4703
This massively speeds up serialization. It also seems to produce deterministic metadata hashes (before I was seeing inconsistent results). Fixesrust-lang#35232.

address comments from mw
ecbcf1b

nikomatsakis mentioned this pull request Aug 9, 2016
Optimize dep-graph serialization and improve meta-data hashing #35406
Merged

michaelwoerister reviewed Aug 9, 2016
View reviewed changes

pacify the mercilous tidy
76eecc7

fix license
e0b82d5

bors merged commit e0b82d5 into rust-lang:masterAug 9, 2016

This was referenced Aug 9, 2016
Take commandline arguments into account for incr. comp. #35340
Merged
Implement impl Trait in return type position by anonymization. #35091
Merged
Implement the ! type #35162
Merged

nikomatsakis mentioned this pull request Aug 10, 2016
incr. comp.: Clarify story around MIR dependency tracking #34765
Closed

nikomatsakis deleted the incr-comp-ice-34991-2 branch October 3, 2016 14:55

		@@ -538,8 +551,7 @@ impl<'b, 'tcx> SharedCrateContext<'b, 'tcx> {

		pub fn get_mir(&self, def_id: DefId) -> Option<CachedMir<'b, 'tcx>> {

Uh oh!

Uh oh!

Conversation

nikomatsakis commented Aug 1, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Aug 1, 2016

Uh oh!

michaelwoeristerAug 2, 2016

Choose a reason for hiding this comment

Uh oh!

michaelwoerister commented Aug 2, 2016

Uh oh!

nikomatsakis commented Aug 2, 2016

Uh oh!

nikomatsakis commented Aug 2, 2016

Uh oh!

michaelwoeristerAug 5, 2016

Choose a reason for hiding this comment

Uh oh!

nikomatsakisAug 6, 2016

Choose a reason for hiding this comment

Uh oh!

michaelwoerister commented Aug 5, 2016

Uh oh!

nikomatsakis commented Aug 6, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelwoerister commented Aug 8, 2016

Uh oh!

nikomatsakis commented Aug 9, 2016

Uh oh!

michaelwoeristerAug 9, 2016

Choose a reason for hiding this comment

Uh oh!

michaelwoerister commented Aug 9, 2016

Uh oh!

bors commented Aug 9, 2016

Uh oh!

nikomatsakis commented Aug 9, 2016

Uh oh!

bors commented Aug 9, 2016

Uh oh!

bors commented Aug 9, 2016

Uh oh!

nikomatsakis commented Aug 9, 2016

Uh oh!

bors commented Aug 9, 2016

Uh oh!

bors commented Aug 9, 2016

Uh oh!

bors commented Aug 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nikomatsakis commented Aug 1, 2016•
edited
Loading

nikomatsakis commented Aug 6, 2016•
edited
Loading