Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix memory leak in ./pants changed #5011

Merged
merged 9 commits into from Oct 25, 2017
Merged

Fix memory leak in ./pants changed #5011

merged 9 commits into from Oct 25, 2017

Conversation

stuhood
Copy link
Sponsor Member

@stuhood stuhood commented Oct 24, 2017 •
edited

Problem

Before @kwlzn fixed #4508 by introducing compute_dependency_specs, there was no way to get the "implicit" dependencies of a Target without constructing it. So #4424 switched to using a full BuildGraph to compute the dependents in changed. While that fixed the bug, it also introduced a memory leak, because instantiating a Target causes a cycle with the BuildGraph that owns it.

Solution

Now that #4508 is fixed, restore the codepaths that were replaced in #4424, and use compute_dependency_specs to compute the "implicit" dependencies of Targets in the graph. Additionally, cleans up the tool.jar/javac split in the Java subsystem to remove the requirement that def injectables has been called before requesting injectable specs.

Result

Fixes #4494.

@stuhood
Copy link
Sponsor Member Author

stuhood commented Oct 24, 2017

This is now reviewable. It's possible that this could go further and refactor LegacyBuildGraph to be computed entirely from the _HydratedTargetDependentGraph (which might reduce code duplication), but it seemed like doing that here would cover up the intent.

@stuhood stuhood changed the title WIP: Changed calculator uses hydrated targets Fix memory leak in ./pants changed Oct 24, 2017
kwlzn
kwlzn approved these changes Oct 24, 2017
View reviewed changes
Copy link
Member

@kwlzn kwlzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

noting that there are a few disabled changed integration tests (linked to #4010) that might be nice to re-enable as part of this change if possible. seems like if we're WONTFIX'ing #4010 there shouldn't be any python resources in the pants repo of the type that will break us?

ref: https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/engine/legacy/test_changed_integration.py#L435-L445

src/python/pants/engine/legacy/change_calculator.py
)
for dep in transitive_set:
yield dep

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like it's possible for this to emit duplicate entries. would it make sense to dedupe either here or in the callsite below (in the if changed_request.include_dependees == 'direct' block)?

@stuhood
Copy link
Sponsor Member Author

stuhood commented Oct 24, 2017 •
edited

noting that there are a few disabled changed integration tests (linked to #4010)

With regard to #4010 and #3563: those should both have been fixed by Benjy's change to generate synthetic resource targets as part of the python_library macro, which is why I dropped that portion of #4424. I'm currently adding an additional test to confirm.

@kwlzn
Copy link
Member

kwlzn commented Oct 25, 2017

yeah, seems stale. since they test master commit over commit, I'd guess that #4010 was ultimately the first cause of failures of these tests in master and so they got skipped with a pointer there that has since become irrelevant. so probably good to simply re-enable them would be my guess.

@stuhood stuhood merged commit 806ae3c into pantsbuild:master Oct 25, 2017
@stuhood stuhood deleted the stuhood/changed-calculator-uses-hydrated-targets branch October 25, 2017 03:37
This was referenced Oct 26, 2017
Closed
Merged
stuhood pushed a commit that referenced this pull request Oct 26, 2017
Fix support for custom javac definitions (#5024)
beaa805 
### Problem

#5011 introduced a failure in an integration test which was missed due to #5019.

### Solution

Fix the test by moving the decision between using custom Javac and using the `tools.jar` out of `JavacPlugin` and into the `ProvideToolsJar` task (which runs after `bootstrap-jvm-tools`).

### Result

Fixes #5020.
lenucksi added a commit to lenucksi/pants that referenced this pull request Nov 8, 2017
@lenucksi
Merge branch 'master' into ticket3630
882b9c9 
* master: (305 commits)
  Add configurable message when missing-deps-suggest doesn't have suggestions (pantsbuild#5036)
  Use split_whitespace for parsing of cflags. (pantsbuild#5038)
  Add documentation about strict deps (pantsbuild#5025)
  Fix JarCreate invalidation in the presence of changing resources. (pantsbuild#5030)
  Prepare the 1.4.0dev18 release. (pantsbuild#5034)
  Use the script verified identity when signing. (pantsbuild#5032)
  Dedup dependencies output (pantsbuild#5029)
  Have twine use the previously established pgp key during release. (pantsbuild#5031)
  [simple-code-gen] extension point for injecting extra exports (pantsbuild#4976)
  Prepare the 1.4.0dev17 release. (pantsbuild#5028)
  Content-addressable {file,directory} store and utility (pantsbuild#5012)
  [pantsd] Launch the daemon via a subprocess call. (pantsbuild#5021)
  Use the service deps if the target declares an exception. (pantsbuild#5017)
  Fix support for custom javac definitions (pantsbuild#5024)
  Pass references to Paths (pantsbuild#5022)
  Replace Blake2 with Sha256 (pantsbuild#5014)
  Revert pytest successful test caching in CI. (pantsbuild#5016)
  Fingerprint has from_hex_string, as_bytes, Display, and Debug (pantsbuild#5013)
  Fix memory leak in `./pants changed` (pantsbuild#5011)
  Move confluence related things to contrib (pantsbuild#4986)
  ...
kwlzn pushed a commit to twitter/pants that referenced this pull request Nov 27, 2017
@kwlzn
Fix memory leak in ./pants changed (pantsbuild#5011)
16b2536 
### Problem

Before @kwlzn fixed pantsbuild#4508 by introducing `compute_dependency_specs`, there was no way to get the "implicit" dependencies of a Target without constructing it. So pantsbuild#4424 switched to using a full `BuildGraph` to compute the dependents in changed. While that fixed the bug, it also introduced a memory leak, because instantiating a Target causes a cycle with the BuildGraph that owns it. 

### Solution

Now that pantsbuild#4508 is fixed, restore the codepaths that were replaced in pantsbuild#4424, and use `compute_dependency_specs` to compute the "implicit" dependencies of Targets in the graph. Additionally, cleans up the `tool.jar`/`javac` split in the Java subsystem to remove the requirement that `def injectables` has been called before requesting injectable specs.

### Result

Fixes pantsbuild#4494.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwlzn kwlzn kwlzn approved these changes

@wisechengyi wisechengyi Awaiting requested review from wisechengyi

@benjyw benjyw Awaiting requested review from benjyw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace traversable_*_specs pantsd memory leak when used with v2 changed
2 participants
@stuhood @kwlzn