Introduce HashStable trait and base ICH implementations on it. #40878

michaelwoerister · 2017-03-28T15:15:18Z

This PR introduces the HashStable trait which marks that a type can be hashed in a way that is stable across multiple compilation sessions. The PR also moves HIR incr. comp. hashing over to implementations of this trait instead of doing this via a HIR visitor. It also provides many HashStable implementations that are not used yet (e.g. for MIR types) but soon will be used when we directly hash crate metadata for incr. comp.

I've only done superficial performance measurements but it looks like the new implementation is a bit faster than the current one (due, I suppose, to some bugs I fixed and some unnecessary inefficiencies I removed). Here is the time in seconds for the compute_incremental_hashes_map pass for various crates:

	OLD	NEW
libcore	0.507	0.409
libsyntax	0.320	0.260
librustc	0.730	0.611
librustc_driver	0.024	0.015

Some notes regarding the implementation:

Most HashStable implementations are provided via the impl_hash_stable_for! macro (as suggested by @nikomatsakis). This works out quite well. A custom_derive would have been better but Macros 1.1 are not available in the compiler.
The trait implementation take care to exhaustively destructure everything they hash so that fields added in the future don't fall through the cracks. This is a bit verbose but I think it's well worth the trouble since we've had quite a few issues with missing fields or visitor callbacks in this area in the past. Most of it is behind the macro anyway.

cc @rust-lang/compiler
r? @nikomatsakis

michaelwoerister · 2017-03-29T08:48:17Z

Those travis errors seem to indicate that something went wrong when rebasing ontop of the catch changes. Looking into it...

michaelwoerister · 2017-03-29T09:54:01Z

So, I can reliably reproduce these errors but I can't see how the changes in this PR would have an effect like that :/

michaelwoerister · 2017-03-29T13:01:53Z

OK, the problem was that those tests where exercising code paths that would try to hash the DefPath of NodeIds that don't have a corresponding DefId.

bors · 2017-03-30T09:29:48Z

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

michaelwoerister · 2017-03-30T13:05:30Z

This latest error is legitimate (the SVH is more sensitive than it needs to be) and I figured out that impl specialization offers a way of solving this in a nice and general way, that also allows for reverting the splitting-out of dep_graph_assert attrs. Please hold off on reviewing until I've done that.

michaelwoerister · 2017-03-30T14:46:01Z

Alright, this passes make check for me now and the implementation is simpler too. Hurray specialization!

nikomatsakis · 2017-04-03T19:44:55Z

@bors r+

bors · 2017-04-03T19:44:56Z

📌 Commit 7684437 has been approved by nikomatsakis

bors · 2017-04-03T19:50:17Z

⌛ Testing commit 7684437 with merge 730547d...

bors · 2017-04-03T19:58:12Z

💔 Test failed - status-travis

alexcrichton · 2017-04-03T22:15:41Z

@bors: retry * centos network issues #40474

…

On Mon, Apr 3, 2017 at 1:58 PM, bors ***@***.***> wrote: 💔 Test failed - status-travis <https://travis-ci.org/rust-lang/rust/builds/218207848> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#40878 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95Hw_765vUX8-pAsyVO_JYkh0wlPFks5rsU9WgaJpZM4MrxhQ> .

@nikomatsakis

Introduce HashStable trait and base ICH implementations on it. This PR introduces the `HashStable` trait which marks that a type can be hashed in a way that is stable across multiple compilation sessions. The PR also moves HIR incr. comp. hashing over to implementations of this trait instead of doing this via a HIR visitor. It also provides many `HashStable` implementations that are not used yet (e.g. for MIR types) but soon will be used when we directly hash crate metadata for incr. comp. I've only done superficial performance measurements but it looks like the new implementation is a bit faster than the current one (due, I suppose, to some bugs I fixed and some unnecessary inefficiencies I removed). Here is the time in seconds for the `compute_incremental_hashes_map` pass for various crates: | | OLD | NEW | |:---------------:|:-----:|:-----:| | libcore | 0.507 | 0.409 | | libsyntax | 0.320 | 0.260 | | librustc | 0.730 | 0.611 | | librustc_driver | 0.024 | 0.015 | Some notes regarding the implementation: * Most `HashStable` implementations are provided via the `impl_hash_stable_for!` macro (as suggested by @nikomatsakis). This works out quite well. A custom_derive would have been better but Macros 1.1 are not available in the compiler. * The trait implementation take care to exhaustively destructure everything they hash so that fields added in the future don't fall through the cracks. This is a bit verbose but I think it's well worth the trouble since we've had quite a few issues with missing fields or visitor callbacks in this area in the past. Most of it is behind the macro anyway. cc @rust-lang/compiler r? @nikomatsakis

frewsxcv · 2017-04-06T03:51:04Z

Ran into an issue with this PR in the rollup

https://travis-ci.org/rust-lang/rust/jobs/219131927

@bors r-

nikomatsakis · 2017-04-06T12:51:08Z

@michaelwoerister

Looks like a minor merge thing-y:

[00:06:09] error[E0026]: struct `hir::Block` does not have a field named `break_to_expr_id`
[00:06:09]    --> /checkout/src/librustc/ich/impls_hir.rs:347:13
[00:06:09]     |
[00:06:09] 347 |             break_to_expr_id,
[00:06:09]     |             ^^^^^^^^^^^^^^^^ struct `hir::Block` does not have field `break_to_expr_id`
[00:06:09] 
[00:06:09] error[E0027]: pattern does not mention field `targeted_by_break`
[00:06:09]    --> /checkout/src/librustc/ich/impls_hir.rs:341:13
[00:06:09]     |
[00:06:09] 341 |           let hir::Block {
[00:06:09]     |  _____________^ starting here...
[00:06:09] 342 | |             ref stmts,
[00:06:09] 343 | |             ref expr,
[00:06:09] 344 | |             id,
[00:06:09] 345 | |             rules,
[00:06:09] 346 | |             span,
[00:06:09] 347 | |             break_to_expr_id,
[00:06:09] 348 | |         } = *self;
[00:06:09]     | |_________^ ...ending here: missing field `targeted_by_break`
[00:06:09] 
[00:06:15] error: aborting due to 2 previous errors

nikomatsakis · 2017-04-06T12:51:31Z

In particular I renamed break_to_expr_id to targeted_by_break

This initial commit provides implementations for HIR, MIR, and everything that also needs to be supported for those two.

michaelwoerister · 2017-04-06T14:02:44Z

@bors r=nikomatsakis

Rebased.

bors · 2017-04-06T14:02:45Z

📌 Commit c47cdc0 has been approved by nikomatsakis

@nikomatsakis

Introduce HashStable trait and base ICH implementations on it. This PR introduces the `HashStable` trait which marks that a type can be hashed in a way that is stable across multiple compilation sessions. The PR also moves HIR incr. comp. hashing over to implementations of this trait instead of doing this via a HIR visitor. It also provides many `HashStable` implementations that are not used yet (e.g. for MIR types) but soon will be used when we directly hash crate metadata for incr. comp. I've only done superficial performance measurements but it looks like the new implementation is a bit faster than the current one (due, I suppose, to some bugs I fixed and some unnecessary inefficiencies I removed). Here is the time in seconds for the `compute_incremental_hashes_map` pass for various crates: | | OLD | NEW | |:---------------:|:-----:|:-----:| | libcore | 0.507 | 0.409 | | libsyntax | 0.320 | 0.260 | | librustc | 0.730 | 0.611 | | librustc_driver | 0.024 | 0.015 | Some notes regarding the implementation: * Most `HashStable` implementations are provided via the `impl_hash_stable_for!` macro (as suggested by @nikomatsakis). This works out quite well. A custom_derive would have been better but Macros 1.1 are not available in the compiler. * The trait implementation take care to exhaustively destructure everything they hash so that fields added in the future don't fall through the cracks. This is a bit verbose but I think it's well worth the trouble since we've had quite a few issues with missing fields or visitor callbacks in this area in the past. Most of it is behind the macro anyway. cc @rust-lang/compiler r? @nikomatsakis

@nikomatsakis

Introduce HashStable trait and base ICH implementations on it. This PR introduces the `HashStable` trait which marks that a type can be hashed in a way that is stable across multiple compilation sessions. The PR also moves HIR incr. comp. hashing over to implementations of this trait instead of doing this via a HIR visitor. It also provides many `HashStable` implementations that are not used yet (e.g. for MIR types) but soon will be used when we directly hash crate metadata for incr. comp. I've only done superficial performance measurements but it looks like the new implementation is a bit faster than the current one (due, I suppose, to some bugs I fixed and some unnecessary inefficiencies I removed). Here is the time in seconds for the `compute_incremental_hashes_map` pass for various crates: | | OLD | NEW | |:---------------:|:-----:|:-----:| | libcore | 0.507 | 0.409 | | libsyntax | 0.320 | 0.260 | | librustc | 0.730 | 0.611 | | librustc_driver | 0.024 | 0.015 | Some notes regarding the implementation: * Most `HashStable` implementations are provided via the `impl_hash_stable_for!` macro (as suggested by @nikomatsakis). This works out quite well. A custom_derive would have been better but Macros 1.1 are not available in the compiler. * The trait implementation take care to exhaustively destructure everything they hash so that fields added in the future don't fall through the cracks. This is a bit verbose but I think it's well worth the trouble since we've had quite a few issues with missing fields or visitor callbacks in this area in the past. Most of it is behind the macro anyway. cc @rust-lang/compiler r? @nikomatsakis

bors · 2017-04-06T19:55:22Z

⌛ Testing commit c47cdc0 with merge 14d938b...

bors · 2017-04-06T20:15:14Z

💔 Test failed - status-appveyor

Rollup of 8 pull requests - Successful merges: #40878, #40976, #41089, #41090, #41108, #41111, #41112, #41114 - Failed merges:

@nikomatsakis

…shes, r=nikomatsakis Handle DefPath hashing centrally as part of DefPathTable (+ save work during SVH calculation) In almost all cases where we construct a `DefPath`, we just hash it and throw it away again immediately. With this PR, the compiler will immediately compute and store the hash for each `DefPath` as it is allocated. This way we + can get rid of any subsequent `DefPath` hash caching (e.g. the `DefPathHashes`), + don't need to allocate a transient `Vec` for holding the `DefPath` (although I'm always surprised how little these small, dynamic allocations seem to hurt performance), and + we don't hash `DefPath` prefixes over and over again. That last part is because we construct the hash for `prefix::foo` by hashing `(hash(prefix), foo)` instead of hashing every component of prefix. The last commit of this PR is pretty neat, I think: ``` The SVH (Strict Version Hash) of a crate is currently computed by hashing the ICHes (Incremental Computation Hashes) of the crate's HIR. This is fine, expect that for incr. comp. we compute two ICH values for each HIR item, one for the complete item and one that just includes the item's interface. The two hashes are are needed for dependency tracking but if we are compiling non-incrementally and just need the ICH values for the SVH, one of them is enough, giving us the opportunity to save some work in this case. ``` r? @nikomatsakis This PR depends on rust-lang#40878 to be merged first (you can ignore the first commit for reviewing, that's just rust-lang#40878).

rust-highfive assigned nikomatsakis Mar 28, 2017

michaelwoerister force-pushed the dmh branch from 0210c9f to a7a21b6 Compare March 29, 2017 13:00

michaelwoerister force-pushed the dmh branch from a7a21b6 to 9d79083 Compare March 30, 2017 10:06

michaelwoerister force-pushed the dmh branch 2 times, most recently from 4589028 to 7684437 Compare March 30, 2017 13:28

michaelwoerister mentioned this pull request Apr 4, 2017

Handle DefPath hashing centrally as part of DefPathTable (+ save work during SVH calculation) #41056

Merged

frewsxcv mentioned this pull request Apr 5, 2017

Rollup of 9 pull requests #41093

Closed

frewsxcv mentioned this pull request Apr 6, 2017

Rollup of 8 pull requests #41100

Closed

frewsxcv mentioned this pull request Apr 6, 2017

Rollup of 7 pull requests #41101

Closed

Introduce HashStable trait and base ICH implementations on it.

c47cdc0

This initial commit provides implementations for HIR, MIR, and everything that also needs to be supported for those two.

michaelwoerister force-pushed the dmh branch from 7684437 to c47cdc0 Compare April 6, 2017 14:02

frewsxcv mentioned this pull request Apr 6, 2017

Rollup of 8 pull requests #41116

Closed

frewsxcv mentioned this pull request Apr 6, 2017

Rollup of 8 pull requests #41121

Merged

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

Auto merge of #41121 - frewsxcv:rollup, r=frewsxcv

2277f4b

Rollup of 8 pull requests - Successful merges: #40878, #40976, #41089, #41090, #41108, #41111, #41112, #41114 - Failed merges:

bors merged commit c47cdc0 into rust-lang:master Apr 7, 2017

carols10cents mentioned this pull request Apr 7, 2017

Spurious failures on AppVeyor due to ar.exe errors #40546

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce HashStable trait and base ICH implementations on it. #40878

Introduce HashStable trait and base ICH implementations on it. #40878

michaelwoerister commented Mar 28, 2017

michaelwoerister commented Mar 29, 2017

michaelwoerister commented Mar 29, 2017

michaelwoerister commented Mar 29, 2017

bors commented Mar 30, 2017

michaelwoerister commented Mar 30, 2017

michaelwoerister commented Mar 30, 2017

nikomatsakis commented Apr 3, 2017

bors commented Apr 3, 2017

bors commented Apr 3, 2017

bors commented Apr 3, 2017

alexcrichton commented Apr 3, 2017 via email

frewsxcv commented Apr 6, 2017

nikomatsakis commented Apr 6, 2017

nikomatsakis commented Apr 6, 2017

michaelwoerister commented Apr 6, 2017

bors commented Apr 6, 2017

bors commented Apr 6, 2017

bors commented Apr 6, 2017

Introduce HashStable trait and base ICH implementations on it. #40878

Introduce HashStable trait and base ICH implementations on it. #40878

Conversation

michaelwoerister commented Mar 28, 2017

michaelwoerister commented Mar 29, 2017

michaelwoerister commented Mar 29, 2017

michaelwoerister commented Mar 29, 2017

bors commented Mar 30, 2017

michaelwoerister commented Mar 30, 2017

michaelwoerister commented Mar 30, 2017

nikomatsakis commented Apr 3, 2017

bors commented Apr 3, 2017

bors commented Apr 3, 2017

bors commented Apr 3, 2017

alexcrichton commented Apr 3, 2017 via email

frewsxcv commented Apr 6, 2017

nikomatsakis commented Apr 6, 2017

nikomatsakis commented Apr 6, 2017

michaelwoerister commented Apr 6, 2017

bors commented Apr 6, 2017

bors commented Apr 6, 2017

bors commented Apr 6, 2017