Export to GitHub

flotr - issue #5

Memory Leak when plot is redrawn periodically (PrototypeJS)


Posted on Apr 24, 2008 by Happy Panda

My goal was to redraw a plot from data polled from a server, giving the appearance of a live- updating chart. I maintained a variable to contain the data, then on each interval, pushed new data onto the array and redrew the plot (BTW, this is visually very appealing, with the plot redrawing smoothly - very nice!).

I noticed that the browser's real memory use was growing in time with each redraw. At first I assumed it was my growing data array, but the increase was too large to be explained by a pair of numbers and the size of the array was capped at 1000 elements.

I created an isolated test case and proved that the memory increase occurs with each redraw regardless of new data.

What steps will reproduce the problem? (See attached test case) 1. Load page with initial flotr plot, open a process monitoring tool and view browser mem use 2. Redraw chart (in test case, by clicking button) 3. Note memory use. 4. It may take four to six redraws before problem becomes apparent, but once it manifests, it is consistent. 5. Periodically click 'Redraw Only' button and note that memory use increases in response to each click.

What is the expected output? What do you see instead? The expected result is that memory use would increase to a stable level and remain flat for each subsequent redraw. Actual result is that memory is consumed until browser fails.

For Safari and Firefox under OS X the memory consumed with each redraw is very consistent, about 2 mb each time. For my real application, broswer failure occurs very consistently.

Under Windows and IE, the memory consumed was less for each redraw, the increase for each redraw ranged from 36k to 156k

What version of the product are you using? On what operating system? flotr-0.1-alpha with prototype-1.6.0.2 Reproduced on Mac OSX under Firefox and Safari Reproduced on Windows XP under Internet Explorer

Attachments

Comment #1

Posted on Apr 25, 2008 by Happy Panda

Additional data - after some additional research on common causes of memory leaks I discovered what I think is the issue. In the 'bindEvents' method, calls to 'observe' are made. If the plot is rendered once and the page is discarded this isn't a problem, but if the plot is re-rendered without a page refresh (ie using Ajax) there is no way to call 'stopObserving', the fact that the dom still holds a reference to the mouse/click event methods appears to prevent the GC from freeing the memory of the previous plot.

I proved this by commenting out he calls to 'observe' and retested - the memory leak did NOT reproduce. I wish I knew more about javascript so I could attach a patch, but I'm not sure how I'd solve this, ideally there would be a call which could be made on a flotr reference that would redraw the plot without having to rebuild the whole object. Alternatively a method which could be called just before an ajax-triggered redraw that would wrap calls to 'stopObserving' to release references from the dom to methods declared in the js object.

Comment #2

Posted on May 15, 2008 by Massive Dog

iirc: javascript doesn't have any sort of code "protection" so you could (possibly) foo._ajaxTriggeredRedraw = foo.ajaxTriggeredRedraw();

foo.ajaxTriggeredRedraw = function() { this.stopObserving(); // very poor "super" idea this._ajaxTriggeredRedraw(); }

this is an ugly thing to do, but i have used it when I wanted to semi-transparently modify some code I didn't have the ability to alter (due to it's use in other places).

In general, doing this is a bad coding practice.

Comment #3

Posted on Jun 8, 2008 by Happy Rabbit

I have the exact same problem.

I have reproduced this leak with Flotr revision 34 (2008-06-05 07:55:08) on Firefox3RC2, Safari3.1.1 and IE7 running on WinXP with both mootools-release-1.11.js and prototype-1.6.0.2.js version.

I have even tried it with mootools-trunk-1555-compatible.js (just have to modify the getSize().size.x to getSize().x at lines 500 and 797 for it to work in compatible mode...)

I have attached an example that eats up vast quantities of memory in no time (I have slightly modified the example basic.html to refresh the graph every 100ms)

If I look into the SVN history, I see that calls to stopObserving() have been removed (I don't have a clue as to what that does but it was mentioned in the previous post). Is this bug getting any attention ? (it's priority is only set to medium)

Unfortunately, this is a show stopper for me since I was also planning on refreshing the graph using dynamic data from an ajax feed.

Attachments

Comment #4

Posted on Jun 17, 2008 by Happy Giraffe

Same problem here :( I think there should be some interface provided to access private members of Flotr. Or just for that case some shutdown() or even redraw() methods could help.

Comment #5

Posted on Sep 6, 2008 by Massive Bear

I have also reproduced the problem.

Comment #6

Posted on Sep 11, 2008 by Massive Rhino

You can't use the trick proposed by rhansen, these are private functions and cannot reassigned like that. What you can do, however is this: Find the function Flotr.Plot.bindEvents and change it from this:

function bindEvents() { if(options.selection.mode != null){ overlay.observe('mousedown', mouseDownHandler); } overlay.observe('mousemove', mouseMoveHandler) overlay.observe('click', clickHandler) }

to this: function bindEvents() { if(options.selection.mode != null){ overlay.observe('mousedown', mouseDownHandler); overlay.observe('mousemove', mouseMoveHandler) overlay.observe('click', clickHandler) } }

I do not know if this fixes the problem for people that use the mouse options as we don't use mouse functionality. But it seems to be a simple bug causing mouse events to be registered always instead of when they're needed. -- Richard June

Comment #7

Posted on Nov 12, 2008 by Swift Bear

I fixed this problem.

By adding a global array to record the binding at every time. Then stop observing them at the Clear function.

Comment #8

Posted on Nov 30, 2008 by Massive Bird

Hi all. The memory leak is caused by re-creating a lot of objects (canvas and divs) when calling the draw function.

Attached is a patched version of the 0.1.0 alpha version to avoid the memory leak. (Maybe the ideas in the patch can be used into the latest SVN version---see readme file inside the file attached).

You can compare the memory consumption with the peter.hewat test case (see comment #3).

Hope this can help you.

-- Albert Aymerich

Attachments

Comment #9

Posted on Dec 1, 2008 by Massive Panda

Hello Albert Thank you for your patch, I don't know if this is the best method, but we are sure it will work.In order for this method to work, we'll have to look at the latest revision in the repository, which is really different from the 0.1 alpha. I'm giving a look at it now.

Comment #10

Posted on Dec 2, 2008 by Happy Bird

Comment deleted

Comment #11

Posted on Dec 2, 2008 by Happy Bird

Albert, thank you for posting your fix! I, and I'm sure many others, really appreciate it.

Comment #12

Posted on Dec 5, 2008 by Massive Panda

I've changed the behaviour of the constructCanvas function and a few others not to always empty the container, but to try to find existing canvas and divs of a possible existinf plot. An update function is then useless, you only have to call the drax function back!

I invite you to test the latest revision from the repository

Comment #13

Posted on Dec 6, 2008 by Massive Bird

Comment deleted

Comment #14

Posted on Dec 6, 2008 by Massive Bird

Hi Fabien

I've done some tests with the r57 from the repository. The main part of the memory leak (canvas and labels objects) has been solved... but the leak still remains with the legends when using div elements.

The same code done for drawLabels can be used inside the insertLegend function to remove the leak:

var legends = this.el.select('.flotr-legend')[0]; if (legends) { legends.childElements().invoke('remove'); legends.remove(); } var legendsBg = this.el.select('.flotr-legend-bg')[0]; if (legendsBg) { legendsBg.childElements().invoke('remove'); legendsBg.remove(); }

At last... but not related to the memory leak, the r57 examples doesn't work with IE

Regards Albert

Comment #15

Posted on Dec 7, 2008 by Massive Panda

Hi ALbert ! Thank you for your tests :) I've added your patch to the latest revision. But the examples work for me on IE (IE8 in IE7 quircks mode, it won't work on IE8 Beta2)

Comment #16

Posted on Dec 7, 2008 by Massive Bird

OK... my fault on IE. All works as expected on my IE6(xp) with the latest svn revision of lib folder (excanvas.js, ...).

albert

Comment #17

Posted on Dec 8, 2008 by Massive Panda

Flotr now works on IE thanks to the latest Excanvas revision ! (in the latest Flotr revision) :)

Comment #18

Posted on Dec 26, 2008 by Massive Bird

Hi... with latest revision on svn (r65) the memory leak has come back. I see the new method to remove child elements, but need time to see why it's happening again.

Comment #19

Posted on Dec 26, 2008 by Happy Giraffe

I feared that :( I've cancelled my first attempt to avoid the memory leak because it was too hard to maintain, I think the canvases should not be removed and be reused when the plot is drawn back.

Comment #20

Posted on Dec 26, 2008 by Massive Bird

I agree. It was hard to maintain.

The leak is mainly due by creating the new canvases, so we need to reuse the first canvas that was created.

I attach a file with a modified constructCanvas function with the next algorithm: 1.- search for old canvas (canvas and overlay) 2.- assign to this object 3.- remove all child elements from container (old canvases included):

this.canvas = el.select('.flotr-canvas')[0]; this.overlay = el.select('.flotr-overlay')[0]; el.childElements().invoke('remove');

4.- if they didn't exist, then create the new ones 5.- if they existed, then insert then into the container

// Insert main canvas. if (!this.canvas) { c = this.canvas = new Element('canvas', size); c.className = 'flotr-canvas'; c = c.writeAttribute('style', 'position:absolute;left:0px;top:0px;'); } else { c = this.canvas.writeAttribute(size); } el.insert(c);

(and the same piece of code to this.overlay object )

With this modification, only we must care about canvas objects, so it should be easier to maintain.

albert

Attachments

Comment #21

Posted on Dec 26, 2008 by Massive Bird

Comment deleted

Comment #22

Posted on Dec 26, 2008 by Massive Bird

The previous code was based on svn (r65)

albert

Comment #23

Posted on Dec 27, 2008 by Massive Panda

I've applied your patch, it should work fine, and the memory leak should be limited now.

Comment #24

Posted on Jan 7, 2009 by Happy Bird

Hi I have the memory issue too, seems to be fixed in IE. I still see the issue in Firefox. Does anyone have the issue and any solutions. Thanks Kenny

Comment #25

Posted on Jan 8, 2009 by Happy Giraffe

Hello Kenny, do you use the latest SVN revision or the zip ?

Comment #26

Posted on Jan 12, 2009 by Happy Lion

Hello - I am having the memory leak problem too. Can anyone email me a build of flotr that solves this memory leak problem? (I do not know how to create a build from the SVN release. I have found the instructions on how to do the build - but I do not have or know how to use ANT etc. Sorry about that. Hope someone can help. Thank you.)

yella123@yahoo.com

Comment #27

Posted on Jan 29, 2009 by Happy Wombat

I am getting the same memory leak with the r28 mootools version of flotr under FF3(XP). I have a chart being updated via JSON every 10 seconds and the memory for the FF process jumps 2Mb each time. Visually, the updates are seamless and the charts otherwise work perfectly so I would love it if I can get a fix for this issue.

I noticed there are some potential solutions for the prototype version. Has the latest prototype version been fixed successfully? Will the mootools version be updated to include this fix? I would switch to prototype but have already put a lot of mootools specific code into this project. Are there any changes I can make to the r28 mootools version of flotr that I can use to patch the leak?

Comment #28

Posted on Jan 29, 2009 by Massive Panda

Hello The Mootools version is REALLY out of date. There has been so much work done on the Prototype version that we wait to have finished this work to update the Mootools one (there are a few pending tasks that have to be done to be able to say it is finished) and Bas needs to have more time to be able to work on it. I think there are no more memory leaks in the Prototype version. Personally, I don't waork with Mootools, as Bas does, but with Prototype, that's why the features have been added to the Prototype version, not the Mootools one. I'll try to fix a little bit the Mootools version though.

Comment #29

Posted on Apr 27, 2009 by Massive Panda

I'll reopen this issue if new memory leaks are found

Comment #30

Posted on Sep 14, 2009 by Happy Wombat

I´m having the memory leak problem with r149 from the reposiroty under firefox 3.2

Comment #31

Posted on Sep 14, 2009 by Happy Wombat

What i basically do is:

function drawstuff() {

    new Ajax.Request('get_data.php', {
            method:'get',
            onSuccess: function(transport){
                /**
                * Parse (eval) the JSON from the server.
                */
                var json = transport.responseText.evalJSON();
                if (json.length > 0) {
                    $(container).innerHTML='';
                    f = Flotr.draw(
                        $(containerString), [
                        { // => first series
                            data: json,
                            label: bla,
                            lines: {show: true, false: true,lineWidth:1,fill:true, fillColor:'ccddee'},
                            points: {show:true, radius: 1},
                            selection: {
                            mode: 'x',
                            color: '#aabbcc',
                            fps:10
                            }
                        }],{
                          xaxis: {
                            mode: "time",
                            minTickSize: [1, "second"]
                            },

                        }

                        );
                } // Length > 0

            }

}

function timer() { drawstuff(); setTimeout("timer()",3000); }

Comment #32

Posted on Sep 15, 2009 by Helpful Rabbit

I am having a similar issue in both IE8 and FireFox 3.5. Attached is a very simple test case that exhibits the issue.

Note, this calls to console.log which will kill IE unless the line is commented out.

Attachments

Comment #33

Posted on Sep 16, 2009 by Helpful Rabbit

Another note: the problem is not exhibited in r125.

Comment #34

Posted on Sep 16, 2009 by Massive Panda

Hello, I'll have to investigate about this, as it can be very annoying. Thank you for the last comment, this will help me to find the regression.

Comment #35

Posted on Sep 16, 2009 by Helpful Rabbit

One more thing, the problem shows up (I believe) between r128 and r130. I've been unable to narrow it down farther (the delta between r128 and 130 is substantial).

Comment #36

Posted on Jul 20, 2010 by Happy Dog

This patch seems to fix it; apparently it was caused by the refactoring into plugins; those plugins can register event handlers which are not unregistered when drawing a new flotr plot.

I'm not sure this is the best way to fix it (since some callbacks might register generic browser events that client code might also be observing), but there doesn't seem to be a way to unregister exactly those handlers that were registered by Flotr (since the bind method call creates a fresh anonymous function everytime, which we can't refer back to).

Fabien: I don't seem to have commit access, so could you please commit this ASAP? This has been in the issue queue long enough :)

Attachments

Comment #37

Posted on Jul 20, 2010 by Massive Panda

Thank you for this patch, I'm commiting it now.

Comment #38

Posted on Jul 21, 2010 by Happy Dog

Correct me if I'm wrong, but it seems to me that the way you fixed it makes it impossible for several different plugins to register the same event.

As it would loop through the plugins, plugin A would unobserve event X, then observe event X again. Plugin B would be next, and it would unobserve event X (unregistering plugin A's observer in the process) and then observe event X, etc.

I really think you need to do it like in my patch; first loop through the plugins and unobserve any events they observe, and then loop through them a second time to attach the event handlers.

Comment #39

Posted on Nov 17, 2010 by Swift Monkey

Even with the latest source from the SVN, I still have memory related problem.

I'm pretty sure that my problem is caused by Flotr, because if I comment the line Flotr.draw..., then I don't have any problem.

In my case, it's not the same graph that I have to draw periodically, but different ones. Basically, I have multiple tab, which all contain one graph each.

Is there anything that I can do?

Right now, all graphs are actually drawn in the same div, which I append to the tab before it's activated.

Comment #40

Posted on Nov 22, 2010 by Happy Dog

It could be caused by your code or by an interaction between your code and Flotr. It could also just be that your usage pattern triggers the bug in Flotr more frequently.

It would be nice if you could post your code, or if possible, a simplified version which causes the same problem.

PS: Which browser? All of them or just IE like with the OP's problem?

Comment #41

Posted on Nov 22, 2010 by Happy Dog

Comment deleted

Status: Fixed

Labels:
Type-Defect Priority-Medium