archimatetool / archi-scripting-plugin

jArchi - Scripting for Archi: ArchiMate Modelling Tool
https://www.archimatetool.com
118 stars 33 forks source link

Memory leaks using Nashorn #89

Closed Phillipus closed 3 years ago

Phillipus commented 3 years ago

I've been testing jArchi for memory leaks.

I found two places where Java object references are being held so that they can't be garbage collected - com.archimatetool.script.commands.CommandHandler and com.archimatetool.script.dom.model.CurrentModel.

Both of these have static fields which reference EObjects and the current Model. If these are large objects they can consume memory which is not freed when the model is closed.

I have a fix for this so that those fields are set to null after the script is run. I tested using GraalVM and the objects are now freed and garbage collected.

However, this is not the case for Nashorn.

Even though jArchi frees up references to objects, Nashorn continues to maintain references to them internally, The objects in question are the instance of of an IArchimateModel in com.archimatetool.script.dom.model.CurrentModel and an EObjectProxyCollection in com.archimatetool.script.dom.model.Selection.

These are ScriptEngine bindings that Nashorn references internally.

All you have to do to trigger this is run this jArchi script on a selected large model:

$('concept').each(function(concept) {

});

Then close the model. Heap status will show that the objects are still in memory.

I spent a lot of time investigating why these bindings are not being freed and then I found this:

https://bugs.openjdk.java.net/browse/JDK-8229011?page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel&showAll=true

It's a Nashorn bug since Java 9.

So I've refactored a lot of the code to implement a dispose() method on DOM objects. This will nullify references inside of each binding to mitigate the problem. However, Nashorn will still maintain references to the bindings internally. All we can do is make sure the bindings' fields are cleared or set to null.

I'll merge my changes soon once I'm happy with them.

Note - this is not a problem using GraalVM. I'm hoping that tests with GraalVM will be positive and we can stop using Nashorn.

Phillipus commented 3 years ago

@jbsarrodie This is nuts. I'm fighting a losing battle here.

I thought I'd nailed it.

If I do this:

var selected = selection.first();

or this:

$('concept').each(function(concept) {

});

Then the reference to selected is freed by Nashorn.

But if I do both:

var selected = selection.first();

$('concept').each(function(concept) {

});

Then the reference to selected is retained internally by Nashorn and can't be accessed to free it.

We have to accept that the bug in Nashorn runs deep and try to move to GraalVM.

(That cost me a weekend ;-))

jbsarrodie commented 3 years ago

We have to accept that the bug in Nashorn runs deep and try to move to GraalVM.

Let's make it simple: This bug has been there for a long time, but I never faced any real issue in the real world, even on quite big models. So let's keep it as it is, it will fixes itself when GraalVM support will be mature enough and we'll remove Nashorn.

Phillipus commented 3 years ago

This bug has been there for a long time

Probably since we moved to Java 11 (Archi 4.5, August 2019)

but I never faced any real issue in the real world,

I can tell you why - some time ago I discovered that an IArchiMateModel might be retained internally even after closing it in the model tree. So I added a IArchiMateModel#dispose() method to clear out child objects when the model is closed in the UI. So even if the object is retained its child objects are freed.

This won't help though if a model is created in a jArchi script but not opened and then closed in the models tree.

Phillipus commented 3 years ago

I'll close this since it's served its purpose to record the issue. I've done what I can.