Fixing memory leaks when using metal-cpp without using @autoreleasepo…

[o-micron]

Problem

I caught memory leaks when using backends/imgui_impl_metal.mm mixed with backends/imgui_impl_sdl.cpp I'm using the metal-cpp interface headers I found out that I have to manually call autoRelease() on every allocated object (that we're supposed to own of course ...). According to Cocoa's conventions, you own every object that you create. Methods starting with alloc, new, copy, mutableCopy or create give us ownership of the object to release.

Reproduce the issue

• Compile any sample app with imgui using metal-cpp interface (and usual SDL2 or whatever alternative ...) [I will try to create a sample app for you to reproduce with, hello world example is more than enough to reproduce it]
• Run using leaks -atExit -- ./App to check for leaks

Suggested solution

1. Define IMGUI_METAL_CPP_AUTO_RELEASE_POOL to limit the changes to be applied only when using metal-cpp interface
2. Call [<xptr> autoRelease]; on the objects that were allocated and that we have ownership of

ImGui details

imgui version: v1.88 imgui backend: Metal | SDL

[rokups]

Do we really need IMGUI_METAL_CPP_AUTO_RELEASE_POOL? Are there downsides if autoRelease() simply gets called explicitly always?

[o-micron]

Do we really need IMGUI_METAL_CPP_AUTO_RELEASE_POOL? Are there downsides if autoRelease() simply gets called explicitly always?

So far, I haven't been exposed to the case where one wouldn't want to call autoRelease to avoid such leaks. I wanted to be on the safe side until I am sure that this will always be the case for everyone using the metal backend.

[ocornut]

Pinging @warrenm maybe they would have an opinion? Would be nice to remove the #ifdef blocks if we can.

[warrenm]

The Metal backend was designed under the assumption that Objective-C automatic reference counting (ARC) would always be enabled, as indeed it should be in modern projects. If we're going to insert autorelease calls, we should go all the way and make the backend compilable without ARC, with calls to autorelease and release predicated on __has_feature(objc_arc) rather than some arbitrary external preprocessor symbol.

The better alternative is to compile your target with CLANG_ENABLE_OBJC_ARC on.

[o-micron]

Thanks @warrenm, I am using the new metal-cpp headers, that is mainly what triggered the issue, I don't have a autorelease pool scope in my main function ...

I assume metal-cpp headers are probably going to be used from a non-objc code or so that is what these are intended to be used ? So I wanted to assume the user doesn't want to wrap the code around autorelease pool scope ... only C/C++ code ..

Checking for __has_feature(objc_arc) should be enough. Compiling the code with CLANG_ENABLE_OBJC_ARC might not be very obvious and it might be easily missed

@ocornut @warrenm does this sound Okay ?

[warrenm]

Fine by me, but I would note that in many instances in this PR, it suffices to call release at the end of scope instead of using autorelease. Putting objects in an autorelease pool when the duration of ownership is known is a code smell, in my opinion.

[warrenm]

I'm still pretty queasy about this.

Enabling or disabling ARC is orthogonal to having an autorelease pool in place.

Cocoa frameworks (and the imgui Metal backend) will create autoreleased objects you don't have access to, so there must be an autorelease pool that gets appropriately serviced by a runloop on any thread that autoreleases an object. Simply inserting autorelease calls when ARC is disabled doesn't actually achieve this.

The correct solution is probably to use metal-cpp's NS::AutoreleasePool class in scopes where objects are autoreleased. Not using an autorelease pool when objects are being autoreleased is a programmer error, as much as we might wish to paper over it.

Orthogonal to that, if you wish to completely disable ARC—a long-deprecated practice that is likely to cause more problems than it solves—you must ensure that all code is compatible with ARC being disabled. Adding autorelease calls is not sufficient; you must manage all object lifetimes.

I have written a patch that (I think) works with ARC disabled. You can use it as a template for your changes if you want to solve this properly, but I sincerely believe that if you follow best practices instead, this entire issue will solve itself.

[o-micron]

Cool, thanks @warrenm, I have used your patch as a template as you advised, it's a good starting point. I added a couple of lines in ImGui_ImplMetal_Shutdown to make sure we release the MetalContext as well, link to the lines

I have created a new metal-imgui-example repository with a sample code using the metal-cpp headers with the changes added in the imgui metal backend. It should serve us well as a testing playground. It contains instructions regarding running and checking for leaks as well to make sure we're good.

[UPDATE] I think I'm mixing up between ARC and MRR ... I will update the example project later this week ...

[o-micron]

Ok, I have created a metal-cpp backend which is intended to be only used along with the metal-cpp interface. The implementation is based on the objc metal backend still.

The main differences:

• Removed the Grand Central Dispatch calls (dispatch_async etc ...)
• Replaced the raw pointers with NS::SharedPtr, closer in behavior to std::shared_ptr .. eliminated the need to call retain()/release()/autorelease()

I am not sure whether I should modify the objc backend now, I would love to hear from you on that. I think the imgui_impl_metal_cpp should close the gap for when someone wants to use the metal-cpp interface.

This metal-imgui-example repository uses this backend now, I have checked for memory leaks and I think we're good. There's 1 single leak CFString but that's coming from SDL when initializing video .. not to be concerned about it here ...

Would love to have your thoughts and reviews on that @warrenm @ocornut, thanks !

[Update] I think that extra 48 bytes of CFString leak is appearing even in Apple's own example projects .. I think the string allocation was coming from CoreFoundation .. It's not SDL problem ... let's skip this one leak for now ..

1 (48 bytes) ROOT LEAK: <CFString 0x6000018a8000> [48]  length: 26  "Copyright Apple Inc., 2019"

[ocornut]

Hello, I don't understand all the discussions here, however:

Ok, I have created a metal-cpp backend which is intended to be only used along with the metal-cpp interface.

I don't think that duplication is desirable, couldn't this be using the same source file?

[o-micron]

Hey, Yes it could be using the same source file, the fixes were made in the objc backend first and additionally, I added a C++ metal backend as an extra, we can omit that if the duplication is not desirable. I just wanted to switch from objc backend to a more c++ backend since we now have metal-cpp interface supported and maintained by Apple. It's easier to work with C++ across than to have objc only for apple platforms ..

[o-micron]

I'm closing the PR. In case anyone in the future needs a metal_cpp instead of the objc backend they can find it there https://github.com/o-micron/metal-imgui-example/tree/main/imgui