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

Run process execution Nodes on a CpuPool #5691

Merged
merged 3 commits into from Apr 13, 2018

Conversation

Projects
None yet
2 participants
@stuhood
Copy link
Member

stuhood commented Apr 12, 2018

Problem

There are currently two codepaths that run ExecuteProcessRequests, but they both block the main thread while doing so.

Solution

Unify those two codepaths into impl Node for ExecuteProcess, and then move process execution off onto the core.pool. We might eventually want to move the choice to use a pool or not (and/or the decision to bound concurrency) into the process_executor crate. But it seemed reasonable to defer that until we're invoking remote processes here.

Result

Process executions should run concurrently, and off of the main thread.

@stuhood stuhood requested review from illicitonion , ity and dotordogh Apr 12, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Very nice :) Thanks!

let tmpdir = TempDir::new("process-execution").unwrap();
let request = self.0;
let context2 = context.clone();
// TODO: Process pool management should likely move into the `process_executor` crate, which

This comment has been minimized.

@illicitonion

illicitonion Apr 13, 2018

Contributor

process_execution crate, actually; process_executor is the standalone binary which consumes process_execution :)

(I have no idea why process_execution is used as process_executor in this file. It shouldn't be :)

@stuhood stuhood merged commit 405a964 into pantsbuild:master Apr 13, 2018

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/process-execution-on-pool branch Apr 13, 2018

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