Opened 8 years ago

Closed 7 years ago

#14589 closed defect (fixed)

Suspected memory leak when re-rendering charts

Reported by: rtweed Owned by: cjolif
Priority: high Milestone: 1.7.4
Component: Charting Version: 1.7.1
Keywords: Cc: cjolif, Patrick Ruzand, ben hockey
Blocked By: #15907 Blocking:

Description

There seems to be a memory leak in dojox.charting (latest Dojo 1.7), if you repeatedly update a data series and re-render the chart. On an iPad 1 it will crash the browser after about 200 updates. Here's a self-contained example that illustrates the problem. On a desktop browser it will run successfully for a long time, but if you check the memory you'll see it going down over time. However, I'd like to be able to use this kind of technique for iPad users.

<!DOCTYPE HTML>
<html lang="en">
  <head>
    <meta charset="utf-8">
    <title>Demo: Monthly Sales</title>
    <link rel="stylesheet" href="style.css" media="screen">
    <link rel="stylesheet" href="../../resources/style/demo.css" media="screen">

    <script src="/dojo1.7.1/dojo/dojo.js" data-dojo-config="async: true"></script>
    <script>
      require([
          "dojox/charting/Chart", 
          "dojox/charting/themes/Tom", 
          "dojox/charting/axis2d/Default", 
          "dojox/charting/plot2d/Lines", 
          "dojo/ready"
        ],function(Chart, Themes, Default, Lines, ready){
            ready(function(){	
              var random = function(min, max) {
                return Math.floor(Math.random() * (max - min + 1)) + min;
              };

              // Define the data
              var updatePeriod = 3000;
              var noOfPoints = 50;
              var i;
              var updateNo = 0;
              var chartData = [];
              for (i = 0; i < noOfPoints; i++) {
                chartData[i] = random(0,20000);
              }

              // Create the chart within it's "holding" node
              var chart = new Chart("chartNode");
              // Set the theme
              chart.setTheme(dojox.charting.themes.Tom);

              // Add the only/default plot 
              chart.addPlot("default", {
                type: "Lines",
                markers: true
              });
		
              // Add axes
              chart.addAxis("x");
              chart.addAxis("y", { 
                min: 5000, 
                max: 15000, 
                vertical: true, 
                fixLower: "major", 
                fixUpper: "major" 
              });

              // Add the series of data
              chart.addSeries("Monthly Sales",chartData);

              // Render the chart!
              chart.render();

              var updateChart = function() {
                setTimeout(function() {
                  updateNo++;
                  document.getElementById("updateNo").innerHTML = updateNo;
                  var x = chartData.shift();
                  var lastVal = chartData[noOfPoints - 2];
                  var min = lastVal - 400;
                  if (min < 1000) min = 1000;
                  var max = lastVal + 400;
                  if (max > 20000) max = 20000;
                  chartData.push(random(min, max));
                              
                  chart.updateSeries("Monthly Sales",chartData);
                  chart.render();
                  updateChart();
                },updatePeriod);
              };
              updateChart();
				
            });
      });
			
    </script>
  </head>

  <body>
    <h1>Demo: Monthly Sales</h1>
    <div id="chartNode" style="width:800px;height:400px;"></div>
    <div id="updateNo">0</div>
  </body>

</html>

Attachments (4)

memory-leak-fixes.diff (3.7 KB) - added by Kris Zyp 7 years ago.
More memory leak fixes
shapeRegistryTest.diff (2.2 KB) - added by ben hockey 7 years ago.
shapeRegistryInteractionTest.diff (3.1 KB) - added by ben hockey 7 years ago.
test_StoreSeries-amd.html (6.8 KB) - added by cjolif 7 years ago.
updated test case that also destroy legend widget (to verify we are down to 0 shapes in the end)

Download all attachments as: .zip

Change History (34)

comment:1 Changed 8 years ago by cjolif

Keywords: cjolif added

In order to help us track this down, do you reproduce with older Dojo version?

comment:2 Changed 8 years ago by bill

Cc: cjolif added
Keywords: cjolif removed

comment:3 Changed 8 years ago by cjolif

Milestone: tbd1.7.2
Owner: changed from Eugene Lazutkin to cjolif
Status: newassigned

I have made the test and do not reproduce in 1.6.x. So it looks like a regression it might be related to the changes made to cache gfx shapes (even if not activated here?), but that's just a guess. I will need to investigate.

comment:4 Changed 8 years ago by cjolif

Cc: Patrick Ruzand added

Actually the root issue does not seem to be in charting new features. It seems to come from the new gfx ID feature. If I comment it I don't have the problem anymore.

The flow is the following at each refresh:

  • a gfx group is created
  • shapes are added to the group
  • group.clear() & group.removeShape() are called

I have traced group.clear() and group.removeShape() and shape.dispose() is never called. Which I think probably explains the issue.

From what I understand (thanks pruzand) it will be up to me to call dispose on each shape... However for next release I think we should have an API that does that a group level.

comment:5 Changed 8 years ago by Patrick Ruzand

see #14610 for the corresponding enhancement ticket.

comment:6 Changed 8 years ago by cjolif

In [27556]:

refs #14589. Fixes the issue in 1.7.x branch. In trunk I will wait for gfx new API to implement the fix. !strict.

comment:7 Changed 8 years ago by cjolif

Resolution: fixed
Status: assignedclosed

In [27558]:

fixes #14589. "Temporary" waiting for gfx API to be introduced. !strict.

comment:8 Changed 7 years ago by Patrick Ruzand

In [28309]:

update Element.purgeGroup to use new gfx destroy() api. refs #14589,#14610 !strict

comment:9 Changed 7 years ago by cjolif

In [28371]:

refs #14589, #13256, #14356. Improvement to delayRenderer, leverage gfx clipping when possible (i.e. no VML/IE), and change the memory leak fix to avoid issues when using enableCache. !strict.

comment:10 Changed 7 years ago by cjolif

Milestone: 1.7.2
Resolution: fixed
Status: closedreopened

Most of the problems have been fixed but there is still a potential leak with the rectangles created by the chart itself which are not disposed on each chart full rendering.

comment:11 Changed 7 years ago by cjolif

Milestone: 1.7.3

comment:12 Changed 7 years ago by cjolif

In [28799]:

refs #14589. Fixes potential memory leak. A different fix will be provided for 1.8/trunk. !strict.

comment:13 Changed 7 years ago by cjolif

In [28803]:

refs #14589. Fix another occurrence of the gfx memory leak issue. !strict.

comment:14 Changed 7 years ago by cjolif

Resolution: fixed
Status: reopenedclosed

In [28805]:

fixes #14589. Fixes remaining mem leak for 1.8/trunk. !strict.

Changed 7 years ago by Kris Zyp

Attachment: memory-leak-fixes.diff added

More memory leak fixes

comment:15 Changed 7 years ago by Kris Zyp

Milestone: 1.7.3
Resolution: fixed
Status: closedreopened

I found more memory leaks, there are a number of shapes that are left in the gfx shape registry, including group children and rect/backgrounds. Attached fixes.

comment:16 Changed 7 years ago by cjolif

Milestone: 1.7.4

comment:17 Changed 7 years ago by ben hockey

Cc: ben hockey added

comment:18 in reply to:  15 ; Changed 7 years ago by cjolif

Replying to kzyp:

I found more memory leaks, there are a number of shapes that are left in the gfx shape registry, including group children and rect/backgrounds. Attached fixes.

The proposed patch is breaking the enableCache feature that allows to re-use gfx shapes for some plots from one rendering to another (for example run the mobileCharting demo it won't work anymore). I will try to come up with something that works in all cases. Do you have a test case that does reproduce the issue so I can be 100% sure I fixed it? (the original test-case of this issue seem to work without that additional fixes).

comment:19 in reply to:  18 Changed 7 years ago by ben hockey

Replying to cjolif:

The proposed patch is breaking the enableCache feature that allows to re-use gfx shapes for some plots from one rendering to another (for example run the mobileCharting demo it won't work anymore). I will try to come up with something that works in all cases. Do you have a test case that does reproduce the issue so I can be 100% sure I fixed it? (the original test-case of this issue seem to work without that additional fixes).

since the shape registry is not exposed, it's hard to come up with a standalone test that demonstrates the issue. instead, i'm attaching a patch that exposes the gfx shape registry and adds some logging to the charting/tests/test_StoreSeries-amd.html file that shows the shapes that remain after simply creating and destroying some charts.

without kris's patch, out of the 223 shapes created for the charts in these tests, 37 are not destroyed when the charts are destroyed. even with the patch, 217 shapes are created (seems strange that the number is different) and 14 are not destroyed. ideally there should be no shapes left in the registry after all the charts are destroyed.

also, this is without any interaction with the charts at all - it is probably worth doing some interaction after creating the charts and then seeing if there are even more leaks introduced as a result of interacting with the chart.

Last edited 7 years ago by ben hockey (previous) (diff)

Changed 7 years ago by ben hockey

Attachment: shapeRegistryTest.diff added

Changed 7 years ago by ben hockey

comment:20 Changed 7 years ago by ben hockey

i went ahead and added a test with some interactions. without kris's patch, there are 52 shapes remaining and with the patch there are 25 shapes remaining.

NOTE: i had to put the destroy code in a setTimeout because it seemed that something was still trying to interact with the charts after they were destroyed. i suspect this could be caused by the onChange of the NumberSpinner? firing asynchronously. ideally though, everything should work without the setTimeout but that's probably a different issue.

comment:21 Changed 7 years ago by cjolif

thanks Ben, I'll be looking into this and try to find an alternate patch that gives similar results without breaking enableCache.

comment:22 Changed 7 years ago by cjolif

At least for the first part of the patch (on Chart.js) I suspect the fix must instead be done at gfx level. Indeed the chart destroy method already call surface.destroy(). This should IMO destroy all children from the registry without requiring any additional work from charting. pruzand will look at this part of the fix.

comment:23 Changed 7 years ago by cjolif

see #15907 for the gfx destroy leak.

comment:24 Changed 7 years ago by cjolif

In [29603]:

refs #14589. Now that #15907 is fixed, just use surface destroy to destroy all children in surface.

comment:25 Changed 7 years ago by cjolif

In [29604]:

refs #14589. Disposes gfx element groups we don't use anymore to prevent leaking. !strict.

comment:26 Changed 7 years ago by cjolif

In [29605]:

refs #14589. Prevents legend to leak it gfx objects.

comment:27 Changed 7 years ago by cjolif

In [29606]:

refs #14589. Put back surface destory at the end to avoid re-destroying objects we already destroyed.

comment:28 Changed 7 years ago by cjolif

In [29607]:

refs #14589. Backport mem leak fixes to 1.8 branch. !strict.

Changed 7 years ago by cjolif

Attachment: test_StoreSeries-amd.html added

updated test case that also destroy legend widget (to verify we are down to 0 shapes in the end)

comment:29 Changed 7 years ago by cjolif

Blocked By: 15907 added

With the latest commits we are down to 0 leaked shapes in the updated test case for trunk and 1.8 branch. 1.7 branch fix is waiting for gfx #15907 being backported to 1.7 branch.

comment:30 Changed 7 years ago by cjolif

Resolution: fixed
Status: reopenedclosed

In [29612]:

fixes #14589. Backport mem leak fixes to 1.8 branch. !strict.

Note: See TracTickets for help on using tickets.