NWNX / nwnx2-linux

Neverwinter Nights Extender 2 - Linux version
http://www.nwnx.org/phpBB2/
Other
43 stars 27 forks source link

NWNX ResMan memory leak. #18

Closed jd28 closed 9 years ago

jd28 commented 10 years ago

Wrote a post on the nwnx.org about what I think is causing this:

I formed a hypothesis about what it's causing the memory leak in nwnx_resman. From what I can tell it only occurs when an external resource is loaded that is not represented in one of the Key tables.

Load some file -> GetResObject from key table, if none or no entry exists, create new res object -> SetResObject in the key table, if there's an entry -> Request/Demand actual file data.

So... if the file isn't in one of the key tables, it's leaking the CRes[...] and file data every Demand. At least that's my theory.

I'm not sure what the best way to go about fixing it would be tho.

virusman commented 10 years ago

Yes, that looks like the cause, but it seems like the only way to fix it is to modify the key table, and that's not very easy, even with symbols.

virusman commented 10 years ago

Although afaik CRes instances are registered in ResMan cleanup list, so theoretically they should be collected somewhere down the road, but it either doesn't happen or it doesn't clean up completely.

jd28 commented 10 years ago

Hmm, I spent some more time looking at this after you mentioned the cleanup lists and I'm starting to think the engine (for all intents and purposes) just never deallocates CResNCS instances. That would explain why those loaded by nwnx_resman leak and the effect is unnoticeable when the file is in a KeyTable.

They are added to the cleanup list but aren't ever removed for deallocation - so far as I can tell - only when a new request/demand comes in. Maybe they would be in case of the CExoResMan being out of memory.

So maybe just a cache in nwnx_resman would fix the issue, I'm going to test it out in a bit. I don't think the issue occurs with the other CRes instances, GFFs seemed to properly release memory anyway. Only problem would be if the CExoResMan ran out of memory.

virusman commented 10 years ago

CResNCS requires additional initialization - maybe it allocates some resources at that point that don't get released?

jd28 commented 10 years ago

I'm pretty sure now that the engine just doesn't free CResNCS except maybe no memory and shutdown. Maybe this was the change that made the module script cache obsolete.

I expanded resman to cache for NCS resources and updated a few other things here: https://github.com/jd28/nwnx2-linux/tree/resman_memleak/plugins/resman In case anyone would like to test/see what I think.

In my tests memory consumptizon is in line with non-resman usage. No obvious leaks.

There are still some issues tho:

jd28 commented 10 years ago

I updated my branch. I found there was an issue with CRes2DA: They need additional initialization like CResNCS. I also updated everything to the 2.8 API which makes it all a bit cleaner.
There are still issues with 2das.

I smashed everything in to one commit, since I changed a number of things and some of the files had been committed with windows line endings so I got annoyed with it.

If you're interested in a pull request after further testing, I can redo the commits however you prefer and can change the spaces to tabs too...

jd28 commented 9 years ago

Pull request #41 fixes this.