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

PERF: Release unneeded pipeline terms. #1484

Merged
merged 3 commits into from Sep 15, 2016

Conversation

Projects
None yet
4 participants
@ssanderson
Member

ssanderson commented Sep 14, 2016

Refcount pipeline terms during execution and release terms once they're
no longer needed.

This dramatically reduces memory usage on large pipelines.

PERF: Release unneeded pipeline terms.
Refcount pipeline terms during execution and release terms once they're
no longer needed.

This dramatically reduces memory usage on large pipelines.
@coveralls

This comment has been minimized.

coveralls commented Sep 14, 2016

Coverage Status

Coverage increased (+0.01%) to 86.669% when pulling 3babc38 on refcount-pipelines into fb0981e on master.

@twiecki

This comment has been minimized.

Contributor

twiecki commented Sep 14, 2016

👍 💯

return refcounts
def decref_dependencies(self, term, refcounts):

This comment has been minimized.

@llllllllll

llllllllll Sep 14, 2016

Member

shouldn't decref release the data? It seems like this makes it easier to get the refcounts and workspace out of sync.

This comment has been minimized.

@ssanderson

ssanderson Sep 14, 2016

Member

The data might not be in the actual workspace if we put VWAP in the initial_workspace without close and volume.

The design here, essentially, is that dependency resolution is the TermGraph's job, and computation is the PipelineEngine's job. It's the engine's responsibility to actually do something with the terms that the graph identifies as garbage. This is analogous to the fact that it's the Engine's job to actually compute in the order that the graph determines in TermGraph.ordered.

@coveralls

This comment has been minimized.

coveralls commented Sep 14, 2016

Coverage Status

Coverage increased (+0.03%) to 86.692% when pulling 17d9569 on refcount-pipelines into fb0981e on master.

@ssanderson ssanderson merged commit 2aa646c into master Sep 15, 2016

2 checks passed

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

@ssanderson ssanderson deleted the refcount-pipelines branch Sep 15, 2016

@twiecki

This comment has been minimized.

Contributor

twiecki commented Sep 15, 2016

This is awesome. Can you let me know when this hits staging/prod?

bartosh pushed a commit to bartosh/zipline that referenced this pull request Sep 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment