Opened 11 years ago

Closed 10 years ago

#10827 closed defect (worksforme)

[patch][ccla] Chart2D with events causes considerable memory leak on render()

Reported by: Jared Jurkiewicz Owned by: Eugene Lazutkin
Priority: high Milestone: 1.6
Component: Charting Version: 1.4.0
Keywords: Cc: [email protected]…, [email protected]
Blocked By: Blocking:

Description (last modified by Adam Peller)

Reported by Masato Noguchi (IBM): When some events are listened by connectToPlot() or actions are added, dojox.charting.Chart2D causes considerable memory leaks on render(). Which is caused by calls of connect() to Shape in dojox.charting.plot2d.Base._connectEvents, which are never disconnected.

The following code actually eliminates the leaks:

dojo.extend(dojox.charting.plot2d.Base, {
		 resetEvents: function(){
		 		 dojo.forEach(this._connects, dojo.disconnect);
		 		 delete this._connects;
		 		 this.plotEvent({type: "onplotreset", plot: this});
		 },
		 _connectEvents: function(shape, o){
		 		 if(!this._connects){
		 		 		 this._connects = [];
		 		 }
		 		 this._connects.push(shape.connect("onmouseover", this, function(e){
		 		 		 o.type  = "onmouseover";
		 		 		 o.event = e;
		 		 		 this.plotEvent(o);
		 		 }));
		 		 this._connects.push(shape.connect("onmouseout", this, function(e){
		 		 		 o.type  = "onmouseout";
		 		 		 o.event = e;
		 		 		 this.plotEvent(o);
		 		 }));
		 		 this._connects.push(shape.connect("onclick", this, function(e){
		 		 		 o.type  = "onclick";
		 		 		 o.event = e;
		 		 		 this.plotEvent(o);
		 		 }));
		 }
});

Attachments (4)

chart_leak.html (4.0 KB) - added by ESBANTE 10 years ago.
Memory leak example
test_chart2d_leak_test.html (8.0 KB) - added by shengt 10 years ago.
charting_memory_leak.patch (7.4 KB) - added by Adam Peller 10 years ago.
updated patch from xiangxz
linechart_leak.html (3.0 KB) - added by aryalrabin 10 years ago.
line chart leak

Download all attachments as: .zip

Change History (24)

comment:1 Changed 11 years ago by Adam Peller

Description: modified (diff)
Summary: Charting: Chart2D with events causes considerable memory leak on render()[patch][ccla] Chart2D with events causes considerable memory leak on render()

comment:2 Changed 11 years ago by Les

This is something I don't understand about Dojo. If I destroy a widget or remove a DOM element, all event event handlers bound to that widget or DOM element should be unbound. I shouldn't have to worry about it.

See the jQuery doc for the remove() method. They unbound event handlers, but dojo.orphan() is not doing that.

http://api.jquery.com/remove/

comment:3 Changed 11 years ago by Eugene Lazutkin

Milestone: tbd1.4.2

comment:4 Changed 11 years ago by bill

Milestone: 1.4.21.5

1.4.2 has already been released. Suggest targeting for 1.5 although we could have a 1.4.3.

comment:5 in reply to:  2 Changed 11 years ago by Eugene Lazutkin

Status: newassigned

Replying to Les:

Next time just open a separate ticket, if you see a general problem. I opened a ticket against the core: #10941.

comment:6 Changed 11 years ago by Eugene Lazutkin

Resolution: fixed
Status: assignedclosed

(In [21714]) Guarding against potential memory leaks, thx Masato Noguchi (IBM) and jaredj!, !strict, fixes #10827.

comment:7 Changed 11 years ago by Adam Peller

Cc: [email protected] [email protected] added
Resolution: fixed
Status: closedreopened

Sheng Tian (IBM) reports:

dojox.charting.action2d.Magnify, dojox.charting.action2d.MoveSlice?,dojox.charting.action2d.Shake, dojox.charting.DataChart?, dojox.charting.plot2d.Pie, all have dojo.connect without disconnect.

comment:8 Changed 10 years ago by Adam Peller

new patch provided

comment:9 Changed 10 years ago by ESBANTE

Seems like charting.action2d.Tooltip needs to be fixed too.

Changed 10 years ago by ESBANTE

Attachment: chart_leak.html added

Memory leak example

comment:10 Changed 10 years ago by ESBANTE

hi, I was getting memory leaks on IE 8 so i applied the patch to my dojo lib to see if it will fix the problem. although the memory leak persists. I am not sure if the problem is the patch or something on the approach I am using on the chart (see attached file. chart_leak.html)

Changed 10 years ago by shengt

Attachment: test_chart2d_leak_test.html added

comment:11 Changed 10 years ago by shengt

Still can see some memory leak on IE8 after applying Tooltip, Magnify, Highlight actions. See attached test file.

Changed 10 years ago by Adam Peller

Attachment: charting_memory_leak.patch added

updated patch from xiangxz

comment:12 Changed 10 years ago by Eugene Lazutkin

(In [22375]) charting: eliminating potential memory leaks in programmatic events, thx vmware!, !strict, refs #11356, refs #11374, refs #10827.

comment:13 in reply to:  11 Changed 10 years ago by Eugene Lazutkin

Replying to shengt:

Still can see some memory leak on IE8 after applying Tooltip, Magnify, Highlight actions. See attached test file.

No wonder --- your test file creates and attaches actions yet never destroys them. All external objects (actions, legends, data stores, and so on) should be destroyed separately --- the same way they were created separately.

comment:14 Changed 10 years ago by Eugene Lazutkin

I spent quality time with the updated leak test and with current code I see a sustainable memory level. I tired Chrome, IE6, and IE8.

comment:15 Changed 10 years ago by Eugene Lazutkin

Resolution: fixed
Status: reopenedclosed

Robot missed: (In [22379]) charting: memory-leak preventive changes, !strict, fixes #10827.

comment:16 Changed 10 years ago by Eugene Lazutkin

BTW, it is safe to leave JavaScript objects connected using dojo.connect(). But if a JavaScript object connected to a DOM node, it may cause leaks, and such links should be disconnected with dojo.disconnect(). That's why actions do not have to disconnect when they connected to a local animation object.

Changed 10 years ago by aryalrabin

Attachment: linechart_leak.html added

line chart leak

comment:17 Changed 10 years ago by aryalrabin

Resolution: fixed
Status: closedreopened

I am still seeing memory leaks on IE 8, FF, Chrome. I have similar as chart_leak.html. I have attached file linechart_leak.html.

comment:18 in reply to:  17 Changed 10 years ago by aryalrabin

I am using dojo 1.5

comment:19 Changed 10 years ago by Eugene Lazutkin

Milestone: 1.51.6

Bumping from non-existant 1.5.1 to 1.6.

comment:20 Changed 10 years ago by Eugene Lazutkin

Resolution: worksforme
Status: reopenedclosed

I ran linechart_leak.html using the trunk for one hour on Chrome watching the memory size --- it was clearly bounded from above being on average between 40-80M.

Note: See TracTickets for help on using tickets.