ObjCBlock.fromFunction should return same block for same function

[liamappelbe]

Calling ObjCBlock.fromFunction twice with the same function should return the identical ObjCBlock. This would help a bit with dart-lang/native#204, by creating fewer persistent handles.

It should be pretty easy to, using an Expando to attach the ObjCBlock to the Dart function. Then, inside ObjCBlock.fromFunction, we just check if the user's function has an ObjCBlock attached already, and return it if it's there.

[mkustermann]

It should be pretty easy to, using an Expando to attach the ObjCBlock to the Dart function. Then, inside ObjCBlock.fromFunction, we just check if the user's function has an ObjCBlock attached already, and return it if it's there.

Doesn't that imply we have to keep the ObjC block artificially alive (by explicitly increasing refcount on it)? How would we know when to deref it again (to avoid leaking the block)?

Maybe this kind of optimization is more suitable for users, who may know if a callback will be needed many times, then the user code can re-use the ObjC block it already has instead of calling ObjCBlock.fromFunction?

[liamappelbe]

We shouldn't be keeping it around any longer than before. If the first instance gets GC'd, then passing the same function again should return a new instance. The ref counting is pretty straightforward, and I have a unit test for the GC behavior. I'll include you in the PR.

[dcharkes]

We shouldn't be keeping it around any longer than before. If the first instance gets GC'd, then passing the same function again should return a new instance. The ref counting is pretty straightforward, and I have a unit test for the GC behavior. I'll include you in the PR.

What about if we stick the closure in a toplevel variable?

(snippet taken from testcase in PR)

final func = makeAdder(4000);

void foo(){
  final block1 = ObjCBlock.fromFunction(lib, func);
  final block2 = ObjCBlock.fromFunction(lib, func);
}

Would we have GCed both blocks prior to https://github.com/dart-lang/ffigen/pull/477 ? Now we would hold on to the block forever, correct?

[mkustermann]

The issue with using Expando (amongst other things) is that the key may outlive it's value, thereby making us leak the values. The prime example is using a top-level tearoff of a function (such closures never die).

[liamappelbe]

Yeah, good point. I suppose to do this properly I should use an explicit map from function to ObjCBlock and clean it up when the ObjCBlock is GC'd, but that has the same issue as dart-lang/native#204.

[liamappelbe]

I think this is not worth the effort, as it would provide minimal benefit with non-zero overhead.