Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

incr.comp.: Make DepNode Copy and valid across compilation sessions #42537

Merged
merged 3 commits into from
Jun 12, 2017

Conversation

michaelwoerister
Copy link
Member

@michaelwoerister michaelwoerister commented Jun 8, 2017

This PR moves DepNode to a representation that does not need retracing and thus simplifies comparing dep-graphs from different compilation sessions. The code also gets a lot simpler in many places, since we don't need the generic parameter on DepNode anymore. See #42294 for details.

NOTE: Only the last commit of this is new, the rest is already reviewed in #42504.

This PR is almost done but there are some things I still want to do:

  • Add some module-level documentation to dep_node.rs, explaining especially what the define_dep_nodes!() macro is about.
  • Do another pass over the dep-graph loading logic. I suspect that we can get rid of building the edges map and also use arrays instead of hash maps in some places.

cc @rust-lang/compiler
r? @nikomatsakis

@michaelwoerister michaelwoerister added the A-incr-comp Area: Incremental compilation label Jun 8, 2017
@Mark-Simulacrum
Copy link
Member

Looks like tidy failed here:

[00:03:21] tidy error: /checkout/src/librustc_incremental/persist/preds/mod.rs:69: line longer than 100 chars

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 8, 2017
@michaelwoerister 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 Jun 9, 2017
@michaelwoerister
Copy link
Member Author

OK, this should be ready for review.

@michaelwoerister michaelwoerister changed the title [WIP] Make DepNode Copy and valid across compilation sessions incr.comp.: Make DepNode Copy and valid across compilation sessions Jun 9, 2017
@@ -17,6 +17,58 @@ use rustc_data_structures::stable_hasher::{StableHasher, HashStable};
use ich::StableHashingContext;
use std::hash::Hash;

//! This module defines the `DepNode` type which the compiler uses to represent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥂 love the comments

// erase!() just makes tokens go away. It's used to specify which macro argument
// is repeated (i.e. which sub-expression of the macro we are in) but don't need
// to actually use any of the arguments.
macro_rules! erase {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cute.

@nikomatsakis
Copy link
Contributor

This is really nice. r=me once errors are resolved.

@michaelwoerister
Copy link
Member Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 12, 2017

📌 Commit fdff2d3 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 12, 2017

⌛ Testing commit fdff2d3 with merge 3f8b936...

bors added a commit that referenced this pull request Jun 12, 2017
incr.comp.: Make DepNode `Copy` and valid across compilation sessions

This PR moves `DepNode` to a representation that does not need retracing and thus simplifies comparing dep-graphs from different compilation sessions. The code also gets a lot simpler in many places, since we don't need the generic parameter on `DepNode` anymore.  See #42294 for details.

~~NOTE: Only the last commit of this is new, the rest is already reviewed in #42504

This PR is almost done but there are some things I still want to do:
- [x] Add some module-level documentation to `dep_node.rs`, explaining especially what the `define_dep_nodes!()` macro is about.
- [x] Do another pass over the dep-graph loading logic. I suspect that we can get rid of building the `edges` map and also use arrays instead of hash maps in some places.

cc @rust-lang/compiler
r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Jun 12, 2017

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

@bors bors merged commit fdff2d3 into rust-lang:master Jun 12, 2017
bors added a commit that referenced this pull request Jun 14, 2017
incr.comp.: Make DepNode's std::fmt::Debug implementation useful again.

With #42537 a regular `DepNode` only contains an opaque hash as its identifier. In most cases, this hash is actually a `DefPathHash` and we can reconstruct the `DefId` it came from via a table lookup --- and then use that to print something intelligible for debug outputs. For cases where we cannot reconstruct information from the DepNode's hash, this PR will cache a string representation of the `DepNode` in a side-table. This string is later used for debug outputs.

r? @nikomatsakis
bors added a commit that referenced this pull request Jun 15, 2017
incr.comp.: Make DepNode's std::fmt::Debug implementation useful again.

With #42537 a regular `DepNode` only contains an opaque hash as its identifier. In most cases, this hash is actually a `DefPathHash` and we can reconstruct the `DefId` it came from via a table lookup --- and then use that to print something intelligible for debug outputs. For cases where we cannot reconstruct information from the DepNode's hash, this PR will cache a string representation of the `DepNode` in a side-table. This string is later used for debug outputs.

r? @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants