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

kill the attr cache #1335

Merged
merged 2 commits into from Dec 3, 2015

Conversation

Projects
None yet
2 participants
@llllllllll
Member

llllllllll commented Dec 3, 2015

The attr cache held a reference to every blaze expression that ever had
an attribute looked up on it. This cache prevented them from ever
getting garbage collected.

Moving the attr_cache to the objects' __dict__ slot allows for the
object to become isolated and the cycle detector can kill the object.

ENH: kill the attr cache.
The attr cache held a reference to every blaze expression that ever had
an attribute looked up on it. This cache prevented them from ever
getting garbage collected.

Moving the attr_cache to the objects' __dict__ slot allows for the
object to become isolated and the cycle detector can kill the object.
@@ -104,8 +101,7 @@ class Expr(Node):
contains shared logic and syntax. It in turn inherits from ``Node`` which
holds all tree traversal logic
"""
__slots__ = '_hash', '__weakref__'
__slots__ = '_hash', '__weakref__', '__dict__'

This comment has been minimized.

@cpcloud

cpcloud Dec 3, 2015

Member

With this change, do we lose the benefit of a lower memory footprint from using __slots__? It's probably not a huge performance benefit for us right now, I'm just curious.

This comment has been minimized.

@llllllllll

llllllllll Dec 3, 2015

Member

We will now allocate one extra dictionary per expression. We will still preallocate the space for all of the other slots so there is still some use for the __slots__.

@cpcloud

This comment has been minimized.

Member

cpcloud commented Dec 3, 2015

Needs a release note, and generally +1 on this change.

@cpcloud cpcloud added this to the 0.9.0 milestone Dec 3, 2015

@llllllllll

This comment has been minimized.

Member

llllllllll commented Dec 3, 2015

Added the entry in the whatsnew, merging on pass.

cpcloud added a commit that referenced this pull request Dec 3, 2015

@cpcloud cpcloud merged commit e790787 into blaze:master Dec 3, 2015

1 check passed

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

@llllllllll llllllllll deleted the quantopian:attr-cache branch Dec 3, 2015

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