[engine] Rust IO #4265

Merged
merged 183 commits into from Mar 2, 2017

Conversation

Projects
None yet
3 participants
@stuhood
Member

stuhood commented Feb 17, 2017

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 Futures).

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.

stuhood added some commits Jan 10, 2017

@stuhood

This comment has been minimized.

Show comment
Hide comment
@stuhood

stuhood Feb 23, 2017

Member

All integration tests passing... down to 12 failing unit tests.

Member

stuhood commented Feb 23, 2017

All integration tests passing... down to 12 failing unit tests.

@kwlzn

kwlzn approved these changes Feb 26, 2017

lgtm w/ a handful of misc comments. thanks Stu!

src/python/pants/engine/fs.py
-class PathGlobs(datatype('PathGlobs', ['dependencies'])):
- """A set of 'PathGlob' objects.
+class PathGlobs(datatype('PathGlobs', ['include', 'exclude'])):
+ """A helper class (TODO: possibly unnecessary?) for converting various.

This comment has been minimized.

@kwlzn

kwlzn Feb 26, 2017

Member

for converting various?

@kwlzn

kwlzn Feb 26, 2017

Member

for converting various?

This comment has been minimized.

@stuhood

stuhood Mar 2, 2017

Member

Fixed, thanks.

@stuhood

stuhood Mar 2, 2017

Member

Fixed, thanks.

# TODO resolve what to do with output files, then make these tmp dirs cleaned up.
with temporary_dir(cleanup=False) as sandbox_dir:
if process_request.snapshots:
for snapshot in process_request.snapshots:
- _extract_snapshot(snapshot_directory.root, snapshot, sandbox_dir)
+ todo.extract_snapshot(todo.root, snapshot, sandbox_dir)

This comment has been minimized.

@kwlzn

kwlzn Feb 26, 2017

Member

would expect this to throw AttributeError: 'str' object has no attribute 'extract_snapshot' ?

@kwlzn

kwlzn Feb 26, 2017

Member

would expect this to throw AttributeError: 'str' object has no attribute 'extract_snapshot' ?

This comment has been minimized.

@stuhood

stuhood Mar 2, 2017

Member

Yep... fixed over the weekend.

@stuhood

stuhood Mar 2, 2017

Member

Yep... fixed over the weekend.

@@ -172,7 +118,7 @@ def create(product_type, binary_type, input_selectors, input_conversion, output_
"""TODO: Not clear that `binary_type` needs to be separate from the input selectors."""
# Select the concatenation of the snapshot directory, binary, and input selectors.
- inputs = (Select(_SnapshotDirectory), Select(binary_type)) + tuple(input_selectors)
+ inputs = (Select(binary_type)) + tuple(input_selectors)

This comment has been minimized.

@kwlzn

kwlzn Feb 26, 2017

Member

probably need a trailing comma here inside the parens to keep the lefthand side as a tuple literal?

inputs = (Select(binary_type),) + tuple(...)
@kwlzn

kwlzn Feb 26, 2017

Member

probably need a trailing comma here inside the parens to keep the lefthand side as a tuple literal?

inputs = (Select(binary_type),) + tuple(...)

This comment has been minimized.

@stuhood

stuhood Mar 2, 2017

Member

Fixed.

@stuhood

stuhood Mar 2, 2017

Member

Fixed.

@@ -223,10 +227,10 @@ def _inject(self, subjects):
self._index(target_entries)
yielded_addresses = set()
- for root, state in address_entries.items():
- if not state.value:
+ for (subject, _), state in address_entries.items():

This comment has been minimized.

@kwlzn

kwlzn Feb 26, 2017

Member

for subject in address_entries.keys(): ?

or even better: iterkeys()

@kwlzn

kwlzn Feb 26, 2017

Member

for subject in address_entries.keys(): ?

or even better: iterkeys()

This comment has been minimized.

@stuhood

stuhood Mar 2, 2017

Member

I need both the subject and the state, so I think this is necessary.

@stuhood

stuhood Mar 2, 2017

Member

I need both the subject and the state, so I think this is necessary.

src/python/pants/engine/scheduler.py
- constraint_for(Variants))
+ # Create the ExternContext, and the native Scheduler.
+ self._scheduler =\
+ native.new_scheduler(

This comment has been minimized.

@kwlzn

kwlzn Feb 26, 2017

Member

kill the escaped newline:

self._scheduler = native.new_scheduler(
  ...
)
@kwlzn

kwlzn Feb 26, 2017

Member

kill the escaped newline:

self._scheduler = native.new_scheduler(
  ...
)

This comment has been minimized.

@stuhood

stuhood Mar 2, 2017

Member

Fixed.

@stuhood

stuhood Mar 2, 2017

Member

Fixed.

+ return TypeConstraint(self.context.to_id(constraint))
+
+ scheduler =\
+ self.lib.scheduler_create(

This comment has been minimized.

@kwlzn

kwlzn Feb 26, 2017

Member

ditto re escaped newline

@kwlzn

kwlzn Feb 26, 2017

Member

ditto re escaped newline

This comment has been minimized.

@stuhood

stuhood Mar 2, 2017

Member

Fixed.

@stuhood

stuhood Mar 2, 2017

Member

Fixed.

src/rust/engine/src/fs.rs
+ pub fn new() -> Result<Snapshots, io::Error> {
+ Ok(
+ Snapshots {
+ temp_dir: TempDir::new("snapshots")?,

This comment has been minimized.

@kwlzn

kwlzn Feb 26, 2017

Member

this should probably use new_in and nest somewhere under the pants workdir? or at least get a TODO around that.

@kwlzn

kwlzn Feb 26, 2017

Member

this should probably use new_in and nest somewhere under the pants workdir? or at least get a TODO around that.

This comment has been minimized.

@stuhood

stuhood Mar 2, 2017

Member

Leaving as a TODO. But yes, definitely.

@stuhood

stuhood Mar 2, 2017

Member

Leaving as a TODO. But yes, definitely.

@@ -78,7 +78,7 @@ def test_invalidate_fsnode_incremental(self):
# Invalidate the '3rdparty/python' DirectoryListing, the `3rdparty` DirectoryListing,
# and then the root DirectoryListing by "touching" files/dirs.
- for filename in ('3rdparty/python/BUILD', '3rdparty/python', 'non_existing_file'):
+ for filename in ('3rdparty/python/BUILD', '3rdparty/python', ''):

This comment has been minimized.

@kwlzn

kwlzn Feb 26, 2017

Member

any particular reason for this change? does the literal non_existing_file path cause issues with the new impl or something?

@kwlzn

kwlzn Feb 26, 2017

Member

any particular reason for this change? does the literal non_existing_file path cause issues with the new impl or something?

This comment has been minimized.

@stuhood

stuhood Feb 27, 2017

Member

Oohoo!

So, I removed this case because generally a file changing should not invalidate its parent directory. But I forgot that this is here explicitly because the root is a special case: we will never receive a change notification for the root, so all changes to files under the root need to be treated as a change to the root.

Will fix and make that clearer in the test cases. Thanks!

@stuhood

stuhood Feb 27, 2017

Member

Oohoo!

So, I removed this case because generally a file changing should not invalidate its parent directory. But I forgot that this is here explicitly because the root is a special case: we will never receive a change notification for the root, so all changes to files under the root need to be treated as a change to the root.

Will fix and make that clearer in the test cases. Thanks!

This comment has been minimized.

@stuhood

stuhood Feb 27, 2017

Member

But thinking more about it, that special case should probably be implemented by the fs_service (ie, causing it to spit out the root in addition to files at the root).

@stuhood

stuhood Feb 27, 2017

Member

But thinking more about it, that special case should probably be implemented by the fs_service (ie, causing it to spit out the root in addition to files at the root).

@baroquebobcat

lgtm. The future loop_fn is really neat. I made a few comments.

+ *
+ * TODO: Move `nodes.Context` to this module and rename both of these.
+ */
+pub struct Core {

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 27, 2017

Contributor

What would you think about using Rust's doc comments for comments like this? https://doc.rust-lang.org/beta/book/comments.html

@baroquebobcat

baroquebobcat Feb 27, 2017

Contributor

What would you think about using Rust's doc comments for comments like this? https://doc.rust-lang.org/beta/book/comments.html

This comment has been minimized.

@stuhood

stuhood Mar 2, 2017

Member

Yea, we should. Opened #4300

@stuhood

stuhood Mar 2, 2017

Member

Yea, we should. Opened #4300

src/rust/engine/src/externs.rs
project: ProjectExtern,
project_ignoring_type: ProjectIgnoringTypeExtern,
project_multi: ProjectMultiExtern,
id_to_str: IdToStrExtern,
val_to_str: ValToStrExtern,
create_exception: CreateExceptionExtern,
invoke_runnable: InvokeRunnable,
+ // TODO: Also on `types::Types`

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 27, 2017

Contributor

On first reading, I found this comment confusing. Maybe 'TODO Add str type to types::Types'?

@baroquebobcat

baroquebobcat Feb 27, 2017

Contributor

On first reading, I found this comment confusing. Maybe 'TODO Add str type to types::Types'?

+ specs_constraint = Exactly(SingleAddress,
+ SiblingAddresses,
+ DescendantAddresses,
+ AscendantAddresses)

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 27, 2017

Contributor

Interestingly, I think this will only work for SubjectAsProduct and not for tasks because the task lookup is keyed off of the type constraint. It might be interesting to think about it in terms of #4005

@baroquebobcat

baroquebobcat Feb 27, 2017

Contributor

Interestingly, I think this will only work for SubjectAsProduct and not for tasks because the task lookup is keyed off of the type constraint. It might be interesting to think about it in terms of #4005

@@ -129,15 +136,15 @@ pub fn create_exception(msg: &str) -> Value {
)
}
-pub fn invoke_runnable(runnable: &Runnable) -> Result<Value, Value> {
+pub fn invoke_runnable(func: &Value, args: &[Value], cacheable: bool) -> Result<Value, Value> {

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 27, 2017

Contributor

Nice! This makes more sense.

@baroquebobcat

baroquebobcat Feb 27, 2017

Contributor

Nice! This makes more sense.

src/rust/engine/src/fs.rs
+ symbolic_path: PathBuf,
+ parts: &[Pattern]
+ ) -> Result<Vec<PathGlob>, String> {
+ if canonical_dir.0.as_os_str() == "." && parts.len() == 1 && *CURRENT_DIR == parts[0].as_str() {

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 27, 2017

Contributor

I might be wrong, but I'm not sure the body of this else is reachable. The continue in parse seems like it would prevent there being a "." in the parts slice.

@baroquebobcat

baroquebobcat Feb 27, 2017

Contributor

I might be wrong, but I'm not sure the body of this else is reachable. The continue in parse seems like it would prevent there being a "." in the parts slice.

This comment has been minimized.

@stuhood

stuhood Mar 2, 2017

Member

Indeed!... I think this is a holdout from when we had separate matching syntax for directories that looked for the .. It's now impossible to select the root, and I'm ok with that =P

@stuhood

stuhood Mar 2, 2017

Member

Indeed!... I think this is a holdout from when we had separate matching syntax for directories that looked for the .. It's now impossible to select the root, and I'm ok with that =P

@stuhood stuhood merged commit 53c3e30 into pantsbuild:master Mar 2, 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-rust-io branch Mar 2, 2017

peiyuwang added a commit that referenced this pull request Mar 22, 2017

[engine] Improving performance by iteratively hydrate build file addr…
…esses within SelectTransitive (#4349)

### Problem

As described in #4283, the issue with recursively executing `SelectTransitive` is lots of calls to `store_list` creating massive intermediate lists that increases memory footprint as well as impacts perf.

### Solution

Same idea as in #4265 using `loop_fn` rewrite `SelectTransitive` to execute iteratively. One small difference is in order to use `OrderMap` and `HashSet` for state tracking and outputs are in `Value`s, those `Values` are turned into `Key`s.

ps: previous PR #4334 paved way for this optimization by splitting `SelectTransitive` from `SelectDependencies(...transitive=true)`.

### Result

All existing tests pass. Have verified there is only one `ST` node through viz.

Two follow up items
* #4358 reenable cycle detection
* #3925 Remove BFA now not only is a clean up task, but has performance benefits because the second round hydration is on `Address` which is redundant.

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.

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

[engine] Improving performance by iteratively hydrate build file addr…
…esses within SelectTransitive (#4349)

### Problem

As described in #4283, the issue with recursively executing `SelectTransitive` is lots of calls to `store_list` creating massive intermediate lists that increases memory footprint as well as impacts perf.

### Solution

Same idea as in #4265 using `loop_fn` rewrite `SelectTransitive` to execute iteratively. One small difference is in order to use `OrderMap` and `HashSet` for state tracking and outputs are in `Value`s, those `Values` are turned into `Key`s.

ps: previous PR #4334 paved way for this optimization by splitting `SelectTransitive` from `SelectDependencies(...transitive=true)`.

### Result

All existing tests pass. Have verified there is only one `ST` node through viz.

Two follow up items
* #4358 reenable cycle detection
* #3925 Remove BFA now not only is a clean up task, but has performance benefits because the second round hydration is on `Address` which is redundant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment