KhronosGroup / MoltenVK

MoltenVK is a Vulkan Portability implementation. It layers a subset of the high-performance, industry-standard Vulkan graphics and compute API over Apple's Metal graphics framework, enabling Vulkan applications to run on macOS, iOS and tvOS.
Apache License 2.0
4.63k stars 402 forks source link

Memory leak in vkQueueWaitIdle (macos) #2223

Closed ulowen closed 2 months ago

ulowen commented 2 months ago

Hi,

macOS version 13.0, Xcode version 14.2, Vulkan SDK version 1.3.280.0

I noticed my application's memory usage was growing steadily, I fired up instruments and found that vkQueueWaitIdle seems to regularly allocating a command buffer and it never being freed (screenshot attached). As a simple test case, I used https://github.com/SaschaWillems/Vulkan 's repo, compiling the instancing program and I observed the same memory growth associated with vkQueueWaitIdle. His code calls vkQueueWaitIdle like:

void VulkanExampleBase::submitFrame()
{
    VkResult result = swapChain.queuePresent(queue, currentBuffer, semaphores.renderComplete);
    // Recreate the swapchain if it's no longer compatible with the surface (OUT_OF_DATE) or no longer optimal for presentation (SUBOPTIMAL)
    if ((result == VK_ERROR_OUT_OF_DATE_KHR) || (result == VK_SUBOPTIMAL_KHR)) {
        windowResize();
        if (result == VK_ERROR_OUT_OF_DATE_KHR) {
            return;
        }
    }
    else {
        VK_CHECK_RESULT(result);
    }
    VK_CHECK_RESULT(vkQueueWaitIdle(queue));
}

My code was similar, however I was following the image transition section of this tutorial: https://vulkan-tutorial.com/Texture_mapping/Images

where the tutorial instructs you to use queues to synchronise image updates, like:

VkPipelineStageFlags sourceStage;
VkPipelineStageFlags destinationStage;

if (oldLayout == VK_IMAGE_LAYOUT_UNDEFINED && newLayout == VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL) {
    barrier.srcAccessMask = 0;
    barrier.dstAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT;

    sourceStage = VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT;
    destinationStage = VK_PIPELINE_STAGE_TRANSFER_BIT;
} else if (oldLayout == VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL && newLayout == VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL) {
    barrier.srcAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT;
    barrier.dstAccessMask = VK_ACCESS_SHADER_READ_BIT;

    sourceStage = VK_PIPELINE_STAGE_TRANSFER_BIT;
    destinationStage = VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT;
} else {
    throw std::invalid_argument("unsupported layout transition!");
}

vkCmdPipelineBarrier(
    commandBuffer,
    sourceStage, destinationStage,
    0,
    0, nullptr,
    0, nullptr,
    1, &barrier
);

// Followed later by:
    vkEndCommandBuffer(commandBuffer);

    VkSubmitInfo submitInfo{};
    submitInfo.sType = VK_STRUCTURE_TYPE_SUBMIT_INFO;
    submitInfo.commandBufferCount = 1;
    submitInfo.pCommandBuffers = &commandBuffer;

    vkQueueSubmit(graphicsQueue, 1, &submitInfo, VK_NULL_HANDLE);
    vkQueueWaitIdle(graphicsQueue);

    vkFreeCommandBuffers(device, commandPool, 1, &commandBuffer);

See below trace from leaks in instruments:

image
ulowen commented 2 months ago

in MVKQueue.mm:

-       auto* mtlCmdBuff = getMTLCommandBuffer(cmdUse);
-       [mtlCmdBuff commit];
-       [mtlCmdBuff waitUntilCompleted];
+       @autoreleasepool
+       {
+               auto* mtlCmdBuff = getMTLCommandBuffer(cmdUse);
+
+               [mtlCmdBuff commit];
+               [mtlCmdBuff waitUntilCompleted];
+       }

seems to fix it. Is this a valid fix? If so I can pull request.

billhollings commented 2 months ago

seems to fix it. Is this a valid fix? If so I can pull request.

Ah! Good catch, and fix! Thanks. Looks like we missed that @autoreleasepool.

Yes, please submit a PR for this! It would be great if you could do it by Monday (May 6) so we can include it in the upcoming SDK release.

BTW...the style convention for MoltenVK is { at the end of the line. So it would be @autoreleasepool {.

Thanks!