Break reference cycle Line2D <-> Line2D._lineFunc. #6821

Merged
merged 1 commit into from Aug 2, 2016

Conversation

Projects
None yet
7 participants
Contributor

anntzer commented Jul 24, 2016

Upon drawing, Line2D objects would store a reference to one of their own
bound methods as their _lineFunc argument. This would lead to them
being gc'ed not when going out of scope, but only when the "true" gc
kicks in; additionally this led to some pickle-related bugs (#3627).

One can easily sidestep this problem by not storing this bound method.

To check the behavior, try (py3.4+ only):

import gc
import weakref
from matplotlib import pyplot as plt

def f():
    fig, ax = plt.subplots()
    img = ax.imshow([[0, 1], [2, 3]])
    weakref.finalize(img, print, "gc'ing image")
    l, = plt.plot([0, 1])
    weakref.finalize(l, print, "gc'ing line")
    fig.canvas.draw()
    img.remove()
    l.remove()

f()
print("we have left the function")
gc.collect()
print("and cleaned up our mess")

Before the patch, the AxesImage is gc'ed when the function exits but the
Line2D only upon explicit garbage collection. After the patch, both are
collected immediately.

mdboom added the needs_review label Jul 24, 2016

Owner

efiring commented Jul 24, 2016

Looks good to me. Why didn't Travis run?

Contributor

anntzer commented Jul 24, 2016

Not sure why... it built on my side: https://travis-ci.org/anntzer/matplotlib/builds/146967616 (docs never build for me, btw... suggestions welcome).

@Kojoley Kojoley and 2 others commented on an outdated diff Jul 24, 2016

lib/matplotlib/tests/test_pickle.py
@@ -236,7 +236,7 @@ def test_grid():
fig = manager.canvas.figure
ax = fig.add_subplot(1, 1, 1)
ax.grid()
- # Drawing the grid triggers instance methods to be attached
+ # Drawing the grid used to trigger instance methods to be attached
# to the Line2D object (_lineFunc).
@Kojoley

Kojoley Jul 24, 2016

Member

_lineFunc

@efiring

efiring Jul 24, 2016

Owner

Probably best to delete the whole comment; it is no longer relevant, right?

@anntzer

anntzer Jul 24, 2016 edited

Contributor

Should we just get rid of the whole test? Given that it is exactly there to test the workaround that this PR removes.

@efiring

efiring Jul 24, 2016

Owner

Good point--I didn't notice that this was in a test. Yes, I think removing the test makes perfect sense.

@anntzer

anntzer Jul 24, 2016

Contributor

done.

Owner

jenshnielsen commented Jul 24, 2016 edited

The docs build fails for you on travis because the install of basemap causes matplotlib to be downgraded to 1.5.1. This is most likely because your fork does not have all the tags which confuses the versioneer version script. I suggest pushing the tags to your branch. git push <REMOTENAME> --tags should do the trick https://help.github.com/articles/pushing-to-a-remote/
See the following in the logs

Installing collected packages: matplotlib, pyproj, pyshp, basemap
  Found existing installation: matplotlib 0+untagged.3118.g353275c
    Uninstalling matplotlib-0+untagged.3118.g353275c:
      Successfully uninstalled matplotlib-0+untagged.3118.g353275c
  Running setup.py install for basemap ... done
Successfully installed basemap-1.0.8 matplotlib-1.5.1 pyproj-1.9.5.1 pyshp-1.2.3

jenshnielsen reopened this Jul 24, 2016

Owner

jenshnielsen commented Jul 24, 2016

I opened and closed the pr to re trigger travis

Contributor

anntzer commented Jul 24, 2016

Thanks for the help re: docs.

@anntzer anntzer Break reference cycle Line2D <-> Line2D._lineFunc.
Upon drawing, Line2D objects would store a reference to one of their own
bound methods as their `_lineFunc` argument.  This would lead to them
being gc'ed not when going out of scope, but only when the "true" gc
kicks in; additionally this led to some pickle-related bugs (#3627).

One can easily sidestep this problem by not storing this bound method.

To check the behavior, try (py3.4+ only):

```
import gc
import weakref
from matplotlib import pyplot as plt

def f():
    fig, ax = plt.subplots()
    img = ax.imshow([[0, 1], [2, 3]])
    weakref.finalize(img, print, "gc'ing image")
    l, = plt.plot([0, 1])
    weakref.finalize(l, print, "gc'ing line")
    fig.canvas.draw()
    img.remove()
    l.remove()

f()
print("we have left the function")
gc.collect()
print("and cleaned up our mess")
```

Before the patch, the AxesImage is gc'ed when the function exits but the
Line2D only upon explicit garbage collection.  After the patch, both are
collected immediately.
64479b4

@WeatherGod WeatherGod commented on the diff Jul 28, 2016

lib/matplotlib/lines.py
@@ -1250,9 +1244,6 @@ def set_dashes(self, seq):
else:
self.set_linestyle((0, seq))
- def _draw_lines(self, renderer, gc, path, trans):
@WeatherGod

WeatherGod Jul 28, 2016

Member

The _drawStyles_l dictionary still references _draw_lines, and so I would imagine that by removing this method would cause problems (but I am not sure I am clear how it would have worked properly in the first place?).

@anntzer

anntzer Jul 28, 2016 edited

Contributor

The entire drawStyles dict is now unused, and only left for backcompatibility purposes (mostly to have a list of drawstyles available as matplotlib.lines.drawStyles). With this change none of the values actually refer to a method name anymore. I killed the other entries in #6497 for another purpose.

(That's also the reason why this can't go in 2.x: this depends on #6497 (and its followup, #6645)).

Member

WeatherGod commented Jul 28, 2016

btw, in case it isn't clear, this can not be backported to v2.x branch as-is because there are more usages of _lineFunc in the v2.x branch.

tacaswell added this to the 2.1 (next point release) milestone Jul 28, 2016

Owner

mdboom commented Aug 2, 2016

👍 from me. Thanks for cleaning out our cruft.

@tacaswell tacaswell merged commit f2fabc0 into matplotlib:master Aug 2, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.004%) to 70.345%
Details

tacaswell removed the needs_review label Aug 2, 2016

anntzer deleted the anntzer:break-line-refcycle branch Oct 3, 2016

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