[engine] Split SelectDependencies into SelectDependencies and SelectTransitive #4334

Merged
merged 23 commits into from Mar 15, 2017

Conversation

Projects
None yet
3 participants
@peiyuwang
Contributor

peiyuwang commented Mar 15, 2017

Problem

This is a refactoring change with no functionalities added. it will simplify #4283 so we could optimize just for SelectTransitive.

Solution

Introduce SelectTransitive with identical attributes with SelectDependencies (for now), and split the if else switch in node handling into each node. With that, transitive flag is not needed.

Result

Test all passes.

@peiyuwang peiyuwang requested review from baroquebobcat, stuhood, ity and kwlzn Mar 15, 2017

@stuhood

Thanks Peiyu... looks good!

@@ -121,13 +120,26 @@ def __repr__(self):
field_name_portion = ', {}'.format(repr(self.field))
else:
field_name_portion = ''
- return '{}({}, {}{}{}{})'.format(type(self).__name__,

This comment has been minimized.

@stuhood

stuhood Mar 15, 2017

Member

I don't think this overridden repr is still used anywhere... may be able to remove it.

@stuhood

stuhood Mar 15, 2017

Member

I don't think this overridden repr is still used anywhere... may be able to remove it.

This comment has been minimized.

@stuhood

stuhood Mar 15, 2017

Member

Oh, correction... it's used only in unit tests.

@stuhood

stuhood Mar 15, 2017

Member

Oh, correction... it's used only in unit tests.

This comment has been minimized.

@peiyuwang

peiyuwang Mar 15, 2017

Contributor

yea

@peiyuwang

peiyuwang Mar 15, 2017

Contributor

yea

src/python/pants/engine/selectors.py
+ field_types_portion)
+
+
+class SelectTransitive(datatype('Dependencies', ['product', 'dep_product', 'field', 'field_types']),

This comment has been minimized.

@stuhood

stuhood Mar 15, 2017

Member

s/Dependencies/Transitive/

@stuhood

stuhood Mar 15, 2017

Member

s/Dependencies/Transitive/

This comment has been minimized.

@peiyuwang

peiyuwang Mar 15, 2017

Contributor

changed.

@peiyuwang

peiyuwang Mar 15, 2017

Contributor

changed.

src/rust/engine/src/nodes.rs
+ }
+
+ fn store(&self, dep_product: &Value, dep_values: Vec<&Value>) -> Value {
+ // Not an inner node, or not a traversal.

This comment has been minimized.

@stuhood

stuhood Mar 15, 2017

Member

Comment is probably stale. But also, you can probably just inline this into the caller.

@stuhood

stuhood Mar 15, 2017

Member

Comment is probably stale. But also, you can probably just inline this into the caller.

This comment has been minimized.

@peiyuwang

peiyuwang Mar 15, 2017

Contributor

inlined.

@peiyuwang

peiyuwang Mar 15, 2017

Contributor

inlined.

@stuhood stuhood merged commit 2d7a690 into pantsbuild:master Mar 15, 2017

1 check was pending

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

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.

@peiyuwang peiyuwang deleted the peiyuwang:peiyu/split-select-dependencies branch Mar 22, 2017

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

[engine] Split SelectDependencies into SelectDependencies and SelectT…
…ransitive (#4334)

### Problem

This is a refactoring change with no functionalities added. it will simplify #4283 so we could optimize just for `SelectTransitive`.

### Solution

Introduce `SelectTransitive` with identical attributes with `SelectDependencies` (for now), and split the `if` `else` switch in node handling into each node. With that, `transitive` flag is not needed.

### Result

Test all passes.

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