ICH: Replace old, transitive metadata hashing with direct hashing approach. #41141

michaelwoerister · 2017-04-07T14:37:04Z

This PR replaces the old crate metadata hashing strategy with a new one that directly (but stably) hashes all values we encode into the metadata. Previously we would track what data got accessed during metadata encoding and then hash the input nodes (HIR and upstream metadata) that were transitively reachable from the accessed data. While this strategy was sound, it had two major downsides:

It was susceptible to generating false positives, i.e. some input node might have changed without actually affecting the content of the metadata. That metadata entry would still show up as changed.
It was susceptible to quadratic blow-up when many metadata nodes shared the same input nodes, which would then get hashed over and over again.

The new method does not have these disadvantages and it's also a first step towards caching more intermediate results in the compiler.

Metadata hashing/cross-crate incremental compilation is still kept behind the -Zincremental-cc flag even after this PR. Once the new method has proven itself with more tests, we can remove the flag and enable cross-crate support by default again.

r? @nikomatsakis
cc @rust-lang/compiler

eddyb · 2017-04-07T18:18:13Z

src/librustc_metadata/index_builder.rs

+    {
+        if let Some((ref mut hcx, ref mut hasher)) = self.hcx {
+            let items: Vec<T> = iter.into_iter().collect();
+            items.hash_stable(hcx, hasher);


Wait, why would this be needed? Can't you do it with an iterator adapter?

Because we want to hash the length of the sequence before its elements. But maybe I could only copy out into a Vec if size_hint() is ambiguous...

Ugh, that is annoying. It's not the only way to hash a sequence (you could imagine having 1, element, 1, element, ..., 1, element, 0 or something along those lines), but, yeah, size_hint. It might not be available in a few cases, but IIRC we have to collect anyway for a different reason.
Btw, if you do use size_hint, assert that it matches up.

eddyb · 2017-04-07T18:18:36Z

src/librustc_metadata/schema.rs

+                                          _: &mut CTX,
+                                          _: &mut StableHasher<W>) {
+            // There's nothing to do. Whatever got encoded within this Lazy<>
+            // wrapper has already been hashed.


Strange indentation (here and elsewhere).

I'll fix that.

bors · 2017-04-07T18:20:43Z

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

michaelwoerister · 2017-04-10T09:50:25Z

Rebased and @eddyb's concerns addressed (but see 8a96ad3#diff-f55f5d2e86bc0e08c341b51fc78b27e4R1162, not sure if that's an improvement).

eddyb · 2017-04-10T10:12:25Z

src/librustc_metadata/index_builder.rs

+                lower_bound.hash_stable(hcx, hasher);
+                let ret = self.ecx.lazy_seq(iter.inspect(|item| item.hash_stable(hcx, hasher)));
+                debug!("metadata-hash: {:?}", hasher);
+                ret


You should assert that the number of times the inspect callback gets called is equal to lower_bound.

nikomatsakis

I didn't finish this review but here were some (outdated-ish) comments

nikomatsakis · 2017-04-07T15:50:21Z

src/librustc/ich/impls_ty.rs

+
+            // The other fields just provide fast access to information that is
+            // also contained in `sty`, so no need to hash them.
+            ..


I'd prefer to write out the names of those fields explicitly. My test for this is usually "if I added a new field, is it possible that it would require changes here?" And it seems plausible, though unlikely, that the answer would be "yes" in the future.

I wanted to do that but one of the fields is private. I guess I have to bite the bullet and move the impl to the defining module.

nikomatsakis · 2017-04-07T15:50:53Z

src/librustc/ich/impls_ty.rs

+            }
+
+            TyError     |
+            TyInfer(..) => {


Horray for being exhaustive :)

nikomatsakis · 2017-04-07T15:52:25Z

src/librustc/ich/impls_ty.rs

+            ref fru_field_types,
+
+            ref cast_kinds,
+            lints: _,


It seems... fine to skip these, at least for the purpose of metadata hashing, but not obviously fine for other possible uses (i.e., the red/green incremental algorithm). However, by then I hope that we've refactored lints into "just another case of diagnostic output". Maybe a FIXME pointing at ... idk, some issue? Perhaps not necessary.

I didn't want to spend time on something that does not make a difference now and will probably change in the future, but creating a FIXME issue is definitely a good idea.

nikomatsakis · 2017-04-08T08:34:02Z

src/librustc_data_structures/stable_hasher.rs

@@ -96,6 +149,7 @@ impl<W> StableHasher<W> {
        let mut buf = [0; 16];
        let len = write_unsigned_leb128_to_buf(&mut buf, value);
        self.state.write(&buf[..len]);
+        self.debug.write(&buf[..len]);


Would it be possible to replace self.state with (in debug mode) a wrapper? This seems good but a bit easy to overlook.

I'll look into it. Sounds like an improvement.

michaelwoerister · 2017-04-10T14:07:26Z

All comments so far are addressed now, I think.

nikomatsakis

So this looks pretty good but I think there is one simplification that can still be made. In particular, when we do the graph compression, we consider metadata nodes as "outputs". This means that graph compression will ensure that we track their (transitive) inputs. I think that with this PR we do not need to consider those metadata nodes as outputs, since we no longer need to enumerate their transitive inputs.

michaelwoerister · 2017-04-11T08:08:00Z

With the changes here, we don't create output metadata nodes at all anymore, so we never should hit the code path you linked. But I'll clean it up, make it explicit that we don't expect to find these kinds of nodes.

In the future, if we ever implement a way to update metadata incrementally and in place (and there are some compelling reasons for which I should write up in an issue), we might want to start tracking output metadata nodes again.

nikomatsakis · 2017-04-11T15:03:10Z

In the future, if we ever implement a way to update metadata incrementally and in place (and there are some compelling reasons for which I should write up in an issue), we might want to start tracking output metadata nodes again.

Yes.

nikomatsakis · 2017-04-11T15:04:01Z

@bors r+

bors · 2017-04-11T15:04:02Z

📌 Commit d78453f has been approved by nikomatsakis

bors · 2017-04-12T04:03:21Z

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

…evisions.

…roach. Instead of collecting all potential inputs to some metadata entry and hashing those, we directly hash the values we are storing in metadata. This is more accurate and doesn't suffer from quadratic blow-up when many entries have the same dependencies.

michaelwoerister · 2017-04-12T09:48:39Z

@bors r=nikomatsakis

Rebased.

bors · 2017-04-12T09:48:40Z

📌 Commit ca2dce9 has been approved by nikomatsakis

@nikomatsakis

…h-final, r=nikomatsakis ICH: Replace old, transitive metadata hashing with direct hashing approach. This PR replaces the old crate metadata hashing strategy with a new one that directly (but stably) hashes all values we encode into the metadata. Previously we would track what data got accessed during metadata encoding and then hash the input nodes (HIR and upstream metadata) that were transitively reachable from the accessed data. While this strategy was sound, it had two major downsides: 1. It was susceptible to generating false positives, i.e. some input node might have changed without actually affecting the content of the metadata. That metadata entry would still show up as changed. 2. It was susceptible to quadratic blow-up when many metadata nodes shared the same input nodes, which would then get hashed over and over again. The new method does not have these disadvantages and it's also a first step towards caching more intermediate results in the compiler. Metadata hashing/cross-crate incremental compilation is still kept behind the `-Zincremental-cc` flag even after this PR. Once the new method has proven itself with more tests, we can remove the flag and enable cross-crate support by default again. r? @nikomatsakis cc @rust-lang/compiler

Rollup of 9 pull requests - Successful merges: #41063, #41087, #41141, #41166, #41183, #41205, #41206, #41232, #41243 - Failed merges:

rust-highfive assigned nikomatsakis Apr 7, 2017

eddyb reviewed Apr 7, 2017

View reviewed changes

michaelwoerister force-pushed the direct-metadata-ich-final branch from ae57ed1 to 8a96ad3 Compare April 10, 2017 09:48

eddyb reviewed Apr 10, 2017

View reviewed changes

michaelwoerister force-pushed the direct-metadata-ich-final branch 2 times, most recently from 6dd5053 to b41d98d Compare April 10, 2017 12:07

nikomatsakis reviewed Apr 10, 2017

View reviewed changes

michaelwoerister mentioned this pull request Apr 10, 2017

incr.comp.: Lints and other error messages are not included in the ICH #41184

Closed

michaelwoerister force-pushed the direct-metadata-ich-final branch from b41d98d to c2dcf2a Compare April 10, 2017 14:06

nikomatsakis requested changes Apr 10, 2017

View reviewed changes

michaelwoerister force-pushed the direct-metadata-ich-final branch from c2dcf2a to d78453f Compare April 11, 2017 09:11

nikomatsakis approved these changes Apr 11, 2017

View reviewed changes

michaelwoerister added 3 commits April 12, 2017 11:42

Make compiletest write test output to different files for different r…

c008cd7

…evisions.

ICH: Hash everything that gets encoded into crate metadata.

bc7af81

michaelwoerister force-pushed the direct-metadata-ich-final branch from d78453f to ca2dce9 Compare April 12, 2017 09:47

TimNN mentioned this pull request Apr 12, 2017

Rollup of 9 pull requests #41246

Merged

bors added a commit that referenced this pull request Apr 12, 2017

Auto merge of #41246 - TimNN:rollup, r=TimNN

910c481

Rollup of 9 pull requests - Successful merges: #41063, #41087, #41141, #41166, #41183, #41205, #41206, #41232, #41243 - Failed merges:

bors merged commit ca2dce9 into rust-lang:master Apr 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICH: Replace old, transitive metadata hashing with direct hashing approach. #41141

ICH: Replace old, transitive metadata hashing with direct hashing approach. #41141

michaelwoerister commented Apr 7, 2017

eddyb Apr 7, 2017

michaelwoerister Apr 7, 2017

eddyb Apr 7, 2017 •

edited

Loading

eddyb Apr 7, 2017

michaelwoerister Apr 7, 2017

bors commented Apr 7, 2017

michaelwoerister commented Apr 10, 2017

eddyb Apr 10, 2017

michaelwoerister Apr 10, 2017

michaelwoerister Apr 10, 2017

nikomatsakis left a comment

nikomatsakis Apr 7, 2017

michaelwoerister Apr 10, 2017

nikomatsakis Apr 7, 2017

nikomatsakis Apr 7, 2017

michaelwoerister Apr 10, 2017

nikomatsakis Apr 8, 2017

michaelwoerister Apr 10, 2017

michaelwoerister commented Apr 10, 2017

nikomatsakis left a comment

michaelwoerister commented Apr 11, 2017

nikomatsakis commented Apr 11, 2017

nikomatsakis commented Apr 11, 2017

bors commented Apr 11, 2017

bors commented Apr 12, 2017

michaelwoerister commented Apr 12, 2017

bors commented Apr 12, 2017

ICH: Replace old, transitive metadata hashing with direct hashing approach. #41141

ICH: Replace old, transitive metadata hashing with direct hashing approach. #41141

Conversation

michaelwoerister commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 7, 2017

michaelwoerister commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelwoerister commented Apr 10, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

michaelwoerister commented Apr 11, 2017

nikomatsakis commented Apr 11, 2017

nikomatsakis commented Apr 11, 2017

bors commented Apr 11, 2017

bors commented Apr 12, 2017

michaelwoerister commented Apr 12, 2017

bors commented Apr 12, 2017

eddyb Apr 7, 2017 •

edited

Loading