Single allocation of encode_async block with non-ARC capture in ggml-metal.m

[ptsochantaris]

This PR hopefully addresses the issue raised in a comment in ggml-metal.m:

// TODO: how to avoid this allocation? I tried initializing it in ggml_backend_metal_set_n_cb but it crashes.

The reason that fails is because ObjC blocks in manual memory management mode are not retained upon creation, they need to be explicitly retained, or rather, copied after creation. Usually this is done by using a copyable property declaration on the owning object, but in this case the block is assigned to a plain C structure. In such cases we can reproduce the same behaviour by using the Block_copy function. Having done that, we also need to release the block once we are done with it, preferably just before setting that property to nil.

This PR makes this change, and seems to fix the crash mentioned, but please do give it a go to verify before merging, just in case :D I hope this helps!

• I have read the contributing guidelines
• Self-reported review complexity:
  • Low
  • Medium
  • High

[ggerganov]

Great - thank you for the explanation!

Would Block_release make more sense to be in ggml_metal_free() instead?

diff --git a/ggml/src/ggml-metal.m b/ggml/src/ggml-metal.m
index 40835074..83bf658c 100644
--- a/ggml/src/ggml-metal.m
+++ b/ggml/src/ggml-metal.m
@@ -438,7 +438,6 @@ static struct ggml_backend_metal_context * ggml_metal_init(void) {
     ctx->capture_scope = nil;
 
     ctx->gf = nil;
-    Block_release(ctx->encode_async);
     ctx->encode_async = nil;
     for (int i = 0; i < GGML_METAL_MAX_COMMAND_BUFFERS; ++i) {
         ctx->command_buffers[i] = nil;
@@ -684,6 +683,8 @@ static void ggml_metal_free(struct ggml_backend_metal_context * ctx) {
         [ctx->kernels[i].pipeline release];
     }
 
+    Block_release(ctx->encode_async);
+
     [ctx->queue release];
     [ctx->device release];

In ggml_metal_init() we only initialize the pointer with nil and are never supposed to pass through that function a second time.

[ptsochantaris]

Oh indeed, I had thought that was the place where things were being de-init because of the nil assignment. It definitely belongs in the deallocation place instead. Will update in a sec.

[ggerganov]

In theory, we might want to call ggml_backend_metal_set_n_cb() multiple times in the future (although unlikely). So to make this more correct in this case, we should check if ctx->encode_async == nil here and if not, call Block_release() first

[ptsochantaris]

Ah, that had crossed my mind indeed, but didn't want to add more logic there than needed for now - worried about scope creep. I agree this makes total sense. Will add 👍