Use more fine grained locks for the dep graph #63756

Zoxc · 2019-08-20T19:19:26Z

Split out from #61845.

Zoxc · 2019-08-20T19:21:25Z

src/librustc/dep_graph/graph.rs

 }

+ // FIXME: Remove this since it appears to be unused?


@michaelwoerister I'm guessing you're the person to ask about this.

At some point we had debug output for incremental compilation that was using this. If it is really unused now, you can remove it.

rust-highfive · 2019-08-20T19:35:29Z

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-08-20T19:20:07.0651330Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-20T19:20:07.0864563Z ##[command]git config gc.auto 0
2019-08-20T19:20:07.1299588Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-20T19:20:07.1362515Z ##[command]git config --get-all http.proxy
2019-08-20T19:20:07.1516728Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/63756/merge:refs/remotes/pull/63756/merge
---
2019-08-20T19:20:41.2725184Z do so (now or later) by using -b with the checkout command again. Example:
2019-08-20T19:20:41.2725230Z 
2019-08-20T19:20:41.2725436Z   git checkout -b <new-branch-name>
2019-08-20T19:20:41.2725475Z 
2019-08-20T19:20:41.2725522Z HEAD is now at 5ca57c7a4 Merge 80d345747ebb3d890aa08021b969cd24fd7cc0fa into 5a56e05abd34e1936df74625c1f40cb6fee0cd4a
2019-08-20T19:20:41.2892821Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-08-20T19:20:41.2895528Z ==============================================================================
2019-08-20T19:20:41.2895586Z Task         : Bash
2019-08-20T19:20:41.2895632Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-08-20T19:30:46.9097528Z    Compiling arena v0.0.0 (/checkout/src/libarena)
2019-08-20T19:30:50.0986540Z    Compiling syntax_pos v0.0.0 (/checkout/src/libsyntax_pos)
2019-08-20T19:30:57.1455584Z    Compiling rustc_errors v0.0.0 (/checkout/src/librustc_errors)
2019-08-20T19:31:12.5147916Z    Compiling fmt_macros v0.0.0 (/checkout/src/libfmt_macros)
2019-08-20T19:31:35.6448390Z error[E0599]: no method named `borrow` found for type `dep_graph::graph::CurrentDepGraph` in the current scope
2019-08-20T19:31:35.6448944Z    --> src/librustc/dep_graph/graph.rs:612:41
2019-08-20T19:31:35.6449175Z     |
2019-08-20T19:31:35.6449528Z 612 |             debug_assert!(!data.current.borrow().node_to_node_index.contains_key(dep_node));
2019-08-20T19:31:35.6450023Z ...
2019-08-20T19:31:35.6450023Z ...
2019-08-20T19:31:35.6450324Z 942 | pub(super) struct CurrentDepGraph {
2019-08-20T19:31:35.6450655Z     | --------------------------------- method `borrow` not found for this
2019-08-20T19:31:35.6461201Z     |
2019-08-20T19:31:35.6461539Z     = help: items from traits can only be used if the trait is in scope
2019-08-20T19:31:35.6461860Z     = note: the following trait is implemented but not in scope, perhaps add a `use` for it:
2019-08-20T19:31:35.6462102Z             `use std::borrow::Borrow;`
2019-08-20T19:31:50.7942952Z error: aborting due to previous error
2019-08-20T19:31:50.7946791Z 
2019-08-20T19:31:50.7956209Z For more information about this error, try `rustc --explain E0599`.
2019-08-20T19:31:50.9721881Z error: Could not compile `rustc`.
---
2019-08-20T19:32:39.4985343Z == clock drift check ==
2019-08-20T19:32:39.5007344Z   local time: Tue Aug 20 19:32:39 UTC 2019
2019-08-20T19:32:39.6574654Z   network time: Tue, 20 Aug 2019 19:32:39 GMT
2019-08-20T19:32:39.6575475Z == end clock drift check ==
2019-08-20T19:32:40.7149248Z ##[error]Bash exited with code '1'.
2019-08-20T19:32:40.7189082Z ##[section]Starting: Checkout
2019-08-20T19:32:40.7191497Z ==============================================================================
2019-08-20T19:32:40.7191574Z Task         : Get sources
2019-08-20T19:32:40.7191645Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

michaelwoerister

Thanks for the PR, @Zoxc!

The changes look correct to me. Do we have evidence that they improve performance?

src/librustc/dep_graph/graph.rs

Zoxc · 2019-08-25T09:01:34Z

The changes look correct to me. Do we have evidence that they improve performance?

This PR probably doesn't help. It's just a prerequisite for other stuff which does improve performance.

JohnCSimon · 2019-09-07T15:55:28Z

Ping from triage:
@Zoxc @michaelwoerister - this has sat idle for over nearly two weeks. What else is there to be done for this PR?
Thanks!

michaelwoerister · 2019-09-09T07:45:52Z

I've taken a look at the code and it looks correct to me, so now it's more of a design question whether we want to do this or not. Since you seem to be doing an audit on all the locking going, @aturon, I'll let you give the go-ahead.

r? @aturon

JohnCSimon · 2019-09-14T02:45:55Z

Ping from triage
@aturon Can you please review this PR? Thanks.

Thanks.

JohnTitor · 2019-09-22T11:28:52Z

Ping from triage: @aturon could you review this PR?

joelpalmer · 2019-09-30T11:37:19Z

Ping from Triage: Pinging @aturon one more time.
CC: @Dylan-DPC

nikomatsakis · 2019-10-03T13:29:17Z

r? @nikomatsakis

@aturon is stepping back from this role, we should reassign all PRs assigned to them.

src/librustc/dep_graph/graph.rs

nikomatsakis

Looks good overall. I have one nit, which is a request to document the overall pattern on CurrentDepGraph.

In general, we discussed this PR a bit in our "parallel sync meeting" today and the general conclusion was that -- in the future -- it might be nice to extract the CurrentDepGraph code into its own module, so that it can have a smaller "abstraction boundary", but since this is part of a larger series of PRs it doesn't make sense to do it now.

We also mentioned using SeqCst everywhere unless there's a strong reason to do otherwise (which doesn't seem to apply here, but @Mark-Simulacrum already noted that here).

nikomatsakis · 2019-10-07T19:12:49Z

src/librustc/dep_graph/graph.rs

@@ -948,8 +939,10 @@ struct DepNodeData {
 }

 pub(super) struct CurrentDepGraph {
- data: IndexVec<DepNodeIndex, DepNodeData>,
- node_to_node_index: FxHashMap<DepNode, DepNodeIndex>,
+ data: Lock<IndexVec<DepNodeIndex, DepNodeData>>,


The comment below is great -- but it'd be good to document just a bit more (this is pre-existing, as the code wasn't all that thoroughlly commented before =)

I'd say something like:

Encodes the rustc dependency graph. The nodes are identified by an index (DepNodeIndex); the data for each node is stored in its DepNodeData, found in the data field.

We never remove nodes from the graph: they are only added. Most of the time, the node is mapped 1-to-1 with some DepNode, which basically identifies a query. When such nodes are allocated, we add the DepNod into the node_to_node_index map and allocate a fresh node index.

This struct uses two locks internally: the data and node_to_node_index field are locked separately. Operations that begin with a DepNodeIndex typically just access the data field.

The only operation that must manipulate both fields is adding new nodes, in which case we first acquire the node_to_node_index lock and then, once a new node is to be inserted, acquire the lock on data.

(We also have a field fields that use atomics: these are simple counters that are used for profiling and debugging and are not used with debug_assertions.)

I added that, but removed the incorrect parts =P

nikomatsakis · 2019-10-07T19:15:07Z

r=me with comment + SeqCst nits applied =)

bors · 2019-10-08T21:14:43Z

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

Zoxc · 2019-10-16T13:57:26Z

@bors r=nikomatsakis

bors · 2019-10-16T13:57:27Z

📌 Commit b6a5740 has been approved by nikomatsakis

bors · 2019-10-16T20:49:32Z

⌛ Testing commit b6a5740 with merge c8fa82c...

@michaelwoerister

Use more fine grained locks for the dep graph Split out from #61845. r? @michaelwoerister cc @aturon

bors · 2019-10-17T00:39:22Z

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing c8fa82c to master...

@nikomatsakis

…sakis Use a sharded dep node to dep node index map Split out from rust-lang#61845 and based on rust-lang#63756. r? @nikomatsakis

@nikomatsakis

…sakis Use a sharded dep node to dep node index map Split out from rust-lang#61845 and based on rust-lang#63756. r? @nikomatsakis

@nikomatsakis

…sakis Use a sharded dep node to dep node index map Split out from rust-lang#61845 and based on rust-lang#63756. r? @nikomatsakis

@nikomatsakis

…sakis Use a sharded dep node to dep node index map Split out from rust-lang#61845 and based on rust-lang#63756. r? @nikomatsakis

rust-highfive assigned michaelwoerister Aug 20, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 20, 2019

Zoxc commented Aug 20, 2019

View reviewed changes

Zoxc force-pushed the sharded-dep-graph-1 branch from 80d3457 to bc5ac9c Compare August 21, 2019 04:22

michaelwoerister reviewed Aug 21, 2019

View reviewed changes

src/librustc/dep_graph/graph.rs Outdated Show resolved Hide resolved

src/librustc/dep_graph/graph.rs Show resolved Hide resolved

Zoxc force-pushed the sharded-dep-graph-1 branch from bc5ac9c to 9972c69 Compare August 25, 2019 08:59

rust-highfive assigned aturon and unassigned michaelwoerister Sep 9, 2019

rust-highfive assigned nikomatsakis and unassigned aturon Oct 3, 2019

Mark-Simulacrum mentioned this pull request Oct 5, 2019

Use a single map for the CurrentDepGraph #65146

Closed

Mark-Simulacrum reviewed Oct 7, 2019

View reviewed changes

src/librustc/dep_graph/graph.rs Outdated Show resolved Hide resolved

nikomatsakis approved these changes Oct 7, 2019

View reviewed changes

Dylan-DPC-zz added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 10, 2019

Zoxc force-pushed the sharded-dep-graph-1 branch 2 times, most recently from 9ee2119 to 89c6415 Compare October 14, 2019 21:33

Use more fine grained locks for the dep graph

b6a5740

Zoxc force-pushed the sharded-dep-graph-1 branch from 89c6415 to b6a5740 Compare October 14, 2019 21:35

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 16, 2019

Zoxc mentioned this pull request Oct 16, 2019

Use a sharded dep node to dep node index map #65472

Merged

bors added a commit that referenced this pull request Oct 16, 2019

Auto merge of #63756 - Zoxc:sharded-dep-graph-1, r=nikomatsakis

c8fa82c

Use more fine grained locks for the dep graph Split out from #61845. r? @michaelwoerister cc @aturon

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 17, 2019

bors merged commit b6a5740 into rust-lang:master Oct 17, 2019

Zoxc deleted the sharded-dep-graph-1 branch October 17, 2019 05:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use more fine grained locks for the dep graph #63756

Use more fine grained locks for the dep graph #63756

Zoxc commented Aug 20, 2019

Zoxc Aug 20, 2019

michaelwoerister Aug 21, 2019

rust-highfive commented Aug 20, 2019

michaelwoerister left a comment

Zoxc commented Aug 25, 2019

JohnCSimon commented Sep 7, 2019

michaelwoerister commented Sep 9, 2019

JohnCSimon commented Sep 14, 2019

JohnTitor commented Sep 22, 2019

joelpalmer commented Sep 30, 2019

nikomatsakis commented Oct 3, 2019

nikomatsakis left a comment •

edited

Loading

nikomatsakis Oct 7, 2019

Zoxc Oct 14, 2019

nikomatsakis commented Oct 7, 2019

bors commented Oct 8, 2019

Zoxc commented Oct 16, 2019

bors commented Oct 16, 2019

bors commented Oct 16, 2019

bors commented Oct 17, 2019

Use more fine grained locks for the dep graph #63756

Use more fine grained locks for the dep graph #63756

Conversation

Zoxc commented Aug 20, 2019

Zoxc Aug 20, 2019

Choose a reason for hiding this comment

michaelwoerister Aug 21, 2019

Choose a reason for hiding this comment

rust-highfive commented Aug 20, 2019

michaelwoerister left a comment

Choose a reason for hiding this comment

Zoxc commented Aug 25, 2019

JohnCSimon commented Sep 7, 2019

michaelwoerister commented Sep 9, 2019

JohnCSimon commented Sep 14, 2019

JohnTitor commented Sep 22, 2019

joelpalmer commented Sep 30, 2019

nikomatsakis commented Oct 3, 2019

nikomatsakis left a comment • edited Loading

Choose a reason for hiding this comment

nikomatsakis Oct 7, 2019

Choose a reason for hiding this comment

Zoxc Oct 14, 2019

Choose a reason for hiding this comment

nikomatsakis commented Oct 7, 2019

bors commented Oct 8, 2019

Zoxc commented Oct 16, 2019

bors commented Oct 16, 2019

bors commented Oct 16, 2019

bors commented Oct 17, 2019

nikomatsakis left a comment •

edited

Loading