# Parallel Compiler Audit: Fine grained locks in DepGraph ###### tags: `parallel-compiler` ## Conclusions - would be good to extract CurrentDepGraph to separate module for auditability - not doing that yet due to future work in this area PR: https://github.com/rust-lang/rust/pull/63756 - `git remote add Zoxc git@github.com:Zoxc/rust.git` - `git fetch Zoxc sharded-dep-graph-1` - `git checkout sharded-dep-graph-1` ## Current situation - Single lock around CurrentDepGraph - No internal locks in CurrentDepGraph - There are no concrete docs around requirements between the fields. - Lock is not consistently held for function bodies - doesn't seem to be re-acquired, either, but hard to audit - did not look at issues, but no recollection of problems with this lock Fields: - `anon_id_seed` is constant - `forbidden_edge` is constant - `data: IndexVec<DepNodeIndex, DepNodeData>` and `node_to_node_index: FxHashMap<DepNode, DepNodeIndex>` - maps are kept in sync, such that DepNode -> DepNodeIndex -> DepNodeData always holds - unclear why two maps - we may want a `IndexMap<DepNode, DepNodeData>`. - DepNodeIndex is easily obtainable - O(1) lookup - https://github.com/rust-lang/rust/pull/65146 - `total_read_count`, `total_duplicate_read_count` * only touched in debug_assertions mode (though not enforced) * [only increased](https://github.com/rust-lang/rust/blob/7870050796e5904a0fc85ecbe6fa6dde1cfe0c91/src/librustc/dep_graph/graph.rs#L1090-L1110) * [read in one place](https://github.com/rust-lang/rust/blob/7870050796e5904a0fc85ecbe6fa6dde1cfe0c91/src/librustc/dep_graph/graph.rs#L497-L498) * for debug printing during encode_dep_graph * presumably one-off, after all changes ## PR proposal https://github.com/Zoxc/rust/blob/sharded-dep-graph-1/src/librustc/dep_graph/graph.rs - Lock removed around CurrentDepGraph Concerns: - CurrentDepGraph is not in its own module. Fields are as such public outside its methods - Atomic ordering for total_read_count and total_duplicate_read_count Changes: * Individual locks around `data` and `node_to_node_index` fields * lock ordering is `node_to_node_index`, then `data`. * both only acquired when adding new nodes * no known problems, but also does not seem helpful * followup PRs here probably bring most of weight? * total_read_count, total_duplicate_read_count * moved to AtomicU64 * Uses Acquire ordering (why?) on read, SeqCst on increment * inconsistent * may want separate meeting to discuss * Alex has noted preference for SeqCst everywhere