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

Nodes output cheaply-cloneable values #6078

Merged
merged 5 commits into from Jul 10, 2018

Conversation

Projects
None yet
3 participants
@stuhood
Copy link
Member

stuhood commented Jul 9, 2018

Problem

Node::Item has a Clone bound because in order to give each consumer of a Node it's own copy of an output, we need to be able to Clone it. Using references into the Graph would avoid these copies, but that change would not be feasible without moving toward holding locks on individual Nodes and getting more reference friendly via async/await.

More generally, Cloneing a python Value instance requires mutating python collections while the GIL is acquired, and later requires us to call back to python to Drop the clone we have created.

Solution

Make the output of Nodes more cheaply-cloneable in a few cases, primarily by wrapping all python handles in Arcs.

In particular, this involved moving all "opaque pointer" logic out of Value and into a more-explicit Handle wrapper, which replaces Value in all locations in the python FFI. Value then acts as a smart pointer, containing an Arc<Handle>. This avoids calling back to python to clone values in almost all cases.

Result

5-10% faster cold ./pants list ::.

@stuhood stuhood requested review from baroquebobcat , illicitonion and ity Jul 9, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 9, 2018

Commits are useful to review independently.

@stuhood stuhood requested review from dotordogh and removed request for ity Jul 9, 2018

@stuhood stuhood force-pushed the twitter:stuhood/node-output-arcs branch 2 times, most recently from 928258a to 74553e2 Jul 9, 2018

@stuhood stuhood force-pushed the twitter:stuhood/node-output-arcs branch from 74553e2 to b6f2a49 Jul 9, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 9, 2018

Expecting travis to go green.

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks good :) Particularly the Python rename commit :)

@@ -69,7 +69,7 @@ pub struct ExecuteProcessRequest {
///
/// The result of running a process.
///
#[derive(Clone, Debug, Eq, PartialEq)]
#[derive(Debug, Eq, PartialEq)]

This comment has been minimized.

@illicitonion

illicitonion Jul 10, 2018

Contributor

This one's a little surprising; this clone should already be super cheap; it's two refcount bumps, two one-word copies, and a four-word copy... Maybe not as cheap as a single refcount bump, but cheap enough?

This comment has been minimized.

@stuhood

stuhood Jul 10, 2018

Member

Oh... Bytes. Good call! I had forgotten that that was ref counted.

Will drop this commit.


impl Handle {
///
/// An escape hatch to allow for cloning a Handle without cloning the value it points to. You

This comment has been minimized.

@illicitonion

illicitonion Jul 10, 2018

Contributor

This sounds super scary. Can we avoid it somehow?

This comment has been minimized.

@stuhood

stuhood Jul 10, 2018

Member

AFAICT, no. We use it in exactly one place: HandleBuffer, which is used to return a list of Handles from python to rust. In that case, we're cloning the handles out of an allocation that Python made, and that Python will later deallocate. Doing a full clone of each Handle would involve calling back to Python n times to clone a bunch of handles that we already had exclusive access to.

This isn't a new implementation either: it just moved from Value to Handle.

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Looks good to me. I appreciate how you broke it up into commits. It made it really easy to review.

@stuhood stuhood force-pushed the twitter:stuhood/node-output-arcs branch from b6f2a49 to 0ac9e50 Jul 10, 2018

@stuhood stuhood merged commit ddc9621 into pantsbuild:master Jul 10, 2018

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/node-output-arcs branch Jul 10, 2018

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