Skip to content

metal : fix memory leaks due to missing autoreleasepools - #27758

Merged
ggerganov merged 1 commit into
masterfrom
nikwen/metal-autoreleasepool
Aug 27, 2026
Merged

metal : fix memory leaks due to missing autoreleasepools#27758
ggerganov merged 1 commit into
masterfrom
nikwen/metal-autoreleasepool

Conversation

@nikwen

@nikwen nikwen commented Aug 26, 2026

Copy link
Copy Markdown
Member

Overview

Running llama-cli with OBJC_DEBUG_MISSING_POOLS=YES on Mac shows various memory leaks in the Metal init path that look like this:

objc[15978]: MISSING POOLS: (0x16c257000) Object 0x9facfc000 of class {class name} autoreleased with no pool in place - just leaking - break on objc_autoreleaseNoPool() to debug

This PR adds three @autoreleasepools to fix these leaks. Apart from that, it's only whitespace changes. (Run git show --ignore-all-space locally to view the diff without whitespace changes.)

Additional information

There are still some lines left related to AGXG16SDevice. I'll check if we can fix those or if those come from inside Metal. I'd love to get this merged in the meantime though.

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: YES, used Opus 5 to help me find where to put the @autoreleasepools, verified it manually though and in the process deleted 80% of Opus' code.

@nikwen
nikwen requested a review from a team as a code owner August 26, 2026 18:32
@nikwen

nikwen commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

/bot review

@github-actions github-actions Bot added ggml changes relating to the ggml tensor library for machine learning Apple Metal https://en.wikipedia.org/wiki/Metal_(API) labels Aug 26, 2026
@ggml-gh-bot

ggml-gh-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown
Automated code review

I reviewed the diff (verified via git show --ignore-all-space that it is exactly three @autoreleasepool additions plus re-indentation) and traced the ownership of every object stored into the dev/res structs to confirm the pools are safe.

Verdict: looks correct, no blocking findings

I confirmed that all objects assigned to struct members inside the new autoreleasepools are +1 retained (Create/new/alloc/init/newSharedEvent rules) and are explicitly released in the corresponding free functions, so they survive the pool drain and no dangling pointers are introduced:

  • ggml-metal-device.m: MTLCreateSystemDefaultDevice() (+1 Create), [device newCommandQueue] (+1 new), ggml_metal_library_init() (calloc + retained NSLock/pipelines/libs), ggml_metal_rsets_init() (calloc + retained NSLock/NSMutableArray/dispatch_group). ggml_metal_device_free releases mtl_queue/mtl_device and frees library/rsets.
  • ggml-metal-context.m: res is calloc'd; ev_cpy from ggml_metal_device_event_init holds a +1 newSharedEvent (released in event_free); cmd_bufs_ext is alloc init; pipelines_ext is new'd; d_queue from dispatch_queue_create (+1). The residency background thread's captured rsets pointer is safe because ggml_metal_rsets_free does dispatch_group_wait before freeing.

The pattern (wrapping a whole one-shot init body in @autoreleasepool) matches the existing convention already used ~15 times in these two files (e.g. ggml_metal_library_build_index, ggml_metal_event_record). The event_record/event_wait helpers even demonstrate the codebase's own rule: [queue commandBuffer] is autoreleased and gets an explicit [cmd_buf retain] before the pool drains - consistent with how the init paths here rely on +1 ownership.

The ggml_metal_encoder_debug_group_push pool is genuinely necessary, not redundant: it is reached via ggml_metal_op_encode inside the encode_async block, which runs on a GCD queue with no enclosing autoreleasepool, so the autoreleased NSString stringWithCString: would otherwise leak (exactly what OBJC_DEBUG_MISSING_POOLS reports). pushDebugGroup: retains/copies the name internally, so draining the string immediately is safe.

Notes for the contributor

(point 1) The bulk of the diff is pure re-indentation. Reviewers can use git show --ignore-all-space to see only the three real lines; the PR description already points this out, which is appreciated.

(point 2) Nit (pre-existing, not introduced here): in ggml_metal_init, the queue == nil early-return path does return NULL; without free(res) after res = calloc(...), leaking the struct. It sits inside the new autoreleasepool but the pool cannot reclaim a calloc'd pointer, so the leak remains. Since this PR is about leaks in this exact function, a one-line free(res); before that return NULL; would be a natural inclusion - but it is out of the PR's stated scope, so use your judgement.

(point 3) Nit: the two const char * src_tensor_* source strings inside the ggml_metal_device_init pool are large stack literals that now live one indentation level deeper; no behavior change, just noting the re-indent is what inflates the device.m hunk.

(point 4) AI disclosure in the PR template is present and appropriate; the commit message is clean (no Co-authored-by:). No action needed.

No changes to public API, no new ops/quant types, no security-relevant input parsing touched. The change is Metal-internal, minimal, and consistent with the codebase. This should be a straightforward review.

This review was generated automatically by pi coding agent using zai-org/GLM-5.2. It may contain mistakes. Maintainers make the final call.

@nikwen

nikwen commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

Nit (pre-existing, not introduced here): in ggml_metal_init, the queue == nil early-return path does return NULL; without free(res) after res = calloc(...), leaking the struct.

This is a good finding by the bot. Putting that on my list of things to fix in future PRs.

[encoder->obj pushDebugGroup:[NSString stringWithCString:name encoding:NSUTF8StringEncoding]];
@autoreleasepool {
[encoder->obj pushDebugGroup:[NSString stringWithCString:name encoding:NSUTF8StringEncoding]];
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case what is causing the leak - the NSString?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, here it's the NSString.

@nikwen

nikwen commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

The test failures are unrelated. (The changes only affect Apple builds.)

@nikwen nikwen added the merge ready A maintainer can use this label to indicate that they consider the changes final and ready to merge. label Aug 26, 2026
@ggerganov

Copy link
Copy Markdown
Member

Btw, we have this job in the CI that checks for leaks on macos:

-DCMAKE_OSX_DEPLOYMENT_TARGET=13.3
time cmake --build build --config Release -j $(sysctl -n hw.logicalcpu)
leaks -atExit -- ./build/bin/test-thread-safety -hf ggml-org/gemma-3-270m-qat-GGUF -ngl 99 -p "$(printf 'hello %.0s' {1..128})" -n 16 -c 512 -ub 32 -np 2 -t 2 -lv 1
- name: Test

Not sure why it didn't catch these - any ideas? Can we modify it somehow to be sensitive to those leaks?

@nikwen

nikwen commented Aug 27, 2026

Copy link
Copy Markdown
Member Author

@ggerganov Thanks for the pointer to the test!

leaks has an --autoreleasePools option that can help us detect this. Alternatively, we could run with OBJC_DEBUG_MISSING_POOLS=YES and grep the output for autoreleased with no pool in place - just leaking.

That said, we can't update the test yet. There's still one AGXG16SDevice object that's being leaked that's not yet fixed by this PR (as mentioned in the PR description). In a few days, I'll have time to look into that. Then I'll update the test, too. Would be great to already get the improvements from this PR merged before then though. :)

@ggerganov
ggerganov merged commit 915dc6d into master Aug 27, 2026
25 of 28 checks passed
@ggerganov
ggerganov deleted the nikwen/metal-autoreleasepool branch August 27, 2026 09:53
thecodacus pushed a commit to thecodacus/llama.cpp that referenced this pull request Sep 7, 2026
Githab-capibara added a commit to Githab-capibara/llama.cpp that referenced this pull request Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Apple Metal https://en.wikipedia.org/wiki/Metal_(API) ggml changes relating to the ggml tensor library for machine learning merge ready A maintainer can use this label to indicate that they consider the changes final and ready to merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants