[WIP] Stop heartbeats when tearing down the Environment

[saniul]

Resolves https://github.com/Khan/Prototope/issues/29#issuecomment-76036074

Introduces the Heart object owned by the Environment, which holds references to all the Heartbeats in that environment.

[saniul]

This is almost done. Heartbeats are stopped when Heart is destroyed, and most of them get destroyed.

The ones that aren’t destroyed, are the ones created using the HeartbeatBridge. Something’s not right with memory management there, but I’m not sure what. I tried disabling this call in HeartbeatBridge.swift

JSContext.currentContext().virtualMachine.addManagedReference(self, withOwner: self)

but it didn’t help.

Any ideas?

[saniul]

One more thing – HeartbeatBridge.deinit never gets called.

[andymatuschak]

HeartbeatBridge never gets called even if you remove the addManagedReference call, and you don’t assign the new Heartbeat instance to a variable in global scope?

On Feb 25, 2015, at 7:56 PM, Saniul Ahmed notifications@github.com wrote:

One more thing – HeartbeatBridge.deinit never gets called.

— Reply to this email directly or view it on GitHub https://github.com/Khan/Prototope/pull/30#issuecomment-76118362.

[saniul]

Here https://github.com/saniul/Prototope/commit/adbeae6209ec6896e223ff5380d105630831b444:

• Commented out ... addManagedReference ...
• Changed the Color Puddles example to not set the created Heartbeat to a variable (This shouldn’t matter though, right? Since we’re destroying all our references to the Context it should clean up properly...)
• Added a deinit to HeartbeatBridge with a println call

Now, when you run the Color Puddles scene - HeartbeatBridge.deinit never gets called.

[andymatuschak]

Yikes. Uh. My next step would probably be to use Instruments to look at who's hanging onto it. About to run out for the day, but I can check that out tomorrow if you'd like!

[saniul]

:+1: I’ll take a look later today

[saniul]

So I tracked it down to the JSContext not being destroyed. Looks like just setting an empty function as a touchBeganHandler to any layer created using LayerBridge will create a retain cycle which prevents the JSContext from being destroyed.

That, in turn, prevents HeartbeatBridge from being destroyed.

[saniul]

Alright, so the cycle looks something like this:

LayerBridge ---> Layer ---> touchXXXHandler closure ---[captures]---> touchXXXHandler function ---> JSContext ---> LayerBridge ---> ∞

The JS function implicitly retains the JSContext because it needs it to be able to execute.

If we prevent the touchXXXHandler closure from capturing the JS function then it gets deallocated.

[saniul]

In https://github.com/saniul/Prototope/commit/4be751895bdab70fd45e7edb0d233cca63228258 I avoid capturing the JS function objects in closures. Instead I associate the JS function objects with the Environment, so they get destroyed whenever the Environment gets destroyed.

This is a temporary measure, I just wanted to demonstrate that this does indeed solve the retain cycle problem.

We could expose the current PrototopeJSBridge.Context and hold the references to those JS function objects there, somehow. Of course, it would be preferable not to create another global reference, but I couldn’t think of anything nice – ideas welcome!

[andymatuschak]

Nice. Gotcha. Thanks very much for the research!

I'm really disappointed that Swift makes it so easy to invisibly make these kinds of mistakes. At least it's slightly harder than in Obj-C. But still. Anyway:

When Context hits deinit, its JSContext really should be semantically dead. It seems undesirable to me that the JSContext continues to retain root references at that point. It looks like it might be possible to use JSValue.deleteProperty on all of JSContext.globalObject's properties (determined via e.g. JSContext.globalObject.toDictionary.allKeys()) at that point. That would break the cycle and, I think, be more semantically correct.

[saniul]

When Context hits deinit, its JSContext really should be semantically dead. It seems undesirable to me that the JSContext continues to retain root references at that point. It looks like it might be possible to use JSValue.deleteProperty on all of JSContext.globalObject's properties (determined via e.g. JSContext.globalObject.toDictionary.allKeys()) at that point. That would break the cycle and, I think, be more semantically correct.

Sounds good, will do.

Sent from my iPhone

On 27 Feb 2015, at 10:42, Andy Matuschak notifications@github.com wrote:

When Context hits deinit, its JSContext really should be semantically dead. It seems undesirable to me that the JSContext continues to retain root references at that point. It looks like it might be possible to use JSValue.deleteProperty on all of JSContext.globalObject's properties (determined via e.g. JSContext.globalObject.toDictionary.allKeys()) at that point. That would break the cycle and, I think, be more semantically correct.

[saniul]

Argh, looks like that isn’t possible after all. JSContext.globalObject.deleteProperty is failing (and returning false) for everything that was initialized in the main.js script. The rest of the objects (i.e. those added to the JSContext “manually”) are deleted successfully. Looks like objects created using evaluateScript are somehow protected.

[andymatuschak]

Yuck. Set the value for all those properties to null?

On Feb 27, 2015, at 4:59 PM, Saniul Ahmed notifications@github.com wrote:

Argh, looks like that isn’t possible after all. JSContext.globalObject.deleteProperty is failing (and returning false) for everything that was initialized in the main.js script. The rest of the objects (i.e. those added to the JSContext “manually”) are deleted successfully. Looks like objects created using evaluateScript are somehow protected.

— Reply to this email directly or view it on GitHub.

[saniul]

Tried already – didn’t help

[andymatuschak]

Okay, watched WWDC 2013 #615 again. They actually warn about exactly this kind of issue. The suggestion:

Instead of having our closure capture the JS function, we can have it capture a JSManagedValue which wraps that JS function. Then we use JSVirtualMachine.addManagedReference to keep that JS function alive only as long as its containing Context.

[andymatuschak]

Sorry, I realize that was kinda vague. JSManagedValue will make a weak reference whose lifetime is managed by the JS VM.

Something like:

let context: Context = iGuessWeNeedToBeAbleToWeaklyAccessTheOwningSwiftContext
let weakFunction = JSManagedValue(value: callable, owner: context) // maybe this won't work if Context isn't @objc?
layer.touchesBeganHandler = { [context = JSContext.currentContext()] sequenceMapping in
  return weakFunction.value?.callWithArguments([LayerBridge.bridgeTouchSequenceMapping(sequenceMapping, context: context)]).toBool()
}

[saniul]

So annoying – looks like there’s another retain cycle hiding somewhere.

Interestingly, it goes away if I don’t pass LayerBridge.bridgeTouchSequence(sequence, context: context) to the touchXXXHandler JS function. I’ll continue digging later...

[andymatuschak]

Yuck!

I hadn't realized it, but isn't that because the touchHandler closure is keeping the JSContext alive via strong capture (i.e. through its capture list)? You've got:

LayerBridge ---> Layer ---> touchXXXHandler closure ---[captures]--->
    JSManagedValue -[weak]-> touchXXXHandler function ---> JSContext ---> LayerBridge
    JSContext ---> LayerBridge

[saniul]

hadn't realized it, but isn't that because the touchHandler closure is keeping the JSContext alive via strong capture (i.e. through its capture list)?

Nope, the context I’m using is extracted from the JSManagedValue: if let context = managedCallable.value?.context

[andymatuschak]

Yikes. Okay. Will jump in when I get a chance; may not be for a few days.

On Mar 7, 2015, at 2:58 AM, Saniul Ahmed notifications@github.com wrote:

hadn't realized it, but isn't that because the touchHandler closure is keeping the JSContext alive via strong capture (i.e. through its capture list)?

Nope, the context I’m using is extracted from the JSManagedValue: if let context = managedCallable.value?.context

— Reply to this email directly or view it on GitHub https://github.com/Khan/Prototope/pull/30#issuecomment-77678531.

[andymatuschak]

Hm, @saniul, I just tried your branch (well, with HeartbeatBridge.swift's contents uncommented), and I am actually seeing the Heartbeats being destroyed correctly. Maybe that's a fix in 8.3b3? … yay?

One new issue I do observe, though, is that I have to assign the heartbeat to a global to make it stay around:

new Heartbeat({...}) // runs once, dies
var h = new Heartbeat({...}) // sticks around

[saniul]

Ok, I’m back, had an unplanned trip dropped on me this past week.

Looks like 8.3 introduces a bunch of JavaScriptCore stuff (including what looks like ECMAScript 6’s class support?).

I’m downloading 6.3 right now, will have a look at this PR and the new JSC things (there might be something useful for our bridge) later today.

[saniul]

Very odd, I’m still seeing the same behavior I reported earlier. Were you testing the Color Puddles demo or some other one?

I have to assign the heartbeat to a global to make it stay around: I’m not seeing this at all in the stripped down implementation of Color Puddles/main.js that I just pushed up. You can see a string being logged in the console by the Heartbeat that’s not stored in a var.

Here’s how you can see things aren’t cleaned up properly:

I’m logging the #of Layers and LayerBridges that are kept around in memory (simple counter, increments on init, decrements on deinit). If you just keep resaving Color Puddles/main.js without interacting with the scene – the count is balanced. If you even touch the screen once – the count grows by 2 (meaning that two Layers and LayerBridges weren’t destroyed).

[andymatuschak]

Thanks for that! I'm observing that the heartbeat stops executing (i.e. the logging stops), but that the layers stick around, as you mention. If I comment out the lines in the heartbeat closure which refers to touchLayers, then the layers die as expected.

[andymatuschak]

Argh, okay, I still haven't gotten to the bottom of this, and I need to turn my attention to other things for now, but I discovered something interesting: if you comment out the touchEndedHandler (and leave the rest), the memory is all cleaned up as expected. Something's special about that case…