search

prometheus-junkyard / promdash

Prometheus Dashboard Builder
http://prometheus.io/
Apache License 2.0
271 stars 48 forks source link

Rickshaw graph memory leak #64

Closed juliusv closed 10 years ago

juliusv commented 10 years ago

When graphs are re-rendered, we always create a completely new Rickshaw graph. However, the old graph object is still retained in memory and not garbage-collected for some reason. This memory leak eventually causes the browser tab to crash.

Steps to reproduce in Chrome:

The profile inspector also allows you to look at an object's retaining tree (i.e. which parent object references cause this object to not be garbage-collected), but I haven't been able to decipher yet by what exactly the graph is being retained.

Unfortunately Rickshaw also doesn't seem to provide a way for "destroying" a graph through its API.

To be done: figure out what causes the graph objects to be retained and fix it.

stuartnelson3 commented 10 years ago

Fixing the memory leak would fix the problem, but I'm thinking that if we change redrawGraph() to update the data series, we might avoid the whole issue entirely by not creating new graph objects each time something changes.

juliusv commented 10 years ago

Yes, that could also be a solution. I always reasoned that by creating a completely new graph every time we redraw it, we can be sure that it is in 100% pristine and correct state, whereas by reconfiguring an existing one, there's always a slight risk of picking up some kind of unwanted internal state changes which never get completely reset between reconfigurations. Remember, we're not only sending new data, but need to reconfigure legend settings, axes (in the future), and other graph settings every time the user changes something.

So while I think changing the existing graph could work, I'd prefer to do some amount of research into whether we can fix the memory leak in the existing implementation and if that turns out to be too time-consuming, we can try to go the way you suggested. But I'm happy to hear your opinion about it!

stuartnelson3 commented 10 years ago

Spent a good chunk of time on this yesterday. Rickshaw.Graph, Rickshaw.Graph.Axis.Time, and Rickshaw.Graph.Behavior.Series.Toggle are all hanging around in equal number, and it looks like it's a circular reference issue. The Rickshaw.Graph.Axis.Time and Rickshaw.Graph.Behavior.Series.Toggle objects all reference Rickshaw.Graph, and Rickshaw.Graph seems to be getting retained because the hover detail holds a reference to it (and, if I'm reading this right, every single Rickshaw.Graph created): screen shot 2014-02-21 at 9 51 57 am If you match the @+number identifiers up, it looks like @228931's retaining tree through graph in the Retaining Tree lower section has references to every Rickshaw.Graph created.

I worked on removing the references to these yesterday so that the GC would clean them up, but this introduced errors in the console when hovering over the graph. I believe this is because the hover detail was attempting to reference properties I had removed, which further seems to reinforce that the hover detail is holding references to all the graphs.

Will continue to work on this, just thought I should provide an update since I have no pr to indicate progress.

juliusv commented 10 years ago

Awesome, thanks for the update! We're already getting a JS error in the current version after graphs have been resized for the first time: Cannot call method 'getBoundingClientRect' of null. I always assumed it was due to garbage hanging around. Were you seeing the same error message or a different one?

stuartnelson3 commented 10 years ago

Different error. I think the 'getBoundingClientRect' was because of the if (rsGraph != null) statement. I think clearing the inner html was removing the reference that the hover display was trying to access.

I tried to do my cleanup in there (located in redrawGraph() in graph_chart.js), and found several other errors when an existing object was attempting to reference its graph attribute, which I had deleted. I believe the object is the hover display. I'll keep working on it.

stuartnelson3 commented 10 years ago

the getBoundingClientRect error is because hoverDetail.element.parentNode is null. hoverDetail.element seems to get detached from the DOM yet still gets displayed on hover...? weird.

stuartnelson3 commented 10 years ago

Almost done. Just one object left is leaking, Rickshaw.Graph.Behavior.Series.Toggle. Hope to wrap this up tonight.

juliusv commented 10 years ago

\o/

So is what you've found so far fixable without making the code look like crazy? Does Rickshaw need to be changed for it?

On Wed, Feb 26, 2014 at 3:16 PM, stuart nelson notifications@github.comwrote:

Almost done. Just one object left is leaking, Rickshaw.Graph.Behavior.Series.Toggle. Hope to wrap this up tonight.

Reply to this email directly or view it on GitHubhttps://github.com/prometheus/promdash/issues/64#issuecomment-36128800 .

stuartnelson3 commented 10 years ago

Crazy is ... subjective. I'll put up a PR so you an see what it looks like right now, but there's a lot that I'll be able to do to clean up the code once I fix the last bit.

I made a small change to rickshaw, removing the use of itemByName method. It was causing funny behavior when re-rendering the legend when the lines had the same names. If they had the same name the loop that searches for the items by name would always return the same line. https://github.com/shutterstock/rickshaw/blob/master/src/js/Rickshaw.Series.js#L129

juliusv commented 10 years ago

Ok, if we can't get around patching upstream dependencies, we need to find a good way to manage those changes ontop of them. I would say, in order of preference:

stuartnelson3 commented 10 years ago

I will attempt the second bullet point, and then the third. I can look further into a local fix, but nothing was immediately evident last night.

The change is fairly simple. Any place in the project that uses the someFn(itemByName(item.name)) method is changed to someFn(item). This change was made in two places in our vendored copy of rickshaw (the only two places itemByName is used).

juliusv commented 10 years ago

Cool! I'm not sure if I understand it 100%, but it sounds like bug fix which would be generally useful for Rickshaw?

stuartnelson3 commented 10 years ago

Right. If you attempt to re-render the graph and your each line in your series as the same name, you end up having a legend where every single element is for a single line.

juliusv commented 10 years ago

I see! Cool, let's try to submit it to Rickshaw then! Even if they disagree with the exact way you made the change, it seems like a bug which needs to be fixed (ok, they could argue that no sane person would usually have multiple timeseries of the same name in a graph, but hey). In any case, if they already have a reference to the item, the itemByName seems kind of superfluous.