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

Move each EntryState in the graph under its own Mutex #6095

Merged
merged 6 commits into from Jul 16, 2018

Conversation

Projects
None yet
2 participants
@illicitonion
Copy link
Contributor

illicitonion commented Jul 11, 2018

Fixes #6074.

@illicitonion illicitonion requested a review from stuhood Jul 11, 2018

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Jul 11, 2018

Commits should be independently useful; note that commit 3 moves node back outside of the Mutex :)

if let Some(entry) = inner.entry_for_id_mut(dst_id) {
entry.get(context, dst_id).map(|(res, _)| res).to_boxed()
if let Some(mut entry) = maybe_entry {
entry.get(context, entry_id).map(|(res, _)| res).to_boxed()

This comment has been minimized.

@illicitonion

illicitonion Jul 11, 2018

Contributor

I'm kind of tempted to make it so that context is pre-cloned with the correct entry_id here because it feels weird needing to pass both around, but it looks slightly more complicated than I would have guessed, and also probably means we end up cloning Contexts more than we strictly need to... WDYT?

This comment has been minimized.

@stuhood

stuhood Jul 11, 2018

Member

I have wobbled back and forth on this a bit, yea. IIRC, the only case where the context is not cloned inside Entry::get is when the dependency is cyclic? And that should be quite rare.

This comment has been minimized.

@illicitonion

illicitonion Jul 12, 2018

Contributor

We also don't clone if we're getting something which is already running, or complete, which feels like a common case? But maybe there are corners of the code I don't know which mean we don't hit this codepath in those cases?

Either way, definitely a question for a follow-up PR :)

@@ -18,6 +19,8 @@ pub type EntryId = stable_graph::NodeIndex<u32>;
///
/// Defines executing a cacheable/memoizable step within the given NodeContext.
///
/// Note that it is assumed that Nodes are very cheap to clone.

This comment has been minimized.

@illicitonion

illicitonion Jul 11, 2018

Contributor

This seems reasonable given #6078

This comment has been minimized.

@stuhood

stuhood Jul 11, 2018

Member

Yep, thanks! This was a comment that I realized later that I should have added here.

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Jul 11, 2018

Also worth noting that I haven't profiled this at all, as I'm not sure how we profile pants in general; a pointer to the docs showing how we do so would be much appreciated :)

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/entrystatelocking branch from 2a844f0 to bf3219b Jul 11, 2018

@stuhood
Copy link
Member

stuhood left a comment

This looks great, thanks!

let (entry, entry_id, dep_generations) = {
let mut inner = self.inner.lock().unwrap();
// Get the Generations of all dependencies of the Node. We can trust that these have not changed
// since we began executing, as long as we are not currently marked dirty (see the method doc).

This comment has been minimized.

@stuhood

stuhood Jul 11, 2018

Member

Is this still true? Hm.

I believe that this is still true, as long as we continue to ensure that Node clearing/dirtying occurs under the graph lock. If that sounds right (I only spent 5 minutes thinking about it...) could you update the comments here and there?

@@ -18,6 +19,8 @@ pub type EntryId = stable_graph::NodeIndex<u32>;
///
/// Defines executing a cacheable/memoizable step within the given NodeContext.
///
/// Note that it is assumed that Nodes are very cheap to clone.

This comment has been minimized.

@stuhood

stuhood Jul 11, 2018

Member

Yep, thanks! This was a comment that I realized later that I should have added here.

if let Some(entry) = inner.entry_for_id_mut(dst_id) {
entry.get(context, dst_id).map(|(res, _)| res).to_boxed()
if let Some(mut entry) = maybe_entry {
entry.get(context, entry_id).map(|(res, _)| res).to_boxed()

This comment has been minimized.

@stuhood

stuhood Jul 11, 2018

Member

I have wobbled back and forth on this a bit, yea. IIRC, the only case where the context is not cloned inside Entry::get is when the dependency is cyclic? And that should be quite rare.

///
/// An Entry and its adjacencies.
///
#[derive(Clone)]

This comment has been minimized.

@stuhood

stuhood Jul 11, 2018

Member

Er, actually... you're now cloning the Node for every Graph::get. That's maybe not ideal? Unfortunately I have never found a smoking gun pointing to cloning Nodes being expensive, so this is potentially just FUD.

This comment has been minimized.

@illicitonion

illicitonion Jul 12, 2018

Contributor

As far as I can tell, these are the two options; do everything under one lock, or clone things out of the lock (or do some weird "take and replace with a placeholder" thing, but that seems scary)...

If we need to make Node clones more efficient, we can :)

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 11, 2018

Frankly, the extent of the benchmarking I generally do is running a series of cold ./pants list :: calls on a large repository.

As to profiling: OSX Instruments in recent XCode is quite good (significantly better than previous versions):

  1. Use the CpuProfiler instrument
  2. Select the relevant process in the dropdown near the top left
  3. Alt-Click the Record button (to pop up Recording options) and select "Record Waiting Threads" (since in this case we want to record time spent waiting on locks)
  4. Record most of a pants run (since it involves multiple phases)
  5. Stop recording and then use the "Top Functions" view across all threads:
    screen shot 2018-07-11 at 11 12 02 am
@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 11, 2018

@illicitonion : One more potential thing to profile (if you have time) would be to try using the parking_lot Mutex, which avoids some allocation. https://docs.rs/parking_lot/0.6.2/parking_lot/type.Mutex.html

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/entrystatelocking branch from 5fb71da to 489fe9e Jul 12, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks, looks good!

A small amount of before/after benchmarking would be good, but even breaking even here would be totally fine with me because the finer grained locking sets us up to be optimizing the "more concurrent" case rather than the status quo.

@@ -586,7 +588,10 @@ impl<N: Node> Graph<N> {
/// change while we're running. In order for a dependency to "change" it must have been cleared
/// or been marked dirty. But if our dependencies have been cleared or marked dirty, then we will
/// have been as well. We can thus use the dirty bit as a signal that the generation values of
/// our dependencies are still accurate.
/// our dependencies are still accurate. The dirty bit is safae to rely on as it is only ever

This comment has been minimized.

@stuhood
@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Jul 13, 2018

Profiled on Twitter's source repo, and found no significant change to running list ::.

Still trying to work out how to symbolise an instruments run to see if there's any low-hanging fruit...

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/entrystatelocking branch from 489fe9e to bd26790 Jul 13, 2018

illicitonion added some commits Jul 11, 2018

Move Entry into its own file
And lock down the visibility of functions.

This avoids people poking at inner in ways which may be problematic.
Move node outside of Mutex
It's immutable, so doesn't need a Mutex at all.
Reduce sections under graph lock
These are now guarded by the Entry lock, which is much more granular.

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/entrystatelocking branch from bd26790 to 776f527 Jul 14, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 15, 2018

Feel free to merge when ready.

@illicitonion illicitonion merged commit b42d305 into pantsbuild:master Jul 16, 2018

1 check passed

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

@illicitonion illicitonion deleted the twitter:dwagnerhall/entrystatelocking branch Jul 16, 2018

stuhood added a commit that referenced this pull request Oct 17, 2018

Add bounds checking to Entry::current_running_duration (#6643)
### Problem

As shown in #6640, once we moved `Entry` locks outside of the `Graph` lock in #6095, it became possible for `heavy_hitters` to race against `Nodes` that are starting outside of the `Graph` lock. 

### Solution

Add a bounds check for `Instant::duration_since` in `current_running_duration`, to handle the case where a Node is started after we begin computing `heavy_hitters`. While switching to `Instant::elapsed` would "mostly" avoid this case, it would incur additional syscalls, without fully avoiding panics.

### Result

Fixes #6640.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment