Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#16367 closed defect (fixed)

dojox/charting massive memory leak of shapes

Reported by: ben hockey Owned by: Eugene Lazutkin
Priority: blocker Milestone: 1.8.2
Component: Charting Version: 1.8.1
Keywords: Cc: cjolif, Patrick Ruzand
Blocked By: Blocking:

Description

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.

Attachments (3)

16367.diff (2.6 KB) - added by ben hockey 7 years ago.
chartingfix_16367.patch (1.1 KB) - added by cjolif 7 years ago.
fix for trunk for non disposed shaped (based on gfx "fix" adding a recursive parameter to dispose)
gfx-dispose.patch (2.4 KB) - added by Patrick Ruzand 7 years ago.
patch gfx/shape.js to add recursive dispose()

Download all attachments as: .zip

Change History (12)

Changed 7 years ago by ben hockey

Attachment: 16367.diff added

comment:1 Changed 7 years ago by ben hockey

Cc: cjolif added
Milestone: tbd1.8.2
Priority: undecidedblocker

comment:2 Changed 7 years ago by cjolif

Cc: Patrick Ruzand added

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?

comment:3 Changed 7 years ago by cjolif

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

Last edited 7 years ago by cjolif (previous) (diff)

Changed 7 years ago by cjolif

Attachment: chartingfix_16367.patch added

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

comment:4 Changed 7 years ago by 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.

Changed 7 years ago by Patrick Ruzand

Attachment: gfx-dispose.patch added

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

comment:5 Changed 7 years ago by 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.

comment:6 Changed 7 years ago by Patrick Ruzand

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

comment:7 Changed 7 years ago by Patrick Ruzand

Backport to 1.8

In [29993]:

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

Last edited 7 years ago by Patrick Ruzand (previous) (diff)

comment:8 Changed 7 years ago by cjolif

Resolution: fixed
Status: newclosed

In [29997]:

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

comment:9 Changed 7 years ago by cjolif

trunk fix was committed in [29996]

Note: See TracTickets for help on using tickets.