Memory management for blocks

[liamappelbe]

Right now we leak all the block pointers.

[liamappelbe]

Blocks have a dispose_helper function that we can use to detect that the block is destroyed. Function pointer blocks just need to free their Block struct. Closure blocks need to additionally deregister their closure.

[liamappelbe]

@dcharkes

I'm not really sure how to handle the closure case. Dart closures get registered in a static map from int ID to function, and then we put that ID in an ObjC block. I can attach a native finalizer to clean up the block, but I can't make it delete the function from the map, because doing so requires running Dart code.

Another option I've considered is replacing the global function map with an Expando or something, so that the function reference is only weakly held. Then the question is what the key of the Expando is. It can't be the block pointer, because that's explicitly disallowed. I could make it some sort of Dart object (maybe the ObjCBlock wrapper object), but then the issue becomes what do we stick in the native block, since (afaik) we can't put a reference to a Dart object in a native struct field.

Any ideas?

[dcharkes]

It can't be the block pointer, because that's explicitly disallowed

It would also not be useful. You could lose track of the pointer in Dart, but still hold on to the block in ObjectiveC, in which case the ObjectiveC could try to call the closure after the Dart GC has collected it.

How about passing the closure as Object in Dart and getting it as Dart_Handle in ObjectiveC? And then storing it as a Dart_PersistentHandle in the ObjectiveC object. On callbacks you then turn it back into a Dart_Handle and Object (either as FFI callback or in the message sent through the native port). And finally when running the native finalizer you also delete the persistent handle with Dart_DeletePersistentHandle.

[liamappelbe]

How about passing the closure as Object in Dart and getting it as Dart_Handle in ObjectiveC?

So far I haven't had to write/generate any ObjectiveC or C code. Is there a way of getting the handles and persistent handle in Dart? Can I call those functions using FFI?

[dcharkes]

So far I haven't had to write/generate any ObjectiveC or C code.

Nice.

Is there a way of getting the handles and persistent handle in Dart?

You could try looking up Dart_NewPersistentHandle with <Pointer<Void> Function(Object), Pointer<Void> Function(Dart_Handle)>. You're talking about the code that ultimately runs in Flutter apps as well right? In that case it should be Dart_NewPersistentHandle_DL from dart_api_dl.h, because Flutter doesn't re-expose dart_api.h. (@brianquinlan has already been using dart_api_dl.h if you want some help on that.)

[liamappelbe]

There's a lot of moving parts here, so I'm writing down my plan. There are 3 kinds of block, and they have different memory management requirements: blocks that come from ObjC code, blocks that are constructed in Dart from C function pointers, and blocks constructed in Dart from Dart functions.

There are 3 stages to this, in increasing order of difficulty, and decreasing order of importance (so maybe we can skip the last step):

• Need to add retain/release stuff like _ObjCWrapper to ObjCBlock
  • ObjCBlock needs to implement Finalizable
  • When the Dart ObjCBlock object is garbage collected, its finalizer will call release on the block pointer (this works exactly the same for all 3 kinds of block)
  • TODO: Where is the ref count stored in the block struct? Do we need to retain blocks that are constructed in Dart?
• Memory management for raw function pointer blocks:
  • Need to set dispose_helper to C's free function, and set the BLOCK_HAS_COPY_DISPOSE bit of the flags field
  • TODO: What about copy_helper? Can we get away with leaving it null? What exactly are its semantics? Maybe we can just set it to C's memcpy function?
• Memory management for Dart function blocks:
  • Delete the closureRegistry map
  • Instead we're going to have an object called _ObjCBlockFunctionLink that holds a reference to the Dart function being wrapped
  • _ObjCBlockFunctionLink is owned by the block struct, using Dart_NewPersistentHandle_DL, in the field we currently put the int function ID
  • The block struct's dispose function calls Dart_DeletePersistentHandle (we'll need a separate Block_descriptor for Dart function blocks, rather than reusing the function pointer block descriptor)
  • _ObjCBlockFunctionLink also implements Finalizable, and its finalizer is what deletes the block struct's memory
    • We could do this at the same time we call Dart_DeletePersistentHandle, but then we'd have to write some C code
  • TODO: Technically the dispose function can be called from any thread, which is a problem for Dart_DeletePersistentHandle
  • TODO: What about copy_helper? Unlike above, we can't just set it to memcpy.

[dcharkes]

• Memory management for Dart function blocks:
  • _ObjCBlockFunctionLink also implements Finalizable, and its finalizer is what deletes the block struct's memory

Let me see if I grasped this correctly.

• ObjCBlock (Dart) points to "block struct" (native)
  • A native finalizer decreases the ref-count on "block struct" when ObjCBlock is GCed.
  • If native code still holds on to the "block struct" somewhere else, it will keep the "block struct", _ObjCBlockFunctionLink and closure alive.
• "block struct" (native) points to _ObjCBlockFunctionLink (Dart) by means of a Dart_PersistentHandle
  • The dispose function of "block struct" will delete the persistent handle (effectively reducing the ref count of _ObjCBlockFunctionLink in Dart land.
• _ObjCBlockFunctionLink (Dart) points to the closure (Dart)
  • When the _ObjCBlockFunctionLink (Dart) gets GCed, the closure can also get GCed.
  • A native finalizer deletes the "block struct" memory.

I think we might be able to simplify the design a bit:

• The dispose function of "block struct" can delete itself after deleting the persistent handle
• The "block struct" stores the closure in the persistent handle rather than a wrapper object _ObjCBlockFunctionLink containing the closure.
• We have no _ObjCBlockFunctionLink wrapper object.

Or does the _ObjCBlockFunctionLink serve another purpose that I am missing here?

copy_helper

Do we need to support copying of these blocks? Or can we disallow that?

If we need to support it, we need to allocate a new persistent handle to the closure on copying. That way the closure will only get GCed when all blocks referring to it from native are deleted.

Technically the dispose function can be called from any thread, which is a problem for Dart_DeletePersistentHandle.

Correct, it needs to be called from a thread where at least an isolate group is. (It doesn't have to be a mutator thread, so delete persistent handle can be called from native finalizers.) A workaround for this would be to send the handle through a native port (to an isolate in the isolate group, but probably easiest to do the same isolate) to Dart and let Dart call Dart_NewPersistentHandle_DL through dart:ffi.

You probably need a similar trick for the callbacks themselves, if they can be called from any thread. But for those you would also need to setup a port the other way, so you can communicate the return value back.

[liamappelbe]

• The dispose function of "block struct" can delete itself after deleting the persistent handle
• The "block struct" stores the closure in the persistent handle rather than a wrapper object _ObjCBlockFunctionLink containing the closure.
• We have no _ObjCBlockFunctionLink wrapper object.

Oh yeah, that should work. Thanks.

Do we need to support copying of these blocks? Or can we disallow that?

Not sure, I need to test it. I don't know what will happen if we leave that function as null.

You probably need a similar trick for the callbacks themselves, if they can be called from any thread. But for those you would also need to setup a port the other way, so you can communicate the return value back.

Yeah, since this is already a problem for callbacks, I was planning to ignore it for now. Just flagging it as a potential issue.

[dcharkes]

Yeah, since this is already a problem for callbacks, I was planning to ignore it for now. Just flagging it as a potential issue.

FWIW: We do the same for C bindings. It is the users' responsibility to use native ports instead of passing in a Pointer<...> from a fromFunction call.

[liamappelbe]

TODO: Where is the ref count stored in the block struct? Do we need to retain blocks that are constructed in Dart?

Do we need to support copying of these blocks? Or can we disallow that?

Ran some tests this morning. For reference, the copy and dispose function fields on the Block look like this:

void (*copy_helper)(void *dst, void *src);
void (*dispose_helper)(void *src);

Findings:

• As well as setting the flag to BLOCK_HAS_COPY_DISPOSE, we also have to change the isa field to identify the block as a stack block. Global blocks seem to ignore all retain/release/copy/dispose calls, which makes sense I suppose.
• If you set the flags and the dispose function, but leave the copy function as null, it won't crash. It will still call dispose, it'll just ignore copy.
• No matter how many times I release a block constructed in Dart, I can't get it to be deleted. I guess this means Dart blocks aren't ref counted, or there's a secret ref count that is set to a sentinel value telling the runtime to ignore it.
• But a copy of that block behaves as we'd expect from normal ref counting (starts with 1 ref, and is deleted when the ref count reaches 0).
• The copy function is passed a fresh block of memory to copy into. This must mean that the runtime automatically allocates the new block of memory.
  • Unsurprisingly, the src and dst pointers match the block pointers you see in ObjC land.
  • The dst block is automatically filled with whatever was in src (with some differences, see below). The copy function doesn't need to do anything by default.
  • All of that is true regardless of whether you set the copy function, or leave it null.
  • Presumably this all means that the runtime also handles deleting the copy's memory (it's hard to to confirm that the memory isn't leaking, but I do get a crash if I continue to try to release it)

As for why the original is not ref counted, but the copy is, I looked at the memory layout of the blocks, and these were the differences:

• The isa field is different
  • The docs say that it should either be _NSConcreteStackBlock or _NSConcreteGlobalBlock.
  • The original is using stack, but the copy is using a different value that isn't mentioned in the docs.
  • These are pointers to internal blocks of memory. The copy's pointer is close to the stack and block ones, so it's probably _NSConcreteMallocBlock, but that symbol is not exported, so it'll be tricky for us to use it.
• The flags field has added (1 << 24), which is an undocumented flag called BLOCK_NEEDS_FREE
  • Enabling this flag on the original doesn't enable ref counting, and causes the copy to stop being ref counted (?!)
• When I retain/release the copy, nothing changes except the flags field
  • It seems to be storing the ref count in the low bits
  • 0x1 is skipped, so the ref count changes by 0x2 at a time.
  • The first documented flag value is (1 << 23), so we potentially have 21 bits to store the ref count (probably only 16-1 tho)
  • I thought the runtime might not be ref counting our original block because its ref count in the flags was already 0, so I tried starting it at 2, but this didn't do anything.

No matter what combination of those factors I try, I can't get the runtime to ref count the Dart constructed blocks. I also tried dumping the bytes before the copy, and setting the same bytes before the Dart constructed object, in case there were some more undocumented flags there, but nothing worked. This is a problem because if release doesn't do anything then we can't free the block's memory.

Something else that's interesting is that copying the copy gives you back the same pointer (and increments the copy's ref count). So the best way to do this might be to construct the fake Dart Block, then make a copy of it (by sending a "copy" message or using Block_copy), delete the original, and just return the copy. The nice thing about this approach is that subsequent copies don't actually copy anything, so we don't need to set the copy function. We should also probably use Block_release to release the reference, rather than objc_release.

[liamappelbe]

New simpler design, based on my investigation, and Daco's suggestions:

• Add retain/release logic like _ObjCWrapper to ObjCBlock, but call Block_copy and Block_release
• Whenever a block is constructed in Dart, turn it into a proper ObjC block by calling Block_copy, and deleting the original
  • After this first copy, the block is never actually copied again, and subsequent Block_copy calls just increment the ref count, so we can leave the Block's copy function as null
• For raw function pointer blocks, don't need to do anything else
  • The copy's memory will automatically be deleted when its ref count hits 0, so we don't need to set the dispose function, or the BLOCK_HAS_COPY_DISPOSE flag
• For Dart function blocks, we need to:
  • Delete the closure registry
  • Stick the Dart function directly in the block target, using Dart_NewPersistentHandle/Dart_NewPersistentHandle_DL
  • Set the BLOCK_HAS_COPY_DISPOSE flag
  • Create a separate block descriptor struct for Dart function blocks
  • Set the dispose function to Dart_DeletePersistentHandle/Dart_DeletePersistentHandle_DL

[liamappelbe]

There's an implementation detail that the dispose function can't just be Dart_DeletePersistentHandle, because dispose is passed the Block struct pointer, not the Function handle. The handle is stored at the end of the Block struct, so we need a small wrapper function that extracts the handle and calls Dart_DeletePersistentHandle. That function can't be implemented in Dart, so we're back to having to generate C boilerplate:

struct Block {
    void *isa;
    int flags;
    int reserved;
    void* invoke;
    void* descriptor;
    void* target;
};

void disposeDartBlock(void* ptr) {
  Dart_DeletePersistentHandle(((Block*)ptr)->target);
}

This tiny amount of C would significantly complicate the workflow for developers (and complicate our APIs), so I'm really hesitant to go down this route. If we do chose to do this, it should at least be behind a config flag, because most use cases will have a small/bounded number of closures, so won't really care if they're never cleaned up.

For now, unless we come up with another approach, I'm just going to do the other parts, and leak these Dart functions.