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.

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

Memoize graph walks on Context #7758

Merged
merged 2 commits into from May 19, 2019

Conversation

Projects
None yet
2 participants
@stuhood
Copy link
Member

commented May 18, 2019

Problem

In profiling of noop runs, Context.targets(predicate=..) was taking up a surprising amount of time due to not being memoized. While investigating, I also noticed that MutableBuildGraph was made unnecessary via #7243 and others.

Solution

Remove MutableBuildGraph, inline one method that was unnecessarily abstract into BuildGraph, and then add support for a callback that can be used to invalidate caches of graph consumption. As the comments point out, we hope to eliminate mutability entirely for this usecase in the future.

Result

A 17% speedup for noop runs of larger targets.

@stuhood stuhood requested review from jsirois, benjyw and illicitonion May 18, 2019

@stuhood stuhood added this to the 1.16.x milestone May 18, 2019

@stuhood stuhood force-pushed the twitter:stuhood/memoize-graph-walks branch from 74fbc91 to 40fc18e May 19, 2019

@stuhood stuhood merged commit aa23024 into pantsbuild:master May 19, 2019

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/memoize-graph-walks branch May 19, 2019

stuhood added a commit that referenced this pull request May 19, 2019

Memoize graph walks on Context (#7758)
### Problem

In profiling of noop runs, `Context.targets(predicate=..)` was taking up a surprising amount of time due to not being memoized. While investigating, I also noticed that `MutableBuildGraph` was made unnecessary via #7243 and others.

### Solution

Remove `MutableBuildGraph`, inline one method that was unnecessarily abstract into `BuildGraph`, and then add support for a callback that can be used to invalidate caches of graph consumption. As the comments point out, we hope to eliminate mutability entirely for this usecase in the future. 

### Result

A 17% speedup for noop runs of larger targets.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.