Skip to content

/rust

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

rewrite the predecessors code to create a reduced graph #39424

Merged
merged 5 commits into from Feb 4, 2017

Conversation

Reviewers

@michaelwoerister michaelwoerister

Assignees

@michaelwoerister michaelwoerister

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@nikomatsakis @michaelwoerister @bors
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jan 31, 2017 •
edited

The old code created a flat listing of "HIR -> WorkProduct" edges.
While perfectly general, this could lead to a lot of repetition if the
same HIR nodes affect many work-products. This is set to be a problem
when we start to skip typeck, since we will be adding a lot more
"work-product"-like nodes.

The newer code uses an alternative strategy: it "reduces" the graph
instead. Basically we walk the dep-graph and convert it to a DAG, where
we only keep intermediate nodes if they are used by multiple
work-products.

This DAG does not contain the same set of nodes as the original graph,
but it is guaranteed that (a) every output node is included in the graph
and (b) the set of input nodes that can reach each output node is
unchanged.

(Input nodes are basically HIR nodes and foreign metadata; output nodes
are nodes that have assocaited state which we will persist to disk in
some way. These are assumed to be disjoint sets.)

r? @michaelwoerister

Fixes #39494
@nikomatsakis
rewrite the predecessors code to create a reduced graph 
The old code created a flat listing of "HIR -> WorkProduct" edges.
While perfectly general, this could lead to a lot of repetition if the
same HIR nodes affect many work-products. This is set to be a problem
when we start to skip typeck, since we will be adding a lot more
"work-product"-like nodes.

The newer code uses an alternative strategy: it "reduces" the graph
instead. Basically we walk the dep-graph and convert it to a DAG, where
we only keep intermediate nodes if they are used by multiple
work-products.

This DAG does not contain the same set of nodes as the original graph,
but it is guaranteed that (a) every output node is included in the graph
and (b) the set of input nodes that can reach each output node is
unchanged.

(Input nodes are basically HIR nodes and foreign metadata; output nodes
are nodes that have assocaited state which we will persist to disk in
some way. These are assumed to be disjoint sets.)
Loading status checks…
d848f1d

@rust-highfive rust-highfive assigned michaelwoerister Jan 31, 2017

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Feb 1, 2017

@michaelwoerister I did some measurements of syntex-syntax. The rust master takes 3.6 seconds to encode the dep-graph. This branch takes 2.6 seconds. So this seems like a clear win overall.
@michaelwoerister

michaelwoerister reviewed Feb 2, 2017
View changes

Copy link
Contributor

michaelwoerister left a comment

I did one pass over this and it looks very good! I want to take another, closer look at the implementation and tests for the graph reduction algorithm.
src/librustc_incremental/persist/load.rs Outdated
let mut len = 0;
while len != dirty_nodes.len() {
len = dirty_nodes.len();
for edge in edges {

This comment has been minimized.

Sign in to view
Copy link
@michaelwoerister

michaelwoerister Feb 2, 2017

Contributor

This implementation looks a bit inefficient.

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Feb 2, 2017

Author Contributor

Yeah, I was lazy. :) I can rewrite it to use a work-list or some such thing. One thing is that we don't have the edges indexed by their target, which we would want here. Perhaps I'll change how things are serialized to be a Map<Target, Vec<Source>>. I think we even build one of those in the "reduction" algorithm, so perhaps I should just return that (i.e., don't return a Graph, but some kind of ReducedGraph). I have to look at how the code works there.
src/librustc_incremental/persist/preds/compress/dag_id.rs Outdated
}

impl DagId {
pub fn from_in_index(n: NodeIndex) -> Self {

This comment has been minimized.

Sign in to view
Copy link
@michaelwoerister

michaelwoerister Feb 2, 2017

Contributor

where does the in in from_in_index come from? Wouldn't from_node_index be clearer?

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Feb 2, 2017

Author Contributor

Hmm, I meant in as in "the index in the INPUT graph". Perhaps input_index?
src/librustc_incremental/persist/preds/compress/test.rs Outdated
]);
}

//#[test]

This comment has been minimized.

Sign in to view
Copy link
@michaelwoerister

michaelwoerister Feb 2, 2017

Contributor

Unported test.

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Feb 2, 2017

Author Contributor

Ah yes I forgot about that one.
src/librustc_incremental/persist/save.rs
@@ -178,7 +179,9 @@ pub fn encode_dep_graph(preds: &Predecessors,
// Create a flat list of (Input, WorkProduct) edges for

This comment has been minimized.

Sign in to view
Copy link
@michaelwoerister

michaelwoerister Feb 2, 2017

Contributor

That's note quite true anymore...

nikomatsakis added some commits Feb 3, 2017

@nikomatsakis
make dirty process O(dirty) 
The old algorithm was O(graph)
ef9ae85
@nikomatsakis
s/in_index/input_index/
Loading status checks…
afbf6c8
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Feb 3, 2017

@michaelwoerister I addressed (I think) your feedback w/ exception of commented out test, jfyi.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Feb 3, 2017

@michaelwoerister ok, ported the test.

@nikomatsakis nikomatsakis referenced this pull request Feb 3, 2017

Closed

Refactor dependency graph reduction to allow for more work-products #39494

@michaelwoerister

michaelwoerister reviewed Feb 3, 2017
View changes

src/librustc_incremental/persist/preds/compress/classify/test.rs
let mut reduce = GraphReduce::new(&graph, |n| inputs.contains(n), |n| outputs.contains(n));
Classify::new(&mut reduce).walk();

assert!(reduce.in_cycle(nodes("B"), nodes("C")));

This comment has been minimized.

Sign in to view
Copy link
@michaelwoerister

michaelwoerister Feb 3, 2017

Contributor

Can you also add assert!(reduce.in_cycle(nodes("A"), nodes("C"))); and assert!(reduce.in_cycle(nodes("A"), nodes("B")));?
@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor

michaelwoerister commented Feb 3, 2017

OK, I did another pass. Looks good to me. The graph reduction algorithm is very nice!
Please add the assertions in the test case, if you don't mind. Also, make tidy complains. Otherwise, consider the PR approved.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Feb 4, 2017

@bors r=mw
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 4, 2017

📌 Commit b3096e2 has been approved by mw

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 4, 2017

@frewsxcv
Rollup merge of rust-lang#39424 - nikomatsakis:incr-comp-skip-typeck-… 
…3, r=mw

rewrite the predecessors code to create a reduced graph

The old code created a flat listing of "HIR -> WorkProduct" edges.
While perfectly general, this could lead to a lot of repetition if the
same HIR nodes affect many work-products. This is set to be a problem
when we start to skip typeck, since we will be adding a lot more
"work-product"-like nodes.

The newer code uses an alternative strategy: it "reduces" the graph
instead. Basically we walk the dep-graph and convert it to a DAG, where
we only keep intermediate nodes if they are used by multiple
work-products.

This DAG does not contain the same set of nodes as the original graph,
but it is guaranteed that (a) every output node is included in the graph
and (b) the set of input nodes that can reach each output node is
unchanged.

(Input nodes are basically HIR nodes and foreign metadata; output nodes
are nodes that have assocaited state which we will persist to disk in
some way. These are assumed to be disjoint sets.)

r? @michaelwoerister

Fixes rust-lang#39494
3f518eb

@frewsxcv frewsxcv referenced this pull request Feb 4, 2017

Closed

Rollup of 11 pull requests #39536

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 4, 2017

@frewsxcv
Rollup merge of rust-lang#39424 - nikomatsakis:incr-comp-skip-typeck-… 
…3, r=mw

rewrite the predecessors code to create a reduced graph

The old code created a flat listing of "HIR -> WorkProduct" edges.
While perfectly general, this could lead to a lot of repetition if the
same HIR nodes affect many work-products. This is set to be a problem
when we start to skip typeck, since we will be adding a lot more
"work-product"-like nodes.

The newer code uses an alternative strategy: it "reduces" the graph
instead. Basically we walk the dep-graph and convert it to a DAG, where
we only keep intermediate nodes if they are used by multiple
work-products.

This DAG does not contain the same set of nodes as the original graph,
but it is guaranteed that (a) every output node is included in the graph
and (b) the set of input nodes that can reach each output node is
unchanged.

(Input nodes are basically HIR nodes and foreign metadata; output nodes
are nodes that have assocaited state which we will persist to disk in
some way. These are assumed to be disjoint sets.)

r? @michaelwoerister

Fixes rust-lang#39494
f7af8a5

@frewsxcv frewsxcv referenced this pull request Feb 4, 2017

Closed

Rollup of 10 pull requests #39537

bors added a commit that referenced this pull request Feb 4, 2017

@bors
Auto merge of #39537 - frewsxcv:rollup, r=frewsxcv 
Rollup of 10 pull requests

- Successful merges: #39424, #39442, #39443, #39453, #39454, #39471, #39478, #39486, #39506, #39517
- Failed merges: #39444
Loading status checks…
6791962
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 4, 2017

⌛️ Testing commit b3096e2 with merge eb5cb95...

bors added a commit that referenced this pull request Feb 4, 2017

@bors
Auto merge of #39424 - nikomatsakis:incr-comp-skip-typeck-3, r=mw 
rewrite the predecessors code to create a reduced graph

The old code created a flat listing of "HIR -> WorkProduct" edges.
While perfectly general, this could lead to a lot of repetition if the
same HIR nodes affect many work-products. This is set to be a problem
when we start to skip typeck, since we will be adding a lot more
"work-product"-like nodes.

The newer code uses an alternative strategy: it "reduces" the graph
instead. Basically we walk the dep-graph and convert it to a DAG, where
we only keep intermediate nodes if they are used by multiple
work-products.

This DAG does not contain the same set of nodes as the original graph,
but it is guaranteed that (a) every output node is included in the graph
and (b) the set of input nodes that can reach each output node is
unchanged.

(Input nodes are basically HIR nodes and foreign metadata; output nodes
are nodes that have assocaited state which we will persist to disk in
some way. These are assumed to be disjoint sets.)

r? @michaelwoerister

Fixes #39494
Loading status checks…
eb5cb95
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 4, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: mw
Pushing eb5cb95 to master...

@bors bors merged commit b3096e2 into rust-lang:master Feb 4, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@SimonSapin SimonSapin referenced this pull request Feb 5, 2017

Closed

Experiment with incremental builds #14870

@michaelwoerister michaelwoerister referenced this pull request Feb 6, 2017

Closed

Nightly rustc 1.17.0-nightly (ea7a6486a 2017-02-04) crashes #39569

@nikomatsakis nikomatsakis deleted the nikomatsakis:incr-comp-skip-typeck-3 branch Apr 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.