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

Batch execution of address Specs and remove SelectTransitive #5605

Merged
merged 5 commits into from Mar 19, 2018

Conversation

Projects
None yet
2 participants
@stuhood
Copy link
Member

stuhood commented Mar 15, 2018

Problem

While chasing performance in one area for #4349 we added SelectTransitive. This decreased performance in another significant area: executions involving multiple transitive roots can't structure-share at all when expanding dependencies, leading to many redundant walks (#4533).

Additionally, usability regressed in a few ways: the product Graph could not implement cycle detection for dependencies expanded via SelectTransitive (#4358), and in the case of a missing or broken dependency, the referring context was lost (#4515).

Solution

  • Remove usage of SelectTransitive for TransitiveHydratedTargets to reintroduce structure sharing and improve usability (fixes #4358 and fixes #4515).
  • Replace @rules that operate on single-Specs with batch forms that operate on a Specs collection (fixes #4533).

Result

Cycles should be detected much earlier with:

Exception message: Build graph construction failed: ExecutionError Received unexpected Throw state(s):
Computing Select(Collection(dependencies=(SingleAddress(directory=u'testprojects/src/java/org/pantsbuild/testproject/cycle1', name=u'cycle1'),)), =TransitiveHydratedTargets)
  Computing Task(transitive_hydrated_targets, Collection(dependencies=(SingleAddress(directory=u'testprojects/src/java/org/pantsbuild/testproject/cycle1', name=u'cycle1'),)), =TransitiveHydratedTargets)
    Computing Task(transitive_hydrated_target, testprojects/src/java/org/pantsbuild/testproject/cycle1:cycle1, =TransitiveHydratedTarget)
      Computing Task(transitive_hydrated_target, testprojects/src/java/org/pantsbuild/testproject/cycle2:cycle2, =TransitiveHydratedTarget)
        Throw(No source of required dependency: Dep graph contained a cycle.)
          Traceback (no traceback):
            <pants native internals>
          Exception: No source of required dependency: Dep graph contained a cycle.

(more polish needed here: re-opened #3695 to clean up trace)

And time taken is proportional to the total number of matched targets, rather than to the sum of the closure sizes of the roots:

# before:
$ time ./pants --target-spec-file=targets.txt list | wc -l
    1500

real	0m15.297s
user	0m14.603s
sys	0m1.625s

# after:
$ time ./pants --target-spec-file=targets.txt list | wc -l
    1500

real	0m5.989s
user	0m5.261s
sys	0m1.310s

Runtimes for things like ./pants list :: are unaffected, although memory usage increases by about 5%, likely due to the branchier resulting Graph.

@stuhood stuhood force-pushed the twitter:stuhood/remove-selecttransitive branch from e27fd57 to 0171aba Mar 16, 2018

@stuhood stuhood force-pushed the twitter:stuhood/remove-selecttransitive branch from 0171aba to 2b444f4 Mar 16, 2018

@stuhood stuhood changed the title WIP: Batch execution of `Specs`, and remove `SelectTransitive` Batch execution of address Specs and remove SelectTransitive Mar 16, 2018

@stuhood stuhood force-pushed the twitter:stuhood/remove-selecttransitive branch from 2b444f4 to 5e362ee Mar 16, 2018

@stuhood stuhood requested review from baroquebobcat , illicitonion and kwlzn Mar 16, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Mar 16, 2018

This should now be reviewable. Each commit is useful independently, although commits one and three ended up a bit smeared.

@illicitonion
Copy link
Contributor

illicitonion left a comment

Seems reasonable, but I don't have enough context to provide much meaningful review.

TODO: Further work would be to create exactly one `Collection.of(Spec)` root that also
dedupes.
"""
result = []

This comment has been minimized.

@illicitonion

illicitonion Mar 16, 2018

Contributor

Either document the ordering guarantee here, or use a set

This comment has been minimized.

@stuhood

stuhood Mar 19, 2018

Member

Good point! Thanks.

@stuhood stuhood force-pushed the twitter:stuhood/remove-selecttransitive branch from 5e362ee to 5e1ceec Mar 18, 2018

@stuhood stuhood force-pushed the twitter:stuhood/remove-selecttransitive branch from e098d6d to 5c3bfb7 Mar 19, 2018

@stuhood stuhood merged commit ac0b7e4 into pantsbuild:master Mar 19, 2018

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/remove-selecttransitive branch Mar 19, 2018

@stuhood stuhood added this to the 1.5.x milestone Mar 19, 2018

stuhood added a commit that referenced this pull request Mar 20, 2018

Batch execution of address Specs and remove SelectTransitive (#5605)
While chasing performance in one area for #4349 we added `SelectTransitive`. This decreased performance in another significant area: executions involving multiple transitive roots can't structure-share at all when expanding dependencies, leading to many redundant walks (#4533).

Additionally, usability regressed in a few ways: the product `Graph` could not implement cycle detection for dependencies expanded via `SelectTransitive` (#4358), and in the case of a missing or broken dependency, the referring context was lost (#4515).

* Remove usage of `SelectTransitive` for `TransitiveHydratedTargets` to reintroduce structure sharing and improve usability (fixes #4358 and fixes #4515).
* Replace `@rule`s that operate on single-`Spec`s with batch forms that operate on a `Specs` collection (fixes #4533).

Cycles should be detected much earlier with:
```
Exception message: Build graph construction failed: ExecutionError Received unexpected Throw state(s):
Computing Select(Collection(dependencies=(SingleAddress(directory=u'testprojects/src/java/org/pantsbuild/testproject/cycle1', name=u'cycle1'),)), =TransitiveHydratedTargets)
  Computing Task(transitive_hydrated_targets, Collection(dependencies=(SingleAddress(directory=u'testprojects/src/java/org/pantsbuild/testproject/cycle1', name=u'cycle1'),)), =TransitiveHydratedTargets)
    Computing Task(transitive_hydrated_target, testprojects/src/java/org/pantsbuild/testproject/cycle1:cycle1, =TransitiveHydratedTarget)
      Computing Task(transitive_hydrated_target, testprojects/src/java/org/pantsbuild/testproject/cycle2:cycle2, =TransitiveHydratedTarget)
        Throw(No source of required dependency: Dep graph contained a cycle.)
          Traceback (no traceback):
            <pants native internals>
          Exception: No source of required dependency: Dep graph contained a cycle.
```
_(more polish needed here: re-opened #3695 to clean up `trace`)_

And time taken is proportional to the total number of matched targets, rather than to the sum of the closure sizes of the roots:
```
$ time ./pants --target-spec-file=targets.txt list | wc -l
    1500

real	0m15.297s
user	0m14.603s
sys	0m1.625s

$ time ./pants --target-spec-file=targets.txt list | wc -l
    1500

real	0m5.989s
user	0m5.261s
sys	0m1.310s
```

Runtimes for things like `./pants list ::` are unaffected, although memory usage increases by about 5%, likely due to the branchier resulting `Graph`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment