macOS: fix memory leak in Metal driver#91
Conversation
rkitover
force-pushed
the
macos-metal-memleak-fix
branch
2 times, most recently
from
3311d91 to
e109105
Compare
| assert(commandLists != nullptr); | ||
| assert(commandListCount > 0); | ||
|
|
||
| NS::AutoreleasePool *releasePool = NS::AutoreleasePool::alloc()->init(); |
There was a problem hiding this comment.
This seems like it could be a likely culprit of the current memory leaks. Do you know which part of the enclosed code is responsible for needing the pool?
There was a problem hiding this comment.
None of these are apparently the major memory leak I am seeing in BanjoRecomp, this fix just releases the MTLSTR label string which is an autorelease string and accumulates in that code path.
There was a problem hiding this comment.
I see, so merging this and the other related PRs won't fix the major leak yet?
There was a problem hiding this comment.
A friend asked me to look at the leak in BanjoRecomp. I don't think there is one in plume, these were the first ones I identified but they are minor correctness issues. It looks like I need to look into rt64 in more depth for the major memory leaks.
There was a problem hiding this comment.
Can you elaborate on the MTLSTR bit for my understanding? I was under the impression from looking at metal-cpp that MTLSTR is basically the same as CFSTR, which states:
The returned object is a constant. You may retain and release it, similar to other immutable CFString objects, but are not required to do so—it will remain valid until the program terminates.
There was a problem hiding this comment.
My understanding here was wrong, you are correct that MTLSTR() objects do not need to be autoreleased.
The autorelease pool is actually for the commandBufferWithUnretainedReferences which needs to be autoreleased.
There was a problem hiding this comment.
Seeing what we have I see two different uses of commandBufferWithUnretainedReferences.
MetalCommandList::begin() and MetalCommandList::end() split this responsibility by not using the auto release pool and just calling release on the end method.
The other uses seem to wrap it in the auto-release pool, but a static code review alone doesn't really tell me whether that actually releases the command buffer or not. I'm not finding much documentation on that regard.
Then remains this particular use, where the command buffer is requested but no auto release pool is used, which is what the PR fixes.
It'd be good to get a solid confirmation on whether the auto-release pool actually works or not, because as far as I see, doing what MetalCommandList does could also be a fix that is much easier to understand and review than the auto-release behavior.
Fix memory leaks in the Metal driver on macOS. Add destructor for MetalSwapChain, release devices array and add autorelease pool for MTLSTR objects. Release state and error objects. Signed-off-by: Rafael Kitover <rkitover@gmail.com>
rkitover
force-pushed
the
macos-metal-memleak-fix
branch
from
e109105 to
564e2f7
Compare
3 participants
Fix memory leaks in the Metal driver on macOS.
Add destructor for MetalSwapChain, release devices array and add autorelease pool for MTLSTR objects.