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.

Sign up for GitHub

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

[WIP] Use a sharded dep node to dep node index map #61845

Closed
wants to merge 1 commit into from
+90 −24
Closed

[WIP] Use a sharded dep node to dep node index map #61845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 85 additions & 24 deletions src/librustc/dep_graph/graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,15 @@ use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_index::vec::{Idx, IndexVec};
use smallvec::SmallVec;
use rustc_data_structures::sync::{Lrc, Lock, AtomicU32, AtomicU64, Ordering};
use rustc_data_structures::sync::{
Lrc, Lock, LockGuard, MappedLockGuard, AtomicU32, AtomicU64, Ordering
};
use rustc_data_structures::sharded::{self, Sharded};
use std::sync::atomic::Ordering::SeqCst;
use std::env;
use std::iter;
use std::hash::Hash;
use std::convert::{TryFrom, TryInto};
use std::collections::hash_map::Entry;
use std::mem;
use crate::ty::{self, TyCtxt};
Expand Down Expand Up @@ -119,7 +123,15 @@ impl DepGraph {
}

pub fn query(&self) -> DepGraphQuery {
let data = self.data.as_ref().unwrap().current.data.lock();
let current = &self.data.as_ref().unwrap().current;
let shards = current.data.lock_shards();
let node_count = current.node_count.load(SeqCst) as usize;
let data: IndexVec<DepNodeIndex, _> = (0..node_count).map(|i| {
let shard = i % sharded::SHARDS;
let inner = i / sharded::SHARDS;
&shards[shard][inner]
}).collect();

let nodes: Vec<_> = data.iter().map(|n| n.node).collect();
let mut edges = Vec::new();
for (from, edge_targets) in data.iter().map(|d| (d.node, &d.edges)) {
Expand Down Expand Up @@ -428,8 +440,7 @@ impl DepGraph {

#[inline]
pub fn fingerprint_of(&self, dep_node_index: DepNodeIndex) -> Fingerprint {
let data = self.data.as_ref().expect("dep graph enabled").current.data.lock();
data[dep_node_index].fingerprint
self.data.as_ref().expect("dep graph enabled").current.data(dep_node_index).fingerprint
}

pub fn prev_fingerprint_of(&self, dep_node: &DepNode) -> Option<Fingerprint> {
Expand Down Expand Up @@ -493,25 +504,32 @@ impl DepGraph {
}

pub fn serialize(&self) -> SerializedDepGraph {
let data = self.data.as_ref().unwrap().current.data.lock();
let current = &self.data.as_ref().unwrap().current;
let shards = current.data.lock_shards();
let node_count = current.node_count.load(SeqCst) as usize;
let data = || (0..node_count).map(|i| {
let shard = i % sharded::SHARDS;
let inner = i / sharded::SHARDS;
&shards[shard][inner]
});

let fingerprints: IndexVec<SerializedDepNodeIndex, _> =
data.iter().map(|d| d.fingerprint).collect();
data().map(|d| d.fingerprint).collect();
let nodes: IndexVec<SerializedDepNodeIndex, _> =
data.iter().map(|d| d.node).collect();
data().map(|d| d.node).collect();

let total_edge_count: usize = data.iter().map(|d| d.edges.len()).sum();
let total_edge_count: usize = data().map(|d| d.edges.len()).sum();

let mut edge_list_indices = IndexVec::with_capacity(nodes.len());
let mut edge_list_data = Vec::with_capacity(total_edge_count);

for (current_dep_node_index, edges) in data.iter_enumerated().map(|(i, d)| (i, &d.edges)) {
for (current_dep_node_index, edges) in data().enumerate().map(|(i, d)| (i, &d.edges)) {
let start = edge_list_data.len() as u32;
// This should really just be a memcpy :/
edge_list_data.extend(edges.iter().map(|i| SerializedDepNodeIndex::new(i.index())));
let end = edge_list_data.len() as u32;

debug_assert_eq!(current_dep_node_index.index(), edge_list_indices.len());
debug_assert_eq!(current_dep_node_index, edge_list_indices.len());
edge_list_indices.push((start, end));
}

Expand Down Expand Up @@ -936,7 +954,14 @@ struct DepNodeData {
/// we first acquire the `node_to_node_index` lock and then, once a new node is to be inserted,
/// acquire the lock on `data.`
pub(super) struct CurrentDepGraph {
data: Lock<IndexVec<DepNodeIndex, DepNodeData>>,
/// The current node count. Used to allocate an index before storing it in the
/// `data` and `node_to_node_index` field below.
Copy link
Contributor

Choose a reason for hiding this comment

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

nice comments :)

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum Oct 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think it might be worth adding something like: "This is semantically the length of the data vector at any given point" if that's accurate? That's how I understood it.

(The reason we can't query the length directly is we have N shards of the vector in reality)

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems good, yes (including the parenthetical note at the end)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// `data` and `node_to_node_index` field below.
/// `data` and `node_to_node_index` field below.
///
/// NB. This is the "logical" length of the complete data vector at any point.
/// In practice, though, the "logical" data vector is sharded across many vectors,
/// which will of course be of smaller lengths.

node_count: AtomicU64,

/// Maps from a `DepNodeIndex` to `DepNodeData`. The lowest bits of `DepNodeIndex` determines
/// which shard is used and the higher bits are the index into the vector.
data: Sharded<Vec<DepNodeData>>,

node_to_node_index: Sharded<FxHashMap<DepNode, DepNodeIndex>>,

/// Used to trap when a specific edge is added to the graph.
Expand Down Expand Up @@ -994,7 +1019,8 @@ impl CurrentDepGraph {
let new_node_count_estimate = (prev_graph_node_count * 102) / 100 + 200;

CurrentDepGraph {
data: Lock::new(IndexVec::with_capacity(new_node_count_estimate)),
node_count: AtomicU64::new(prev_graph_node_count.try_into().unwrap()),
data: Sharded::new(|| Vec::with_capacity(new_node_count_estimate / sharded::SHARDS)),
node_to_node_index: Sharded::new(|| FxHashMap::with_capacity_and_hasher(
new_node_count_estimate / sharded::SHARDS,
Default::default(),
Expand Down Expand Up @@ -1058,20 +1084,56 @@ impl CurrentDepGraph {
edges: SmallVec<[DepNodeIndex; 8]>,
fingerprint: Fingerprint
) -> DepNodeIndex {
match self.node_to_node_index.get_shard_by_value(&dep_node).lock().entry(dep_node) {
Entry::Occupied(entry) => *entry.get(),
let (index, inserted) = match self.node_to_node_index
.get_shard_by_value(&dep_node)
.lock()
.entry(dep_node) {
Entry::Occupied(entry) => (*entry.get(), false),
Entry::Vacant(entry) => {
let mut data = self.data.lock();
let dep_node_index = DepNodeIndex::new(data.len());
data.push(DepNodeData {
node: dep_node,
edges,
fingerprint
});
let index = self.node_count.fetch_add(1, SeqCst);
// Cast to u32 to ensure we didn't overflow.
let index = u32::try_from(index).unwrap();

let dep_node_index = DepNodeIndex::new(index as usize);
entry.insert(dep_node_index);
dep_node_index
(dep_node_index, true)
}
};

if inserted {
Copy link
Contributor

Choose a reason for hiding this comment

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

This deserves a comment. It took me a bit to read and understand what was happening. Let me take a stab at it.

In the code above, we secured an index (index) for this node. Now we have to adjust the data array to include this index. However, note that we've realized the lock on the node_to_node_index map -- so there could be multiple threads contending at this point, each with their own index.

Most of the time, the same thread will acquire both locks, one after the other. In that case, index will equal self.data.len() and we can just push onto the end of self.data.

Other times, however, we may find there is a gap -- e.g., we have been assigned index N+1, but the length of self.data is only N. In that case, we will push a dummy node to represent the index N and then push our own node for index N+1. At this point, we return; when the thread with index N acquires the lock, it will overwrite the dummy node.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Update: I wrote this comment before I understood that the vector itself was sharded, so the text is a bit off...)

let dep_node_data = DepNodeData {
node: dep_node,
edges,
fingerprint
};
let inner_index = index.as_usize() / sharded::SHARDS;
let mut data = self.data.get_shard_by_index(index.as_usize()).lock();
Copy link
Contributor

Choose a reason for hiding this comment

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

with my suggested API below, this would be

let (mut data, inner_index) = self.data.get_shard_by_index(index.as_usize()).lock();

let len = data.len();
if likely!(len == inner_index) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment:

The happy path, where we can just push onto the end of the array.

data.push(dep_node_data)
} else {
let dummy_data = DepNodeData {
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment here:

We can't just push, there is some kind of gap. Add dummy nodes as needed (they may already have been added), and then overwrite our index with the correct value.

node: DepNode::new_no_params(DepKind::Null),
edges: SmallVec::default(),
fingerprint: Fingerprint::ZERO,
};
if inner_index >= len {
data.extend(iter::repeat(dummy_data).take(inner_index - len + 1));
}
data[inner_index] = dep_node_data;
}
}

index
}

fn data(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a doc-comment would be good here -- something like

Returns a guard that gives access to the data with index index. This requires first finding the correct shard, acquiring the lock, and then returning the correct index.

&self,
index: DepNodeIndex,
) -> MappedLockGuard<'_, DepNodeData> {
LockGuard::map(self.data.get_shard_by_index(index.as_usize()).lock(), |vec| {
Copy link
Contributor

Choose a reason for hiding this comment

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

With my suggested API, this would become

|(vec, index)| { ... }

and there would be no need to hard-code the division.

&mut vec[index.as_usize() / sharded::SHARDS]
})
}
}

Expand All @@ -1090,9 +1152,8 @@ impl DepGraphData {
#[cfg(debug_assertions)]
{
if let Some(target) = task_deps.node {
let data = self.current.data.lock();
if let Some(ref forbidden_edge) = self.current.forbidden_edge {
let source = data[source].node;
let source = self.current.data(source).node;
if forbidden_edge.test(&source, &target) {
bug!("forbidden edge {:?} -> {:?} created",
source,
Expand Down
5 changes: 5 additions & 0 deletions src/librustc_data_structures/sharded.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ impl<T> Sharded<T> {
}
}

#[inline]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a doc-comment here? Further, I feel like I would like this function to encapsulate both the modulo and the division operations (I think I left this feedback on some other PR before).

I'd like to see the API be:

    /// Index into a sharded vector. The lower-order bits of `index` 
    /// are used to determine the shared, and the higher-order bits 
    /// are used to select the index within the shard.
    ///
    /// Returns:
    /// * a reference to the shared, `&Lock<T>`
    /// * the index within the shared, a usize
     pub fn get_shard_by_index(&self, index: usize) -> (&Lock<T>, usize) ```

Copy link
Contributor

Choose a reason for hiding this comment

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

Obviously this would affect the callers, which would no longer hard-code the / operation

pub fn get_shard_by_index(&self, index: usize) -> &Lock<T> {
&self.shards[index % SHARDS].0
}

#[inline]
pub fn get_shard_by_value<K: Hash + ?Sized>(&self, val: &K) -> &Lock<T> {
if SHARDS == 1 {
Expand Down