From d1a6b5a40dbc18d5615a8b30e3f7ab3147c9efdb Mon Sep 17 00:00:00 2001 From: Bill Hollings Date: Mon, 15 Jul 2019 18:59:52 -0400 Subject: [PATCH] Remove exposure to leakable instances of NSString, NSArray & NSDictionary. Reduce creation of autoreleased instances of NSString, NSArray & NSDictionary. Ensure remaining are covered by deliberate autorelease pools. --- Common/MVKOSExtensions.mm | 10 +- Docs/Whats_New.md | 5 +- .../Commands/MVKCommandResourceFactory.mm | 10 +- MoltenVK/MoltenVK/GPUObjects/MVKImage.mm | 18 ++-- MoltenVK/MoltenVK/GPUObjects/MVKInstance.mm | 52 +++++---- .../MoltenVK/GPUObjects/MVKShaderModule.mm | 101 ++++++++++-------- MoltenVK/MoltenVK/GPUObjects/MVKSwapchain.mm | 4 +- MoltenVK/MoltenVK/GPUObjects/MVKSync.mm | 6 +- .../MoltenVK/GPUObjects/MVKVulkanAPIObject.mm | 2 +- MoltenVK/MoltenVK/OS/MVKGPUCapture.mm | 4 +- MoltenVK/MoltenVK/Utility/MVKWatermark.mm | 4 +- 11 files changed, 123 insertions(+), 93 deletions(-) diff --git a/Common/MVKOSExtensions.mm b/Common/MVKOSExtensions.mm index 7759e48e..39ef03b5 100644 --- a/Common/MVKOSExtensions.mm +++ b/Common/MVKOSExtensions.mm @@ -76,10 +76,12 @@ void mvkDispatchToMainAndWait(dispatch_block_t block) { #pragma mark Process environment string mvkGetEnvVar(string varName, bool* pWasFound) { - NSDictionary* env = [[NSProcessInfo processInfo] environment]; - NSString* envStr = env[@(varName.c_str())]; - if (pWasFound) { *pWasFound = envStr != nil; } - return envStr ? envStr.UTF8String : ""; + @autoreleasepool { + NSDictionary*nsEnv = [[NSProcessInfo processInfo] environment]; + NSString* envStr = nsEnv[@(varName.c_str())]; + if (pWasFound) { *pWasFound = envStr != nil; } + return envStr ? envStr.UTF8String : ""; + } } int64_t mvkGetEnvVarInt64(string varName, bool* pWasFound) { diff --git a/Docs/Whats_New.md b/Docs/Whats_New.md index 22a63de6..d763dc3f 100644 --- a/Docs/Whats_New.md +++ b/Docs/Whats_New.md @@ -31,9 +31,12 @@ Released TBD - Fix pipeline cache lookups. - Fix race condition between swapchain image destruction and presentation completion callback. - Set Metal texture usage to allow texture copy via view. +- Fix memory leak in debug marker and debug utils labelling. +- Reduce use of autoreleased Obj-C objects, and ensure those remaining are + covered by deliberate autorelease pools. - `vkCmdCopyImage()` support copying between compressed and uncompressed formats and validate that formats are compatible for copying. -- `vkCmdBufferImageCopy()` fix crash when setting bytes per image in non-arrayed images. +- `vkCmdBufferImageCopy()` fix crash when setting bytes per image in non-arrayed images. - Document that the functions in `vk_mvk_moltenvk.h` cannot be used with objects retrieved through the *Vulkan SDK Loader and Layers* framework. - Update `VK_MVK_MOLTENVK_SPEC_VERSION` to 21. diff --git a/MoltenVK/MoltenVK/Commands/MVKCommandResourceFactory.mm b/MoltenVK/MoltenVK/Commands/MVKCommandResourceFactory.mm index bf86b8f3..a7eb94c1 100644 --- a/MoltenVK/MoltenVK/Commands/MVKCommandResourceFactory.mm +++ b/MoltenVK/MoltenVK/Commands/MVKCommandResourceFactory.mm @@ -398,10 +398,12 @@ id MVKCommandResourceFactory::newCmdCopyQueryPoolResult #pragma mark Support methods id MVKCommandResourceFactory::getFunctionNamed(const char* funcName) { - uint64_t startTime = _device->getPerformanceTimestamp(); - id mtlFunc = [[_mtlLibrary newFunctionWithName: @(funcName)] autorelease]; - _device->addActivityPerformance(_device->_performanceStatistics.shaderCompilation.functionRetrieval, startTime); - return mtlFunc; + uint64_t startTime = _device->getPerformanceTimestamp(); + NSString* nsFuncName = [[NSString alloc] initWithUTF8String: funcName]; // temp retained + id mtlFunc = [[_mtlLibrary newFunctionWithName: nsFuncName] autorelease]; + [nsFuncName release]; // release temp NSStr + _device->addActivityPerformance(_device->_performanceStatistics.shaderCompilation.functionRetrieval, startTime); + return mtlFunc; } id MVKCommandResourceFactory::newMTLFunction(NSString* mslSrcCode, NSString* funcName) { diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm b/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm index de49bb3f..7c7185cf 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm @@ -372,14 +372,16 @@ VkResult MVKImage::useIOSurface(IOSurfaceRef ioSurface) { } else { #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wdeprecated-declarations" - _ioSurface = IOSurfaceCreate((CFDictionaryRef)@{ - (id)kIOSurfaceWidth: @(_extent.width), - (id)kIOSurfaceHeight: @(_extent.height), - (id)kIOSurfaceBytesPerElement: @(mvkMTLPixelFormatBytesPerBlock(_mtlPixelFormat)), - (id)kIOSurfaceElementWidth: @(mvkMTLPixelFormatBlockTexelSize(_mtlPixelFormat).width), - (id)kIOSurfaceElementHeight: @(mvkMTLPixelFormatBlockTexelSize(_mtlPixelFormat).height), - (id)kIOSurfaceIsGlobal: @(true), // Deprecated but needed for interprocess transfers - }); + @autoreleasepool { + _ioSurface = IOSurfaceCreate((CFDictionaryRef)@{ + (id)kIOSurfaceWidth: @(_extent.width), + (id)kIOSurfaceHeight: @(_extent.height), + (id)kIOSurfaceBytesPerElement: @(mvkMTLPixelFormatBytesPerBlock(_mtlPixelFormat)), + (id)kIOSurfaceElementWidth: @(mvkMTLPixelFormatBlockTexelSize(_mtlPixelFormat).width), + (id)kIOSurfaceElementHeight: @(mvkMTLPixelFormatBlockTexelSize(_mtlPixelFormat).height), + (id)kIOSurfaceIsGlobal: @(true), // Deprecated but needed for interprocess transfers + }); + } #pragma clang diagnostic pop } diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKInstance.mm b/MoltenVK/MoltenVK/GPUObjects/MVKInstance.mm index aae5cf40..88c41aff 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKInstance.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKInstance.mm @@ -252,28 +252,28 @@ VkDebugUtilsMessageSeverityFlagBitsEXT MVKInstance::getVkDebugUtilsMessageSeveri #pragma mark Object Creation -// Returns an autoreleased array containing the MTLDevices available on this system, -// sorted according to power, with higher power GPU's at the front of the array. -// This ensures that a lazy app that simply grabs the first GPU will get a high-power -// one by default. If the MVK_CONFIG_FORCE_LOW_POWER_GPU env var or build setting is set, -// the returned array will only include low-power devices. -// If Metal is not supported, ensure we return an empty array. -static NSArray>* getAvailableMTLDevices() { +// Returns a new array containing the MTLDevices available on this system, sorted according to power, +// with higher power GPU's at the front of the array. This ensures that a lazy app that simply +// grabs the first GPU will get a high-power one by default. If the MVK_CONFIG_FORCE_LOW_POWER_GPU +// env var or build setting is set, the returned array will only include low-power devices. +// It is the caller's responsibility to release the array when not required anymore. +// If Metal is not supported, returns an empty array. +static NSArray>* newAvailableMTLDevicesArray() { + NSMutableArray* mtlDevs = [NSMutableArray new]; + #if MVK_MACOS - NSArray* mtlDevs = [MTLCopyAllDevices() autorelease]; - if ( !mtlDevs ) { return @[]; } + NSArray* rawMTLDevs = MTLCopyAllDevices(); // temp retain + if (rawMTLDevs) { + bool forceLowPower = MVK_CONFIG_FORCE_LOW_POWER_GPU; + MVK_SET_FROM_ENV_OR_BUILD_BOOL(forceLowPower, MVK_CONFIG_FORCE_LOW_POWER_GPU); - bool forceLowPower = MVK_CONFIG_FORCE_LOW_POWER_GPU; - MVK_SET_FROM_ENV_OR_BUILD_BOOL(forceLowPower, MVK_CONFIG_FORCE_LOW_POWER_GPU); - - if (forceLowPower) { - NSMutableArray* lpDevs = [[NSMutableArray new] autorelease]; - for (id md in mtlDevs) { - if (md.isLowPower) { [lpDevs addObject: md]; } + // Populate the array of appropriate MTLDevices + for (id md in rawMTLDevs) { + if ( !forceLowPower || md.isLowPower ) { [mtlDevs addObject: md]; } } - return lpDevs; - } else { - return [mtlDevs sortedArrayUsingComparator: ^(id md1, id md2) { + + // Sort by power + [mtlDevs sortUsingComparator: ^(id md1, id md2) { BOOL md1IsLP = md1.isLowPower; BOOL md2IsLP = md2.isLowPower; @@ -290,14 +290,18 @@ static NSArray>* getAvailableMTLDevices() { return md2IsLP ? NSOrderedAscending : NSOrderedDescending; }]; - } + } + [rawMTLDevs release]; // release temp #endif // MVK_MACOS #if MVK_IOS - id mtlDev = MTLCreateSystemDefaultDevice(); - return mtlDev ? [NSArray arrayWithObject: mtlDev] : @[]; + id md = MTLCreateSystemDefaultDevice(); + if (md) { [mtlDevs addObject: md]; } + [md release]; #endif // MVK_IOS + + return mtlDevs; // retained } MVKInstance::MVKInstance(const VkInstanceCreateInfo* pCreateInfo) : _enabledExtensions(this) { @@ -326,11 +330,13 @@ MVKInstance::MVKInstance(const VkInstanceCreateInfo* pCreateInfo) : _enabledExte } // Populate the array of physical GPU devices - NSArray>* mtlDevices = getAvailableMTLDevices(); + NSArray>* mtlDevices = newAvailableMTLDevicesArray(); // temp retain _physicalDevices.reserve(mtlDevices.count); for (id mtlDev in mtlDevices) { _physicalDevices.push_back(new MVKPhysicalDevice(this, mtlDev)); } + [mtlDevices release]; // release temp + if (_physicalDevices.empty()) { setConfigurationResult(reportError(VK_ERROR_INCOMPATIBLE_DRIVER, "Vulkan is not supported on this device. MoltenVK requires Metal, which is not available on this device.")); } diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.mm b/MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.mm index b80362bc..a243ffb2 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.mm @@ -49,57 +49,60 @@ MVKMTLFunction MVKShaderLibrary::getMTLFunction(const VkSpecializationInfo* pSpe if ( !_mtlLibrary ) { return MVKMTLFunctionNull; } - NSString* mtlFuncName = @(_shaderConversionResults.entryPoint.mtlFunctionName.c_str()); - MVKDevice* mvkDev = _owner->getDevice(); - uint64_t startTime = mvkDev->getPerformanceTimestamp(); - id mtlFunc = [[_mtlLibrary newFunctionWithName: mtlFuncName] autorelease]; - mvkDev->addActivityPerformance(mvkDev->_performanceStatistics.shaderCompilation.functionRetrieval, startTime); + id mtlFunc = nil; + @autoreleasepool { + NSString* mtlFuncName = @(_shaderConversionResults.entryPoint.mtlFunctionName.c_str()); + MVKDevice* mvkDev = _owner->getDevice(); + uint64_t startTime = mvkDev->getPerformanceTimestamp(); + mtlFunc = [_mtlLibrary newFunctionWithName: mtlFuncName]; // retained + mvkDev->addActivityPerformance(mvkDev->_performanceStatistics.shaderCompilation.functionRetrieval, startTime); - if (mtlFunc) { - // If the Metal device supports shader specialization, and the Metal function expects to be - // specialized, populate Metal function constant values from the Vulkan specialization info, - // and compiled a specialized Metal function, otherwise simply use the unspecialized Metal function. - if (mvkDev->_pMetalFeatures->shaderSpecialization) { - NSArray* mtlFCs = mtlFunc.functionConstantsDictionary.allValues; - if (mtlFCs.count) { - // The Metal shader contains function constants and expects to be specialized - // Populate the Metal function constant values from the Vulkan specialization info. - MTLFunctionConstantValues* mtlFCVals = [[MTLFunctionConstantValues new] autorelease]; - if (pSpecializationInfo) { - // Iterate through the provided Vulkan specialization entries, and populate the - // Metal function constant value that matches the Vulkan specialization constantID. - for (uint32_t specIdx = 0; specIdx < pSpecializationInfo->mapEntryCount; specIdx++) { - const VkSpecializationMapEntry* pMapEntry = &pSpecializationInfo->pMapEntries[specIdx]; - NSUInteger mtlFCIndex = pMapEntry->constantID; - MTLFunctionConstant* mtlFC = getFunctionConstant(mtlFCs, mtlFCIndex); - if (mtlFC) { - [mtlFCVals setConstantValue: &(((char*)pSpecializationInfo->pData)[pMapEntry->offset]) - type: mtlFC.type - atIndex: mtlFCIndex]; - } - } - } + if (mtlFunc) { + // If the Metal device supports shader specialization, and the Metal function expects to be + // specialized, populate Metal function constant values from the Vulkan specialization info, + // and compiled a specialized Metal function, otherwise simply use the unspecialized Metal function. + if (mvkDev->_pMetalFeatures->shaderSpecialization) { + NSArray* mtlFCs = mtlFunc.functionConstantsDictionary.allValues; + if (mtlFCs.count) { + // The Metal shader contains function constants and expects to be specialized + // Populate the Metal function constant values from the Vulkan specialization info. + MTLFunctionConstantValues* mtlFCVals = [MTLFunctionConstantValues new]; // temp retain + if (pSpecializationInfo) { + // Iterate through the provided Vulkan specialization entries, and populate the + // Metal function constant value that matches the Vulkan specialization constantID. + for (uint32_t specIdx = 0; specIdx < pSpecializationInfo->mapEntryCount; specIdx++) { + const VkSpecializationMapEntry* pMapEntry = &pSpecializationInfo->pMapEntries[specIdx]; + NSUInteger mtlFCIndex = pMapEntry->constantID; + MTLFunctionConstant* mtlFC = getFunctionConstant(mtlFCs, mtlFCIndex); + if (mtlFC) { + [mtlFCVals setConstantValue: &(((char*)pSpecializationInfo->pData)[pMapEntry->offset]) + type: mtlFC.type + atIndex: mtlFCIndex]; + } + } + } - // Compile the specialized Metal function, and use it instead of the unspecialized Metal function. - MVKFunctionSpecializer* fs = new MVKFunctionSpecializer(_owner); - mtlFunc = [fs->newMTLFunction(_mtlLibrary, mtlFuncName, mtlFCVals) autorelease]; - fs->destroy(); - } - } - } else { - reportError(VK_ERROR_INVALID_SHADER_NV, "Shader module does not contain an entry point named '%s'.", mtlFuncName.UTF8String); - } - - // Set the debug name. First try name of shader module, otherwise try name of owner. - NSString* dbName = shaderModule-> getDebugName(); - if ( !dbName ) { dbName = _owner-> getDebugName(); } - setLabelIfNotNil(mtlFunc, dbName); + // Compile the specialized Metal function, and use it instead of the unspecialized Metal function. + MVKFunctionSpecializer* fs = new MVKFunctionSpecializer(_owner); + mtlFunc = fs->newMTLFunction(_mtlLibrary, mtlFuncName, mtlFCVals); // retained + fs->destroy(); + [mtlFCVals release]; // release temp + } + } + } else { + reportError(VK_ERROR_INVALID_SHADER_NV, "Shader module does not contain an entry point named '%s'.", mtlFuncName.UTF8String); + } + // Set the debug name. First try name of shader module, otherwise try name of owner. + NSString* dbName = shaderModule-> getDebugName(); + if ( !dbName ) { dbName = _owner-> getDebugName(); } + setLabelIfNotNil(mtlFunc, dbName); + } auto& wgSize = _shaderConversionResults.entryPoint.workgroupSize; - return { mtlFunc, _shaderConversionResults, MTLSizeMake(getWorkgroupDimensionSize(wgSize.width, pSpecializationInfo), - getWorkgroupDimensionSize(wgSize.height, pSpecializationInfo), - getWorkgroupDimensionSize(wgSize.depth, pSpecializationInfo))}; + return { [mtlFunc autorelease], _shaderConversionResults, MTLSizeMake(getWorkgroupDimensionSize(wgSize.width, pSpecializationInfo), + getWorkgroupDimensionSize(wgSize.height, pSpecializationInfo), + getWorkgroupDimensionSize(wgSize.depth, pSpecializationInfo))}; } // Returns the MTLFunctionConstant with the specified ID from the specified array of function constants. @@ -124,7 +127,11 @@ MVKShaderLibrary::MVKShaderLibrary(MVKVulkanAPIDeviceObject* owner, const string& mslSourceCode, const SPIRVToMSLConversionResults& shaderConversionResults) : _owner(owner) { MVKShaderLibraryCompiler* slc = new MVKShaderLibraryCompiler(_owner); - _mtlLibrary = slc->newMTLLibrary(@(mslSourceCode.c_str())); // retained + + NSString* nsSrc = [[NSString alloc] initWithUTF8String: mslSourceCode.c_str()]; // temp retained + _mtlLibrary = slc->newMTLLibrary(nsSrc); // retained + [nsSrc release]; // release temp string + slc->destroy(); _shaderConversionResults = shaderConversionResults; diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKSwapchain.mm b/MoltenVK/MoltenVK/GPUObjects/MVKSwapchain.mm index 992f1cb2..c45c8a68 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKSwapchain.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKSwapchain.mm @@ -38,7 +38,9 @@ void MVKSwapchain::propogateDebugName() { if (_debugName) { size_t imgCnt = _surfaceImages.size(); for (size_t imgIdx = 0; imgIdx < imgCnt; imgIdx++) { - _surfaceImages[imgIdx]->setDebugName([NSString stringWithFormat: @"%@(%lu)", _debugName, imgIdx].UTF8String); + NSString* nsName = [[NSString alloc] initWithFormat: @"%@(%lu)", _debugName, imgIdx]; // temp retain + _surfaceImages[imgIdx]->setDebugName(nsName.UTF8String); + [nsName release]; // release temp string } } } diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKSync.mm b/MoltenVK/MoltenVK/GPUObjects/MVKSync.mm index 171df2a7..f0a1b1cf 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKSync.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKSync.mm @@ -215,8 +215,10 @@ void MVKMetalCompiler::compile(unique_lock& lock, dispatch_block_t block) _blocker.wait_for(lock, nanoTimeout, [this]{ return _isCompileDone; }); if ( !_isCompileDone ) { - NSString* errDesc = [NSString stringWithFormat: @"Timeout after %.3f milliseconds. Likely internal Metal compiler error", (double)nanoTimeout.count() / 1e6]; - _compileError = [[NSError alloc] initWithDomain: @"MoltenVK" code: 1 userInfo: @{NSLocalizedDescriptionKey : errDesc}]; // retained + @autoreleasepool { + NSString* errDesc = [NSString stringWithFormat: @"Timeout after %.3f milliseconds. Likely internal Metal compiler error", (double)nanoTimeout.count() / 1e6]; + _compileError = [[NSError alloc] initWithDomain: @"MoltenVK" code: 1 userInfo: @{NSLocalizedDescriptionKey : errDesc}]; // retained + } } if (_compileError) { handleError(); } diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKVulkanAPIObject.mm b/MoltenVK/MoltenVK/GPUObjects/MVKVulkanAPIObject.mm index 6a390b18..109844da 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKVulkanAPIObject.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKVulkanAPIObject.mm @@ -39,7 +39,7 @@ void MVKVulkanAPIObject::destroy() { VkResult MVKVulkanAPIObject::setDebugName(const char* pObjectName) { if (pObjectName) { [_debugName release]; - _debugName = [[NSString stringWithUTF8String: pObjectName] retain]; // retained + _debugName = [[NSString alloc] initWithUTF8String: pObjectName]; // retained propogateDebugName(); } return VK_SUCCESS; diff --git a/MoltenVK/MoltenVK/OS/MVKGPUCapture.mm b/MoltenVK/MoltenVK/OS/MVKGPUCapture.mm index 156a16e4..4ae0de97 100644 --- a/MoltenVK/MoltenVK/OS/MVKGPUCapture.mm +++ b/MoltenVK/MoltenVK/OS/MVKGPUCapture.mm @@ -59,8 +59,10 @@ void MVKGPUCaptureScope::makeDefault() { MVKGPUCaptureScope::MVKGPUCaptureScope(MVKQueue* mvkQueue, const char* purpose) : _queue(mvkQueue) { _mtlQueue = [_queue->getMTLCommandQueue() retain]; // retained if (mvkOSVersion() >= kMinOSVersionMTLCaptureScope) { + NSString* nsQLbl = [[NSString alloc] initWithUTF8String: (_queue->getName() + "-" + purpose).c_str()]; // temp retained _mtlCaptureScope = [[MTLCaptureManager sharedCaptureManager] newCaptureScopeWithCommandQueue: _mtlQueue]; // retained - _mtlCaptureScope.label = @((_queue->getName() + "-" + purpose).c_str()); + _mtlCaptureScope.label = nsQLbl; + [nsQLbl release]; // release temp } } diff --git a/MoltenVK/MoltenVK/Utility/MVKWatermark.mm b/MoltenVK/MoltenVK/Utility/MVKWatermark.mm index db7740ca..aa06d51b 100644 --- a/MoltenVK/MoltenVK/Utility/MVKWatermark.mm +++ b/MoltenVK/MoltenVK/Utility/MVKWatermark.mm @@ -304,9 +304,11 @@ void MVKWatermark::initTexture(unsigned char* textureContent, // Initialize the shader functions for rendering the watermark void MVKWatermark::initShaders(const char* mslSourceCode) { NSError* err = nil; - id mtlLib = [[_mtlDevice newLibraryWithSource: @(mslSourceCode) + NSString* nsSrc = [[NSString alloc] initWithUTF8String: mslSourceCode]; // temp retained + id mtlLib = [[_mtlDevice newLibraryWithSource: nsSrc options: nil error: &err] autorelease]; + [nsSrc release]; // release temp string MVKAssert( !err, "Could not compile watermark shaders (Error code %li):\n%s", (long)err.code, err.localizedDescription.UTF8String); _mtlFunctionVertex = [mtlLib newFunctionWithName: @"watermarkVertex"]; // retained