GrantMStevens / amCharts-Angular

Angular directive for amCharts library
Apache License 2.0
63 stars 39 forks source link

Memory usage increasing over time #32

Closed peluprvi closed 8 years ago

peluprvi commented 8 years ago

You can see this on the Docs JSFiddle Example. In this case is + ou - 4K per second, but this grows a lot when you load more charts on the same page. I'm using ngRoute and ng-view and to stop the memory leak, after change the page, I did:

angular.module('app').run([
  '$rootScope',
  function($rootScope) {
    $rootScope.$on('$routeChangeStart', function(event, next, current) {
      if (typeof AmCharts != 'undefined') {
        if (AmCharts.charts.length) {
          AmCharts.clear();
          console.log("AmCharts.clear()");
        }
      }
    });
  }
]);
GrantMStevens commented 8 years ago

How are you profiling this? In running that fiddle, I can't replicate the memory leak you seem to be experiencing.

Even if I could replicate it, you are calling AmCharts functions to release it, thus implying that the memory leak is in their library. Reporting it here does you no good. I'm closing this issue.

peluprvi commented 8 years ago

The problem is along the time. You can replicate this doing the steps:

If you want to see more memory increasing, you can test with more charts, like this Example.

With my code above, I tryed to show that maybe is a problem with some AmCharts event.

PS: sorry if I wasn't clear. English is not my native language. :)

GrantMStevens commented 8 years ago

Yeah, that's not a very good method of tracking memory usage. If you want to track a memory leak, you should be using the profiles tab in chrome dev tools. You can record the heap allocations for that tab and track down memory leaks by watching the resource allocation.

peluprvi commented 8 years ago

On my multicharts example, you can see the timeline on chrome:

image

GrantMStevens commented 8 years ago

That's all good and well that you can see the heap size increasing, but if you want to target a memory leak, you need to be profiling to track it down.

peluprvi commented 8 years ago

I don't know if the problem is memory leak. On the previous example, 2 minutes change the memory from 8.6 MB for 9.4 MB (increasing 0.8 MB). I know this is not frightful and I know this is from amcharts+angular (this timer?). Maybe is some amcharts listener. The problems here are: (1) When you change the route, this behavior don't stop; (2) If you change the route and return to the charts, a new directive content is loaded, having a double problem; (3) If you have some app that needs a lot of interaction and route changes with charts, this will broke in a few hours.

Here, my application, with 6 charts (without change route).

I'm getting this same behavior on Firefox (10 minutes give more 20 MB on memory on my fiddle example).

GrantMStevens commented 8 years ago

I would recommend that you post this on their support forums to see if the AmCharts team can provide you with more information.

Pauan commented 8 years ago

Hi @GrantMStevens,

I am from the amCharts support team. @peluprvi has contacted us about this issue.

When creating/destroying charts dynamically, you must call the clear method before removing the chart from the DOM.

This allows amCharts to cleanup internal properties, timers, and event listeners. If you do not call the clear method, then you will have a memory leak.

Most of the time, charts are not created or destroyed dynamically, so you don't need to clear the chart. But in @peluprvi's case, they are destroying/creating a new chart every time the route changes.

You should be able to fix this by clearing the chart when the $destroy event happens:

$scope.$on('$destroy', function () {
  chart.clear();
});

You should also make sure to clear the chart when the amCharts.renderChart event occurs:

$scope.$on('amCharts.renderChart', function (event, amChartOptions, id) {
  if (id === $el[0].id || !id) {
    chart.clear();
    renderChart(amChartOptions);
  }
});
peluprvi commented 8 years ago

Exactly what I needed, @Pauan.

I tested this solution and now, when I'm changing the route and I'm returning to the charts, the memory usage is the same. This is what I was thinking that was missing in the directive. Now I can remove my $routeChangeStart listenner without problems.

Using amCharts with AngularJS with Pauan's solution (only page 1 has charts):

image

But I still not understanding the number of nodes. As you can see below, there is no node number increasing when using amCharts with jQuery.

Using amCharts with jQuery:

image

GrantMStevens commented 8 years ago

@Pauan Thanks for the heads up on cleaning up those resources. I guess I assumed that the AmCharts lib would clean up its own resources internally. but I suppose in this instance, you guys would have to jump through a whole lot of hoops to determine when to kill off an instance.

I'll add in a fix to correct the issue.

Pauan commented 8 years ago

@peluprvi AngularJS creates lots of nodes, so that could be the reason for the difference.

If the nodes are properly cleaned up (so that memory usage does not increase over time), then I don't think it's a problem.


@GrantMStevens Unfortunately, it is literally impossible for amCharts to automatically determine when to clean up resources.

There is no way in JavaScript to be notified when an object is garbage collected.

We could try cleaning up resources when the DOM node is removed, but mutation observers are not supported on older browsers.

It's the same reason why AngularJS has to go through extra hoops with $destroy in order to properly clean up resources: there's no good way to automatically clean up resources in JavaScript.

peluprvi commented 8 years ago

You are right, @Pauan. The nodes are removed after a garbage collection.

Thank you guys!

jmussett2005 commented 7 years ago

Greetings

The Pauan solution is ok and well.

When we have: $scope.$on('$destroy', function () { chart.clear(); });

This will function always we do not have a reactive suscription running generating the chart x times before we go to destroy. Suscribes has problems of memory leaks with the browser, and is not because of the reactive is because the way how works of the browser and how we write the code. When we run $destroy to clear the chart, it will clear the last state generated in the DOM that will be cleared, it will function ok and if we mesure the levels of memory it will get down. However if we are retriving data with a function and this function is attached to a suscription and into the suscription we define attributes of the chart in periods of time, attributes which means a render layout, color, dimension,etc., we see memory growing up. So we only need be carefull with this considering the reactive behaviour and the way how we want to render the objects.

Thanks.