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

anntzer · 2016-07-24T09:41:21Z

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.

efiring · 2016-07-24T18:50:49Z

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

anntzer · 2016-07-24T19:03:44Z

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 · 2016-07-24T19:09:19Z

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).


_lineFunc

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

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

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

jenshnielsen · 2016-07-24T19:11:47Z

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 · 2016-07-24T19:14:25Z

I opened and closed the pr to re trigger travis

anntzer · 2016-07-24T19:31:14Z

Thanks for the help re: docs.

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 (matplotlib#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.

WeatherGod · 2016-07-28T15:14:40Z

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):


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?).

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)).

WeatherGod · 2016-07-28T15:22:36Z

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.

mdboom · 2016-08-02T17:25:38Z

👍 from me. Thanks for cleaning out our cruft.

mdboom added the status: needs review label Jul 24, 2016

Kojoley reviewed Jul 24, 2016
View reviewed changes

jenshnielsen closed this Jul 24, 2016

jenshnielsen reopened this Jul 24, 2016

jenshnielsen added status: needs review and removed status: needs review labels Jul 24, 2016

anntzer force-pushed the break-line-refcycle branch from 353275c to 64479b4 Compare July 24, 2016 19:57

WeatherGod reviewed Jul 28, 2016
View reviewed changes

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

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

tacaswell removed the status: needs review label Aug 2, 2016

anntzer deleted the break-line-refcycle branch October 3, 2016 23:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

anntzer commented Jul 24, 2016

efiring commented Jul 24, 2016

anntzer commented Jul 24, 2016

Kojoley Jul 24, 2016

anntzer Jul 24, 2016

efiring Jul 24, 2016

anntzer Jul 24, 2016 •

edited

Loading

efiring Jul 24, 2016

anntzer Jul 24, 2016

jenshnielsen commented Jul 24, 2016 •

edited

Loading

jenshnielsen commented Jul 24, 2016

anntzer commented Jul 24, 2016

WeatherGod Jul 28, 2016

anntzer Jul 28, 2016 •

edited

Loading

WeatherGod commented Jul 28, 2016

mdboom commented Aug 2, 2016

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

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

Conversation

anntzer commented Jul 24, 2016

efiring commented Jul 24, 2016

anntzer commented Jul 24, 2016

Kojoley Jul 24, 2016

Choose a reason for hiding this comment

anntzer Jul 24, 2016

Choose a reason for hiding this comment

efiring Jul 24, 2016

Choose a reason for hiding this comment

anntzer Jul 24, 2016 • edited Loading

Choose a reason for hiding this comment

efiring Jul 24, 2016

Choose a reason for hiding this comment

anntzer Jul 24, 2016

Choose a reason for hiding this comment

jenshnielsen commented Jul 24, 2016 • edited Loading

jenshnielsen commented Jul 24, 2016

anntzer commented Jul 24, 2016

WeatherGod Jul 28, 2016

Choose a reason for hiding this comment

anntzer Jul 28, 2016 • edited Loading

Choose a reason for hiding this comment

WeatherGod commented Jul 28, 2016

mdboom commented Aug 2, 2016

anntzer Jul 24, 2016 •

edited

Loading

jenshnielsen commented Jul 24, 2016 •

edited

Loading

anntzer Jul 28, 2016 •

edited

Loading