Skip to content

macOS: fix memory leak in Metal driver #91

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rkitover wants to merge 1 commit into
base: main
Choose a base branch
from
Open

macOS: fix memory leak in Metal driver #91

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion plume_metal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1327,6 +1327,7 @@ namespace plume {

if (error != nullptr) {
fprintf(stderr, "MTLDevice newLibraryWithSource: failed with error %s.\n", error->localizedDescription()->utf8String());
error->release();
return;
}
}
Expand Down Expand Up @@ -1361,6 +1362,7 @@ namespace plume {

if (error != nullptr) {
fprintf(stderr, "MTLLibrary newFunction: failed with error: %s.\n", error->localizedDescription()->utf8String());
error->release();
return nullptr;
}

Expand Down Expand Up @@ -1434,6 +1436,7 @@ namespace plume {

if (error != nullptr) {
fprintf(stderr, "MTLDevice newComputePipelineStateWithDescriptor: failed with error %s.\n", error->localizedDescription()->utf8String());
error->release();
return;
}

Expand Down Expand Up @@ -1587,6 +1590,7 @@ namespace plume {

if (error != nullptr) {
fprintf(stderr, "MTLDevice newRenderPipelineState: failed with error %s.\n", error->localizedDescription()->utf8String());
error->release();
return;
}

Expand Down Expand Up @@ -1930,6 +1934,13 @@ namespace plume {
}

MetalSwapChain::~MetalSwapChain() {
// Release all retained drawables
for (auto& drawable : drawables) {
if (drawable.mtl) {
drawable.mtl->release();
drawable.mtl = nullptr;
}
}
}

bool MetalSwapChain::present(const uint32_t textureIndex, RenderCommandSemaphore **waitSemaphores, const uint32_t waitSemaphoreCount) {
Expand Down Expand Up @@ -3685,6 +3696,8 @@ namespace plume {
assert(commandLists != nullptr);
assert(commandListCount > 0);

NS::AutoreleasePool *releasePool = NS::AutoreleasePool::alloc()->init();
Copy link
Contributor

@DarioSamo DarioSamo Feb 17, 2026

Choose a reason for hiding this comment

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

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?

Copy link
Author

@rkitover rkitover Feb 17, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@DarioSamo DarioSamo Feb 17, 2026

Choose a reason for hiding this comment

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

I see, so merging this and the other related PRs won't fix the major leak yet?

Copy link
Author

@rkitover rkitover Feb 17, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@squidbus squidbus Feb 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

@rkitover rkitover Feb 18, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@DarioSamo DarioSamo Feb 18, 2026

Choose a reason for hiding this comment

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

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.


// Create a new command buffer to encode the wait semaphores into
MTL::CommandBuffer* cmdBuffer = mtl->commandBufferWithUnretainedReferences();
cmdBuffer->setLabel(MTLSTR("Wait Command Buffer"));
Expand Down Expand Up @@ -3725,6 +3738,8 @@ namespace plume {

MetalCommandList *mutableCommandList = const_cast<MetalCommandList*>(interfaceCommandList);
mutableCommandList->commit();

releasePool->release();
}

void MetalCommandQueue::waitForCommandFence(RenderCommandFence *fence) {
Expand Down Expand Up @@ -3858,6 +3873,9 @@ namespace plume {
clearVertexFunction->release();
clearColorFunction->release();
clearDepthFunction->release();
clearDepthState->release();
clearDepthStencilState->release();
clearStencilState->release();
sharedBlitDescriptor->release();

if (gpuAddressableResidencySet != nullptr) {
Expand Down Expand Up @@ -4084,6 +4102,7 @@ namespace plume {
MTL::Library *clearShaderLibrary = mtl->newLibrary(NS::String::string(clear_shader, NS::UTF8StringEncoding), nullptr, &error);
if (error != nullptr) {
fprintf(stderr, "Error: %s\n", error->localizedDescription()->utf8String());
error->release();
}
assert(clearShaderLibrary != nullptr && "Failed to create clear color library");

Expand Down Expand Up @@ -4147,11 +4166,12 @@ namespace plume {
releasePool->release();

// Fill device names.
const NS::Array* devices = MTL::CopyAllDevices();
NS::Array* devices = MTL::CopyAllDevices();
for (NS::UInteger i = 0; i < devices->count(); i++) {
NS::String* deviceName = ((MTL::Device *)devices->object(i))->name();
deviceNames.push_back(std::string(deviceName->utf8String()));
}
devices->release();
}

MetalInterface::~MetalInterface() {}
Expand Down