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

Fixed memory leak in contour #6942

Merged
merged 1 commit into from Aug 22, 2016

Conversation

ianthomas23
Copy link
Member

Fixed memory leak in contour as reported in #6940. PR is based on v2.x branch, but fix needs to be in all active branches.

Problem is down to my stupidity when adding numpy arrays to python lists to return from contour/contourf calls. New contouring C++ code uses our numpy array_view wrappers, which are in charge of their own reference counting, and native Python/C API list functions for which I am explicitly responsible for reference counting. Consider code:

numpy::array_view<double, 2> line(dims);   // ref count 1
PyObject* pyline = line.pyobj();  // ref count 2
PyList_SetItem(pylist, pyline);  // ref count remains 2 as PyList_SetItem steals reference
return pylist;  // line goes out of scope so ref count 1 for the one and only reference in the list

This is fine, but I am using PyList_Append instead of PyList_SetItem as I don't know the length of the list when it is created. It turns out that PyList_Append increments the reference count whereas PyList_SetItem steals a reference. I assume I used to know this but have since forgotten it and have just rediscovered it.

There are 2 options for the fix. Either

  1. keep using array_view.pyobj() but store the returned object and decrement the reference count myself, or
  2. use a new function array_view.pyobj_steal() that returns a stolen reference.

I've opted for the second option as that keeps all the explicit array reference counting within the array_view class.

@efiring efiring added this to the 2.0 (style change major release) milestone Aug 12, 2016
@efiring efiring added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Aug 12, 2016
@efiring
Copy link
Member

efiring commented Aug 12, 2016

Ian, thanks for the instantaneous response! I will leave the review to @mdboom and/or @tacaswell; I'm not competent in the C++ realm.

@tacaswell
Copy link
Member

👍 The explanation and fix both make sense and seem correct to me. Defer to @mdboom on if there is better way.

Good thing I have not gotten to tagging a 1.5.3 yet.

@tacaswell tacaswell merged commit f6d5303 into matplotlib:v2.x Aug 22, 2016
@stonebig
Copy link
Contributor

a 2.0.0b4 with this fix would be nice

tacaswell added a commit that referenced this pull request Sep 8, 2016
@tacaswell
Copy link
Member

backported to v1.5.x as 45243eb

@QuLogic QuLogic modified the milestones: v1.5.x, 2.0 (style change major release) Sep 9, 2016
@ianthomas23 ianthomas23 deleted the 6940_contour_memory_leak branch July 8, 2021 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants