[engine] Use futures for scheduling #4221

Merged
merged 46 commits into from Feb 4, 2017

Conversation

Projects
None yet
3 participants
@stuhood
Member

stuhood commented Jan 30, 2017

Problem

While working on #4172, a new Node type that attempted to declare large numbers of dependencies recursively was hitting the n^2 behaviour of starting then stopping n times. My first attempt to fix this problem was #4214, but unfortunately that only worked in cases where dependencies had already been computed, which was insufficient in a case that was recursing through the entire filesystem.

The Node step model is vaguely CPS, and we've said for a while that translating it to Futures would be useful to clean it up.

Solution

The native engine now uses futures to execute all work. This review replaces/incorporates #4214.

Result

This review regresses very slightly in terms of performance (5-10%), but unblocks fully concurrent IO on #4172, which gets us within 2x of v1 for cold runtime.

@kwlzn

kwlzn approved these changes Jan 30, 2017

LGTM! thanks Stu!

src/rust/engine/Cargo.toml
@@ -3,9 +3,14 @@ version = "0.0.1"
name = "engine"
authors = [ "Pants Build <pantsbuild@gmail.com>" ]
+[profile.release]
+debug = true

This comment has been minimized.

@kwlzn

kwlzn Jan 30, 2017

Member

temporary? or meant to be permanent?

@kwlzn

kwlzn Jan 30, 2017

Member

temporary? or meant to be permanent?

This comment has been minimized.

@stuhood

stuhood Jan 30, 2017

Member

Meant to be permanent. It doesn't cause an appreciable change in the binary size (a few hundred bytes), but makes profiling and debugging possible. And it doesn't affect whether optimization is enabled.

@stuhood

stuhood Jan 30, 2017

Member

Meant to be permanent. It doesn't cause an appreciable change in the binary size (a few hundred bytes), but makes profiling and debugging possible. And it doesn't affect whether optimization is enabled.

This comment has been minimized.

@kwlzn

kwlzn Jan 30, 2017

Member

ah, gotcha

@kwlzn

kwlzn Jan 30, 2017

Member

ah, gotcha

This comment has been minimized.

@stuhood

stuhood Jan 30, 2017

Member

(I'll add a comment.)

@stuhood

stuhood Jan 30, 2017

Member

(I'll add a comment.)

stuhood added some commits Jan 28, 2017

@stuhood stuhood referenced this pull request Feb 1, 2017

Closed

Port `fs.py` to rust #4172

})
- }
+ .boxed()

This comment has been minimized.

@ity

ity Feb 3, 2017

Member

was reading through this interesting syntax and came across rust-lang-nursery/futures-rs#228 (just fyi)

@ity

ity Feb 3, 2017

Member

was reading through this interesting syntax and came across rust-lang-nursery/futures-rs#228 (just fyi)

@ity

ity approved these changes Feb 3, 2017

this is awesome work! lgtm!

@stuhood stuhood merged commit 5be6fba into pantsbuild:master Feb 4, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@stuhood stuhood deleted the twitter:stuhood/engine-futures branch Feb 4, 2017

stuhood added a commit that referenced this pull request Mar 2, 2017

[engine] Rust IO (#4265)
### Problem

Porting the scheduling of tasks to rust helped to minimize its overhead, but we were still left with a huge number of python objects and the single-core bottleneck of python for the filesystem leaf nodes in the execution graph (the majority).

### Solution

#4221 and #4261 laid the groundwork for executing concurrent IO on the rust side, and this patch enables that. `fs.py` was ported to `fs.rs` with relatively few changes (the largest of which is probably the use of `Future`s).

One significant difference though, is that the IO API in the engine has raised away from low level filesystem operations toward a higher level abstraction. The most fundamental operation exposed to users is now "get me a `Snapshot` for these `PathGlobs`"... ie, "get me a fingerprinted tarball of the files matching these globs". This change had two goals: 1) to make the API boundary between rust and python thin, 2) to prepare for remote execution of tasks by capturing inputs into atomic shippable units. 

### Result

We're able to get multiple cores involved while executing `./pants list ::`, and we're capturing shippable snapshots. Unfortunately, due to #4298, the speedup is not as significant as it should be: will work to fix that issue over the next two weeks.

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

[engine] Use futures for scheduling (#4221)
### Problem

While working on #4172, a new Node type that attempted to declare large numbers of dependencies recursively was hitting the `n^2` behaviour of starting then stopping `n` times. My first attempt to fix this problem was #4214, but unfortunately that only worked in cases where dependencies had already been computed, which was insufficient in a case that was recursing through the entire filesystem.

The `Node` step model is vaguely CPS, and we've said for a while that translating it to Futures would be useful to clean it up.

### Solution

The native engine now uses [futures](https://github.com/alexcrichton/futures-rs) to execute all work. This review replaces/incorporates #4214.

### Result

This review regresses very slightly in terms of performance (5-10%), but unblocks fully concurrent IO on #4172, which gets us within 2x of v1 for cold runtime.

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

[engine] Rust IO (#4265)
### Problem

Porting the scheduling of tasks to rust helped to minimize its overhead, but we were still left with a huge number of python objects and the single-core bottleneck of python for the filesystem leaf nodes in the execution graph (the majority).

### Solution

#4221 and #4261 laid the groundwork for executing concurrent IO on the rust side, and this patch enables that. `fs.py` was ported to `fs.rs` with relatively few changes (the largest of which is probably the use of `Future`s).

One significant difference though, is that the IO API in the engine has raised away from low level filesystem operations toward a higher level abstraction. The most fundamental operation exposed to users is now "get me a `Snapshot` for these `PathGlobs`"... ie, "get me a fingerprinted tarball of the files matching these globs". This change had two goals: 1) to make the API boundary between rust and python thin, 2) to prepare for remote execution of tasks by capturing inputs into atomic shippable units. 

### Result

We're able to get multiple cores involved while executing `./pants list ::`, and we're capturing shippable snapshots. Unfortunately, due to #4298, the speedup is not as significant as it should be: will work to fix that issue over the next two weeks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment