incr.comp.: Use DefPathHash-based DepNodes in the serialized DepGraph and remove obsolete DefIdDirectory #42332

michaelwoerister · 2017-05-31T14:57:52Z

With this PR we don't store the dep-graph as a set of DepNode<IndexIntoDefIdDirectory> anymore but instead as a set of DepNode<DefPathHash>. Since a DefPathHash is a global identifier that is valid across compilation sessions, we don't need the DefIdDirectory anymore.

Since a DepNode<DefPathHash> is bigger than a DepNode<IndexIntoDefIdDirectory> and our on-disk encoding of the dep-graph is inefficient, this PR will probably increase the amount of space the dep-graph takes up on disk. I'm in the process of gathering some performance data.

The changes in here are a step towards implementing ICH-based DepNodes (#42294).

r? @nikomatsakis

…hashes

… and remove obsolete DefIdDirectory.

michaelwoerister · 2017-05-31T15:54:38Z

Some numbers for compiling ripgrep:

New implementation

initial runs = 24.81s 24.85s 24.72s
re-runs = 4.97s 4.91s 4.88s
rg-depgraph = 6.0 MB
grep-depgraph = 2.2 MB

Current implementation

initial run = 24.82s 24.98s 24.88s
re-run = 4.98s 4.87s 4.91s
rg-depgraph = 4.0 MB
grep-depgraph = 1.1 MB

I'd say the timings are pretty much equivalent. The on-disk graph gets bigger though. I'll look into a more efficient on-disk representation (which should be pretty similar to what we soon want to do anyway).

michaelwoerister · 2017-06-01T09:33:13Z

One more thing: This change slightly regresses the output of Zincremental-info because it will show incremental: module XYZ is dirty because DepNode::Hir(<some opaque hash value>) changed or was removed in the case of an item having been removed. For things that just have changed, you still get to see the DefPath and for removed things you at least get to see the kind of DepNode.

This could be fixed by storing some additional information with the DepGraph in case -Zincremental-info or -Zquery-dep-graph is passed. But I don't think it's an urgent problem.

michaelwoerister · 2017-06-01T12:45:45Z

OK, I have a proof of concept implementation of a new dep-graph encoding:

New DepGraph Encoding

initial runs = 24.55s 24.75s 24.76s
re-runs = 4.68s 4.67s 4.71s
rg-depgraph = 2.4 MB
grep-depgraph = 1.1 MB

Looks like that slightly improves re-build times and uses less space than before the PR. I'll clean it up and push it.

michaelwoerister · 2017-06-01T13:25:44Z

Ready for review.

…graph.

nikomatsakis

I'm a bit confused how LOCAL_CRATE works, but presumably it does, and I just have to reskim the code. This looks nice.

We should discuss whether we expect DepNode<N> to remain, or be subsumed into QueryKey, etc.

nikomatsakis · 2017-06-01T19:40:49Z

src/librustc/ty/context.rs

@@ -676,6 +680,40 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
        let max_cnum = s.cstore.crates().iter().map(|c| c.as_usize()).max().unwrap_or(0);
        let mut providers = IndexVec::from_elem_n(extern_providers, max_cnum + 1);
        providers[LOCAL_CRATE] = local_providers;
+
+        let def_path_hash_to_def_id = if s.opts.build_dep_graph() {


seems like a good place for a comment kind of explaining roughly what this table is for -- but a question: does this include only foreign crates? I don't see where it adds LOCAL_CRATE to the table...

The local crate is added below via chain()

nikomatsakis · 2017-06-01T19:43:55Z

@bors r+

bors · 2017-06-01T19:43:56Z

📌 Commit a3417bf has been approved by nikomatsakis

bors · 2017-06-03T10:56:48Z

⌛ Testing commit a3417bf with merge 2f2d741...

@nikomatsakis

…akis incr.comp.: Use DefPathHash-based DepNodes in the serialized DepGraph and remove obsolete DefIdDirectory With this PR we don't store the dep-graph as a set of `DepNode<IndexIntoDefIdDirectory>` anymore but instead as a set of `DepNode<DefPathHash>`. Since a `DefPathHash` is a global identifier that is valid across compilation sessions, we don't need the `DefIdDirectory` anymore. Since a `DepNode<DefPathHash>` is bigger than a `DepNode<IndexIntoDefIdDirectory>` and our on-disk encoding of the dep-graph is inefficient, this PR will probably increase the amount of space the dep-graph takes up on disk. I'm in the process of gathering some performance data. The changes in here are a step towards implementing ICH-based `DepNodes` (#42294). r? @nikomatsakis

bors · 2017-06-03T13:17:16Z

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

michaelwoerister added 4 commits May 31, 2017 13:54

Make a newtype for DefPathHash so they are not confused with content …

59ebe8e

…hashes

Build DefPathHash->DefId table when incr.comp. is enabled

f854f8b

Don't print the whole crate disambiguator value in debug output

9da83a8

incr.comp.: Use DefPathHash-based DepNodes in the serialized DepGraph…

0cbb8b5

… and remove obsolete DefIdDirectory.

rust-highfive assigned nikomatsakis May 31, 2017

aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 1, 2017

incr.comp.: Use a more efficient encoding for the on-disk dependency …

a3417bf

…graph.

michaelwoerister force-pushed the no-more-retracing branch from fdd6ae8 to a3417bf Compare June 1, 2017 15:04

nikomatsakis approved these changes Jun 1, 2017

View reviewed changes

bors merged commit a3417bf into rust-lang:master Jun 3, 2017

bors mentioned this pull request Jun 3, 2017

proc_macro: implement TokenTree, TokenKind, hygienic quote!, and other API #40939

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

incr.comp.: Use DefPathHash-based DepNodes in the serialized DepGraph and remove obsolete DefIdDirectory #42332

incr.comp.: Use DefPathHash-based DepNodes in the serialized DepGraph and remove obsolete DefIdDirectory #42332

Uh oh!

michaelwoerister commented May 31, 2017

Uh oh!

michaelwoerister commented May 31, 2017

Uh oh!

michaelwoerister commented Jun 1, 2017

Uh oh!

michaelwoerister commented Jun 1, 2017

Uh oh!

michaelwoerister commented Jun 1, 2017

Uh oh!

nikomatsakis left a comment

Uh oh!

nikomatsakis Jun 1, 2017

Uh oh!

michaelwoerister Jun 1, 2017

Uh oh!

nikomatsakis commented Jun 1, 2017

Uh oh!

bors commented Jun 1, 2017

Uh oh!

bors commented Jun 3, 2017

Uh oh!

bors commented Jun 3, 2017

Uh oh!

Uh oh!

incr.comp.: Use DefPathHash-based DepNodes in the serialized DepGraph and remove obsolete DefIdDirectory #42332

incr.comp.: Use DefPathHash-based DepNodes in the serialized DepGraph and remove obsolete DefIdDirectory #42332

Uh oh!

Conversation

michaelwoerister commented May 31, 2017

Uh oh!

michaelwoerister commented May 31, 2017

New implementation

Current implementation

Uh oh!

michaelwoerister commented Jun 1, 2017

Uh oh!

michaelwoerister commented Jun 1, 2017

New DepGraph Encoding

Uh oh!

michaelwoerister commented Jun 1, 2017

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Jun 1, 2017

Choose a reason for hiding this comment

Uh oh!

michaelwoerister Jun 1, 2017

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Jun 1, 2017

Uh oh!

bors commented Jun 1, 2017

Uh oh!

bors commented Jun 3, 2017

Uh oh!

bors commented Jun 3, 2017

Uh oh!

Uh oh!