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

weakref to break cyclic references #394

Merged
merged 7 commits into from Jul 14, 2014

Conversation

Projects
None yet
2 participants
@jabooth
Member

jabooth commented Jul 12, 2014

CPython Memory Management

CPython's memory management is a two pronged strategy. The first is reference counting - every time a reference to a Python object is created, a count is incremented on that object.

a = SomeClass()  # count = 1
b = a   # count = 2
a = None  # count = 1
b = None  # count = 0

Once the count is zero the object is unreachable, and it is immediately collected. This is just bookkeeping - there is no real computation involved in this memory management scheme.

However, reference counting has an Achilles heel - cyclic references.

class A(object):
    def __init__(self):
        self.b = B(self)

class B(object):
    def __init__(self, a):
        self.a = a

a = A()  # the A instance has *two* references, one I j
         # just made, and the one that the B instance holds.
a = None  # B instance still holds a reference to A instance and vica versa!

This would be a memory leak if we didn't have some means to go around it.

This is where the second technique comes into play: garbage collection. Here we walk the reference graph of all the objects in the interpreter, looking for isolated parts of the graph. These must be cyclic references, and they can be removed. Garbage collection is much more computationally demanding than reference counting though, so it is run much less frequently.

Menpo's memory usage

I was noticing that some code in Menpo that should have had low memory usage was actually taking a lot of RAM up. Pseudocode:

for m in import_meshes('./'):
    corr_m = correspond(m)
    save_mesh(corr_m, './foo')

For n meshes, the memory usage here should be constant, which is great because I could easily parallelise the code to get the job done much faster. Instead the memory grew linearly to 5GB where it eventually snapped back down, before creeping up again and repeating the pattern.

What I was seeing the Python garbage collector doing it's job.

While we have gotten this far without worrying about memory, it would be nice if we didn't have to put up with these memory patterns. The question of course is - why is the garbage collector even having to fire up in the first place? The answer as we know must be cyclic references (you can even turn the garbage collector off if you are confident you don't have any cyclic references in your code!).

The problem

There are two places in the code with cyclic dependencies:

  1. LandmarkManager and anything Landmarkable
  2. Image and ImageFeatures (thanks @patricksnape!)

This prevents the efficient reference counting from working whenever we delete anything with landmarks (which is pretty much everything we care about in Menpo). Whilst there are good reasons why we might want to change this behaviour anyway (we've already discussed how features should just be functions, landmarks not holding there targets would simplify things + enable more flexible viewing) there is a simple immediate solution.

The solution: weakref

Weakref is in the standard library, and it's sole purpose in life is to combat this problem. Weak references don't count towards reference counting. It's basically a way of saying: I hold a reference to something, but don't let me hold you back from cleaning it up if I'm all that's left.

This PR makes landmarks have weak references to what they landmark, and likewise makes the features proxy object do the same for the images they are attached to. This should massively improve memory usage in Menpo.

The ugly: subtle bugs galore

This PR is a significant improvement to Menpo, but it does expose places where we have potentially had long standing bugs that we just haven't noticed till now. As an example, deepcopy(image) is actually pretty hairy because the landmarks target was a reference. We know this, and added the copy() method to get around the problem. Trouble is, we still use deepcopy in a handful of places, one being as_greyscale() on Image. After this change the weak reference exposed the fact that we were making an extra copy of the image. This caused a whole load of unit tests to fail. I've fixed the problem, but keep your eyes open for weird behaviour after this comes in.

I'll turn this PR into a more fleshed out blog post when I get a mo!

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jul 14, 2014

This is one of my favourite pull requests. Great job Goose

top-gun-goose

I suggest we get this in as soon as possible and make a release?

@jabooth

This comment has been minimized.

Member

jabooth commented Jul 14, 2014

Thanks Mav

top-gun-mav-goose-moment

Yeah I think we should be pretty aggressive in getting this in and checking for any bugs it surfaces. I've moved from deepcopy() to .copy() everywhere in the code with a fallback to deepcopy() if the object isn't copiable. When this does happen a warning is now raised (and this should be rare - all images and shapes are copy() friendly already). I will need to check some things with @jalabort but it shouldn't be too bad to add copy methods to all our Transforms - once that's done we can fully remove all deepcopy calls from Menpo and our memory usage should be a lot saner.

Everyone happy to push forward with this then? @nontas?

jabooth added a commit that referenced this pull request Jul 14, 2014

Merge pull request #394 from jabooth/memfixes
weakref to break cyclic references

@jabooth jabooth merged commit 06f0194 into menpo:master Jul 14, 2014

1 check passed

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

@jabooth jabooth deleted the jabooth:memfixes branch Jul 14, 2014

@jabooth jabooth referenced this pull request Jul 15, 2014

Merged

remove cyclic target reference from landmarks #395

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