Skip to content
This repository has been archived by the owner on Jul 29, 2019. It is now read-only.

Memory Leak in Network Visualization #3439

Closed
fatore opened this issue Sep 12, 2017 · 17 comments
Closed

Memory Leak in Network Visualization #3439

fatore opened this issue Sep 12, 2017 · 17 comments

Comments

@fatore
Copy link

fatore commented Sep 12, 2017

As it can be seen in the attached image, I've found significant leaking in the network visualization. By the end of my component life-cycle I have a block like this:

this.graph.off('stabilizationProgress');
this.graph.off('stabilizationIterationsDone');
this.graph.destroy();
this.graph = null;

However, that doesn't seem to be enough to clean all the bound listeners, causing the leaking.

Visjs: version 4.20.1
Browser: Chrome Version 60.0.3112.113 (Official Build) (64-bit)
OS: Windows Server 2012 R2 Standard

visjsnetworkleak

I would appreciate if someone could provide some guidance on how to properly clean the network visualization or acknowledge this as a bug.

@fatore fatore changed the title Memory leak in Network visualization Memory Leak in Network Visualization Sep 12, 2017
@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 12, 2017

Are you loading images into nodes? There's a known possibility of leakage there.

In any case, an example network would be useful here.

Edit: Holy crap 10K+ nodes in network. OK, so an example might be hard to come by. Still hoping, though.

@wimrijnders
Copy link
Contributor

Another question: Are you using smooth type dynamic by any chance? if so, perhaps you can try the same thing with smooth type continuous.

Reason for this: dynamic adds helper nodes, normally invisible; these might not be cleaned up properly.

@fatore
Copy link
Author

fatore commented Sep 12, 2017

Thanks for the fast reply @wimrijnders.

We are not exactly using the value image for the property shape in nodes, but we are indeed using icon, so I guess the same issue would happen?

And you are right about the edges, we have set:

smooth: {
  enabled: true,
}

which defaults to type: dynamic.

I'll try your suggestion to use continuous and I'll come back.

@wimrijnders
Copy link
Contributor

OK. With images, I know that there is leakage possible, which in fact is preventable. I do not believe that the same applies to icons. There must be another thing going on.

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 13, 2017

I played around with the chrome performance profiles for a while; there's lots of information there, most of which I currently partly understand. But I can see the potential for using this for performance optimization. TIL.

But now I do realize that the nodes and listeners mentioned in the image above are not the same thing as nodes and listeners within Network, i.e. there is no direct mapping. So there is no direct relation between these nodes/listeners and the objects used in Network. This makes it somewhat less useful at this point.

With respect to analyzing the memory leaks, I think the thing to do here is to see what remains after graph.destroy() is called. The usual suspects are the (Network) nodes and edges, which might still be hanging around. There might be more stuff still referenced, but these will be the biggest.

@wimrijnders
Copy link
Contributor

You must have a huge network loaded BTW. I'm testing here with a 1250-node network and the performance-screen only shows a maximum of 84 nodes and 21 listeners.

Is there any chance of a minimal example to recreate the characteristics of the graph above? That would be a starting point.

@fatore
Copy link
Author

fatore commented Sep 13, 2017

Hi @wimrijnders, I also find that performance tab super useful, it's a life saver in cases like this.

Sorry for misleading you with the nodes information, it might be indeed not very useful for this kind of analysis. On the other hand, I usually find the listeners count useful because (from my understanding) after a proper clean-up it should go back to the value it was before.

I've applied your advice to change the property smooth.type for edges from the default value dynamic to continuous and there was a great improvement.

As you can see in the image, in terms of memory itself there is minimal leakage now. There is still, however, several listeners that are still alive, 104 - 87 = 17. But again, I'm not sure about the expected behavior for the listeners count after the garbage collection.

You might notice that there are less nodes and listeners now. The graph data is the same as before, but this time I had the graph opened just once to keep the results simpler. It contains 377 nodes and 726 edges. I'll see if I can set some sort of Plunker to help on this investigation.

leadking-cont

@wimrijnders
Copy link
Contributor

👍 Great feedback! Thanks for running the test.

While it's still mostly vague to me, there is one outstanding conclusion: the hidden nodes are not cleaned up properly. It's good to have a confirmation of this.

Having said that, there are fixes in the current develop-branch which address the cleanup of nodes/edges......#3330 (had to search a bit). A next obvious step is thus to see if latest develop handles this better:

  • One option is if you make that example, would be very much appreciated.
  • Another option is if I make a custom build for you so you can test with your original network. I would actually prefer this option, since you can compare with your previous run(s).

The second option is a bit more work for me, but I think it will be more telling. Do you agree? It means you'll have to run the original test again with latest build, I hope that that's OK with you.

@fatore
Copy link
Author

fatore commented Sep 13, 2017

Sure, I can try with develop. But don't worry, if the steps on the README are up to date I can build it myself. I'll let you know if I face some issues or I'll come back with the results.

@wimrijnders
Copy link
Contributor

Ah OK, you have your own cloned repo? If so, so much the better.

(I really appreciate all participatory efforts of users in issues; sincerely thank you)

@fatore
Copy link
Author

fatore commented Sep 13, 2017

Sure, no problem, I'm glad to help.

Are there any breaking changes on develop?
I'm getting an exception:

Uncaught TypeError: Cannot read property 'blocks' of undefined
    at LabelAccumulator.finalize (vis.js:8162)
    at Label._processLabelText (vis.js:9269)
    at Label._processLabel (vis.js:9283)
    at Label.calculateLabelSize (vis.js:8733)
    at Label.draw (vis.js:8510)
    at Icon.draw (vis.js:44718)
    at Node.draw (vis.js:7862)
    at CanvasRenderer._drawNodes (vis.js:51054)
    at CanvasRenderer._redraw (vis.js:50971)
    at CanvasRenderer._renderStep (vis.js:50880)

I wrapped the inner loop in finalize with a if (line && line.blocks) {} just to go over it, but I think that there is something wrong with the lines array, it doesn't contain the first elements so lines[0] is null. The label positioning seems to be wrong because of that.

Anyways, getting back to the performance issue, there is no significant difference to what has been seen in version 4.20.1. So I would say whatever has been done in the develop branch hasn't solved the issue.

While analyzing the differences between the parameters for the edges smoothing, I noticed that my graph no longer goes through the stabilization process when using 'continuous', since events like stabilizationProgress or stabilizationIterationsDone are never triggered. So I guess the hidden nodes that you mentioned are only created during the stabilization process?

The relevant parts of my configuration object look like the following:

{
  nodes: {
    physics: false,
  },
  edges: {
    smooth: {
      enabled: true,
      type: 'continuous',
    },
    width: 0
  },
  layout: {
    improvedLayout: true,
  },
  physics: {
    enabled: true,
    stabilization: {
      enabled: true,
      onlyDynamicEdges: true,
      fit: false,
    },
  },
}

@fatore
Copy link
Author

fatore commented Sep 13, 2017

If I change physics to true for the nodes the leaking is back like when smoothing is dynamic.

Edit: Probably because the stabilization runs again.

@wimrijnders
Copy link
Contributor

@fatore I appreciate the feedback, but I can't seem to get around to handling it, sorry. But I regard this as important and will be back.

@fatore
Copy link
Author

fatore commented Sep 14, 2017

@wimrijnders, that's ok. Your suggestion to use 'continuous' for the smoothing already helped. Thank you for your time.

@wimrijnders
Copy link
Contributor

But the 'conclusion' I made that it had to do with the hidden nodes is clearly false. So this needs further investigation. As Arnold said, "I'll be back".

@wimrijnders
Copy link
Contributor

Are there any breaking changes on develop?
I'm getting an exception:

Uncaught TypeError: Cannot read property 'blocks' of undefined
...

That's a bug that's also recently been reported in #3464. Working on it now. I should have been more observant, you reported it first.

I noticed that my graph no longer goes through the stabilization process when using 'continuous' ... ... I guess the hidden nodes that you mentioned are only created during the stabilization process?

No the stabilization is always run. However, there is a stop criterium in stabilization. Sort of like 'is this good enough? If so, let's stop'. It appears that this criterium is reached really soon with continuous.

If I change physics to true for the nodes....

Aaah, physics was off. Then indeed stabilization does not run automatically.

Your suggestion to use 'continuous' for the smoothing already helped. Thank you for your time.

My pleasure. If you feel we are done here, please close the issue. But I'm not done yet; examining the leakage with stabilization+dynamic is now on my TODO.

@fatore
Copy link
Author

fatore commented Sep 20, 2017

Sure. Thanks again for the conclusion. I'll be looking forward updates.

@fatore fatore closed this as completed Sep 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants