Memory Leak in Parsing

[jkrot]

https://github.com/chafey/dicomParser/blob/34c02a7cec6b71a68ae21856029a8f5d957e900e/src/readSequenceElementExplicit.js

So I have been doing tests for parsing 1000+ files and I keep running into a memory leak/ inefficient garbage collection happening with these functions. It is making out my heap and causing the parsing to crawl waiting for garbage collection to finish. I was wondering if anyone else ran into this problem. Also I think I may be able to fix the underlying issue but I might need to talk it through with @chafey

[jkrot]

Fyi good reading on this topic is
http://stackoverflow.com/questions/1596782/how-to-unset-a-javascript-variable
#4 https://www.toptal.com/javascript/10-most-common-javascript-mistakes
https://auth0.com/blog/four-types-of-leaks-in-your-javascript-code-and-how-to-get-rid-of-them/

[yagni]

What's your use case and what browser? I've been using the library to parse a 2GB directory (about 3200 files) with less than 150MB memory usage. Once I parse the files and extract the field values I need, I unset the byteArrays and that keeps the memory around 130MB. Are you sure it's not due to how you're using the objects?

[chafey]

I checked the source file you linked and don't see any unsetting of variables - did you mean to link to another file?

[jkrot]

So I used that as an example of previous stuff, I think what is happening is that a fileReader is getting set for each file and not just one fileReader for all of them and parse them one at a time.

[chafey]

OK so you think your leak is outside the library then? If so, please close

[jkrot]

I am still confirming that is the case, I am not sure but I am still
seeing some memory issues even with my changes.

On Fri, Aug 5, 2016 at 8:10 AM Chris Hafey notifications@github.com wrote:

OK so you think your leak is outside the library then? If so, please close

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#54 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAHmqx3BVcpWl2Vbb6guwp8XtXOoqx20ks5qczZagaJpZM4Jbx4X
.

[chafey]

@jkrot any update on this?

[jkrot]

I have made headway on this https://github.com/chafey/dicomParser/blob/master/src/parseDicomDataSet.js line 36 & 70 declaring the variable in the loop was causing a heap leak for me. I am still poking around because it is hard trying to find all the places this type of leak is happening but I am making progress. I will get a commit up by this weekend.

[chafey]

OK this isn't really a "leak" per say but more of a "using more heap than is necessary". The issue is that javascript does not hoist initialized variables. If we moved the declaratio out of the loop or declare in the loop but not assign we should be good. I am going to be working on dicomParser today and will see if I can fix this. Thanks

[chafey]

Useful references:
http://www.w3schools.com/js/js_hoisting.asp
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Memory_Management

[jkrot]

Correct the problem is I am maxing out my heap so when that happens the parsing goes to an absolute crawl waiting for the garbage collector to clean up so it can be used. Part of this was my fault on how I am using the reader which I fixed but part of it is some optimization in the parsing code for better heap management without waiting for the collector.

I guess you never expected to parse several thousand files from disk in parallel.

[jkrot]

I also played around with https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/delete

[chafey]

ok I'll wait for your PR, please rebase with latest first

[jkrot]

So the only way I was able to resolve this was to setup workers and to destroy them when done. I tried forcing the collection to happen in multiple places tracing through the stack in the code and just couldn't get it to happen. I suspect this is more to do with the fileReader then dicomParser but I tried setting my fileReader = undefined; in the onloadend function for it and that didn't help. I traced back from dicomParser.explicitElementToString util all the way to my promise chain for processing these files and just couldn't get it to free up the memory along the way. I still feel like there is room for improvement somewhere but I can't seem to narrow it down. I do know this is because max heap size is being hit with over 1000+ files and those files get parsed in under 30 seconds so the collection doesn't have time to run.

[chafey]

Any update on this? Please close unless you have evidence of a real issue