Fix plotting memory leak
State: closed

This PR resolves #9003 (and explains matplotlib/matplotlib#3892). The root cause of the memory leak is a reference cycle between `MPLPlot` objects and the `AxesSubplot` objects they create.

Specifically, a `plotf` function object is stored in `ax._plot_data` for the purposes of potentially redrawing if the data needs to be resampled. This would be fine if this were a top-level function; however, these are all nested functions that make use of `self`. This means that by `plotf` pulls in `self.ax` and `self.axes`, which point to the `AxesSubplot` that `plotf` is being attached to. We therefore have a reference cycle:

```
AxesSubplot -> AxesSubplot._plot_data -> plotf -> self -> self.ax -> AxesSubplot
```

In order to make the objects collectable, we need to either explicitly break a link or replace it with a weakref. Weakrefs don't work as `AxesSubplot` and `MPLPlot` have the same lifetime. Just not using `_plot_data` prevents the leak but breaks functionality.

The final option as I see it is to change `plotf` to not depend on `self`. This works but involves a fair amount of modifications. I elected to make each of the `plotf`s top-level functions to make the lack of `self`-dependency explicit. This also required making several other functions `classmethods` and moving some data from `MPLPlot` to the `AxesSubplot` object.

The key assumption being made by this change is that either `MPLPlot` objects are discarded immediately after use _or_ we don't want any modifications to the `MPLPlot` (e.g., adding errors post-plotting) to be reflected if redrawing. I believe both cases are true but this patch has the potential for behavioral changes if `MPLPlot` objects are regularly being retained and modified.

I've also added a memory-leak test to prevent a regression; the test fails as expected if applied without the other commits in this patch.


Labels:
name: Visualization
description: plotting

Comments:
@jreback I think this patch is ready for review. Please let me know if you have any questions or suggestions.

cc @TomAugspurger @sinhrks

> Specifically, a plotf function object is stored in ax._plot_data

I understand you're referring to [this](https://github.com/pydata/pandas/blob/master/pandas/tseries/plotting.py#L63). If so, only `line` and `area` are affected and no need to change others?

@sinhrks Yep, it's a subtle bug so I thought it'd be better to convert everything to a safer approach to prevent a reoccurrence.

Understood. What I'm caring is current impl makes it more difficult to understand the logic...

How about:
- Change closure to normal function (not return a function, but uses a function defined in the module directly)
- Make mapping from class to each plot function and unify separated `_get_plot_function`.

```
class LinePlot(MPLPlot):
    plotf = _lineplot_plotf
# or 
_plotf_mapping = {LinePlot: _lineplot_plotf, ...}
```

@sinhrks @TomAugspurger can you have a quick look? 

@qwhelan can you squash to a single commit? and pls add a release note referncing the bug?

@TomAugspurger can you have a look?

@TomAugspurger can you have a look?

I've looked at this a bit. Haven't had a chance to fully digest it since I've never had to deal with this kind of problem before. I'll look more and let you know. It may be something to push for another release. 

> On Mar 10, 2015, at 18:20, jreback notifications@github.com wrote:
> 
> @TomAugspurger can you have a look?
> 
> —
> Reply to this email directly or view it on GitHub.

@jreback Hey, sorry about the delay - made the mistake of letting Inbox decide what emails I see. I'll squash it now. Let me know what version I should put it under for the release notes.

@jreback Given we're at a rc1, I'm going to wait on adding to the docs until 0.16.1 is in development. As @sinhrks pointed out, it'd be possible to trim this down to just `LinePlot` and `AreaPlot` if that makes including this in a point-release more palatable.

whats new is now available for 0.16.1 https://github.com/pydata/pandas/commit/5ebf521eac528f5a4b772e4677f70b689b8b1826

Closing due to better version available in #9814