dojox/charting massive memory leak of shapes

[ben hockey]

this is mostly a continuation of #14589.

apply the attached diff to expose the shape registry and add debugging info to test_themes-amd.html

select a few different themes and watch how the log shows the total number of shapes in the registry start to grow. when you destroy the charts (even if you haven't changed themes) you'll see that there are a lot of shapes still left in the registry. this produces a severe leak that i have seen consistently cause IE9 to crash when plotting large data sets that are updated at regular intervals (ie real-time data).

i found this issue when using the Candlesticks plot so that's probably a good place to start looking for the cause. I was able to see that when the plot was updated shape.destroy was being called for the group of the plot but not for the children (or even more distant descendants of the group). ​http://bugs.dojotoolkit.org/browser/dojo/dojox/trunk/charting/plot2d/Candlesticks.js?rev=29763#L160 shows that the majority of the Elements rendered for a Candlestick are descendants of the group (s is this.group in the snippet below)

var shape = s.createGroup();
shape.setTransform({dx: x, dy: 0 });
var inner = shape.createGroup();
inner.createLine(line).setStroke(finalTheme.series.stroke);
inner.createRect(rect).setStroke(finalTheme.series.stroke).
        setFill(doFill ? finalTheme.series.fill : "white");

i was able to identify this leak but couldn't figure out a simple way to fix it. in my opinion, one approach that would work is that probably most (maybe all) occurrences of shape.dispose(aShape) should be switched to aShape.destroy() and the proper logic for destroying a shape (including destroying it's descendants) should be encapsulated in the shape itself. i really don't see the point in calling shape.dispose(aShape) when aShape.destroy() currently does exactly the same thing but is even better because it could be extended by subclasses to encapsulate more logic when needed.

[cjolif]

The problem of destroy is that it will not allow to re-use gfx shapes which we want to enable for exactly the case you mentionned (frequent data updates) and particularly on mobile. I would say that maybe gfx dispose should be fixed on Groups to be recursive and go down the children like destroy is doing. I'm pretty sure it would solve the issue. pruzand an opinion?

[cjolif]

Ben, do you have the IE9 example that crashes available as well so I can test against it? Thanks.

[cjolif]

fix for trunk for non disposed shaped (based on gfx "fix" adding a recursive parameter to dispose)

[Patrick Ruzand]

To clarify: while the gfx.shape.dispose(aShape) method can be used directly, the standard way should indeed be to use the destroy() path that takes care of the dispose() call in addition to shape specifics cleanup tasks. From my understanding, the charting context is indeed a little bit different, as a shape could be reused (and therefore should not be destroyed, especially the case of composite shapes), but should not retain any pending references to it in case it is never reused, hence the need to call dispose() explicitely to free the gfx registry ref.

I attach a possible patch for gfx that should solve this problem as Christophe suggested. The patch adds an optional boolean parameter to dispose() that indicates whether dispose() should recurse over the shape children, if any.

[Patrick Ruzand]

patch gfx/shape.js to add recursive dispose()

[ben hockey]

i'm unable to share the code that causes IE9 to crash but i've applied these patches and i'm now able to see that the registry is completely emptied between renders - previously i was seeing 1000+ shapes per render that were not being removed from the registry.

i'm going to leave IE9 running for a while but i expect it won't crash this time (and if it does then it's probably not because of this reason). i'd say that these patches have fixed this issue for me.

[Patrick Ruzand]

In [29992] : enable gfx.shape.dispose() to recurse over the shape children. refs #16367

[Patrick Ruzand]

Backport to 1.8

In [29993]:

enable gfx.shape.dispose() to recurse over the shape children. refs #16367

[cjolif]

In [29997]:

fixes #16367. Use dispose recursive version to fix memory leak. !strict.

[cjolif]

trunk fix was committed in [29996]