A warning about Memory Management complacency

bowerandy · March 2013

Folks,

Over the last couple of weeks I have had to solve three fairly major memory leaks that, in the end, were all caused by the same issue so I thought it would be worth mentioning it here to help anyone else who might have the same problems in their code.

One of the things about Garbage Collection is that it appears to be a panacea to finally quash all memory bugs. Unfortunately, it is not! While it is plainly a lot safer and more convenient that having to remember to free objects manually, it is still important to check that one is not leaking memory before releasing anything into the wide world.

I'm my case the issue was to do with using addEventListener() to listen to events from global objects (or at least from objects that were outside the scope of the object doing the listening). I hadn't realized that when using:

xxx:addEventListener("eventName", yyy.func, yyy)

the xxx object will keep a hard link to the yyy object. This will prevent yyy from being garbage collected while xxx remains alive. To use one of @atilim 's demos here is an example:

YYY = Core.class(Sprite)
 
local XXX=Sprite.new()
 
function YYY:init()
	self.proxy = newproxy(true)
	getmetatable(self.proxy).__gc = function() print("collected!") end
 
	XXX:addEventListener(Event.ENTER_FRAME, self.onEnterFrame, self)
end
 
function YYY:onEnterFrame()
	collectgarbage()
end
 
-- create an object
YYY:new()
 
stage:addEventListener("mouseDown", function() 
	XXX = nil
	collectgarbage()
end)

Notice how, even though neither XXX nor YYY are added to the stage, the garbage collector cannot collect YYY until XXX is released.

@atilim, is this what you intended for addEventListener(). My expectation was that it wouldn't keep objects alive. Could you not use a weak link to avoid this?

Anyhow, I hope this might save some other developers a few hours of hair loss if they find they have to track down similar leaks.

best regards

zvardin · March 2013

Thanks for this, definitely good to keep in mind. I agree it might make more sense to keep a weak link to a referenced function though.

atilim · March 2013

@bowerandy, as you've said, here xxx stores 2 references to both yyy.func and yyy. Therefore, if xxx is reachable then yyy.func and yyy are reachable too.

I think here the tricky part is yyy.func. Because if yyy.func accesses a local variable as an upvalue, then that upvalue is also reachable.

If you look at this code:

stage:addEventListener("mouseDown", function() 
	XXX = nil
	collectgarbage()
end)

the anonymous function accesses XXX as an upvalue. Therefore XXX can be reachable with this path: stage->function->XXX and therefore XXX is not collected. And XXX stores a reference to YYY so YYY is not collected also.

I think the current implementation (strong references) is correct. addEventListener idiom is commonly used in JavaScript and ActionScript and both implementations store a strong reference. But ActionScript gives an option for weak references also. And I can also give an option for weak references for experienced users.

zvardin · March 2013

Oops forgot about anonymous functions in that case.

bowerandy · March 2013

@atilim, would this be a global option or an additional parameter? I'm never too keen on global options - they just tend to make things more confusing. If you don't think it's the right things to do, then it might be best to leave it as it stands.

The important thing is for users to realize what's happening. It might be worth an explicit mention in the docs?

best regards

john26 · March 2013

Normally one uses addEventListener like this

planet:addEventListener(Event.MOUSE_DOWN, mouseDownFunc, planet)

which is safe since planet now contains a reference to itself. This does not prevent planet getting garbage collected. But the third argument could be anything so perhaps Gideros could output a warning if the third argument (if present) is anything other than the object itself?

If you had

planet:addEventListener(Event.MOUSE_DOWN, mouseDownFunc, star)

then planet contains a reference to star. However, if both planet and star are removed (and nilled out) at the end of the level then they can both be garbage collected as they are an island and Lua has no trouble with islands (I think).

The only danger is if planet is a persistent object and we want to remove star. The only option here is to use planet:removeEventListener. I think it is good practice to remove event listeners from any object that persists across levels at the end of each level/room.

atilim · March 2013

@bowerandy ActionScript's addEventListener takes a boolean parameter (useWeakReference) http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/events/EventDispatcher.html#addEventListener() with default value of false. We can choose a similar implementation.

And about docs, you're right.

Also if we store weak references by default (or globally), then this implementation:

Timer.delayedCall(1000, function() print("timer") end)
collectgarbage()

won't print anything because the anonymous function will be garbage collected immediately.

alexzheng · March 2013

is there any advantage to store weak references?
Using weak references, you have to take care of removeEventListener before the object to be collected ,otherwise will access freed memory adress, is that right?

bowerandy · March 2013

@atilim, it doesn't seem like other users are that concerned and, now I know what's going on, I won't be bitten again so why not leave things as they are.

best regards

john26 · March 2013

I would simply remember this rule:

"If an object persists across levels/rooms, its event listeners should be removed when the level/room is deleted, then regenerated at the beginning of the next level/room"

Howdy, Stranger!

Categories

In this Discussion

Top Posters

A warning about Memory Management complacency

Comments

Howdy, Stranger!

Quick Links

Categories

In this Discussion

Top Posters

A warning about Memory Management complacency

Comments