diff --git a/Docs/Whats_New.md b/Docs/Whats_New.md index a4ab46a4..434bb858 100644 --- a/Docs/Whats_New.md +++ b/Docs/Whats_New.md @@ -25,6 +25,7 @@ Released TBD - Fix deletion of GPU counter `MTLFence` while it is being used by `MTLCommandBuffer`. - Remove limit on `VkPhysicalDeviceLimits::maxSamplerAllocationCount` when not using Metal argument buffers. - Avoid adjusting SRGB clear color values by half-ULP on GPUs that round float clear colors down. +- Fixes to optimize resource objects retained by descriptors beyond their lifetimes. - `MoltenVKShaderConverter` tool defaults to the highest MSL version supported on runtime OS. - Update *glslang* version, to use `python3` in *glslang* scripts, to replace missing `python` on *macOS 12.3*. - Update to latest SPIRV-Cross: diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKBuffer.h b/MoltenVK/MoltenVK/GPUObjects/MVKBuffer.h index bbb91766..b5bbdfbe 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKBuffer.h +++ b/MoltenVK/MoltenVK/GPUObjects/MVKBuffer.h @@ -87,6 +87,8 @@ public: ~MVKBuffer() override; + void destroy() override; + protected: friend class MVKDeviceMemory; @@ -99,6 +101,7 @@ protected: VkResult flushToDevice(VkDeviceSize offset, VkDeviceSize size); VkResult pullFromDevice(VkDeviceSize offset, VkDeviceSize size); void initExternalMemory(VkExternalMemoryHandleTypeFlags handleTypes); + void detachMemory(); VkBufferUsageFlags _usage; bool _isHostCoherentTexelBuffer = false; @@ -133,8 +136,11 @@ public: ~MVKBufferView() override; + void destroy() override; + protected: void propagateDebugName() override; + void detachMemory(); MVKBuffer* _buffer; NSUInteger _offset; diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKBuffer.mm b/MoltenVK/MoltenVK/GPUObjects/MVKBuffer.mm index 2eb253a5..7975f72b 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKBuffer.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKBuffer.mm @@ -241,10 +241,33 @@ void MVKBuffer::initExternalMemory(VkExternalMemoryHandleTypeFlags handleTypes) } } +// Memory detached in destructor too, as a fail-safe. MVKBuffer::~MVKBuffer() { + detachMemory(); +} + +// Overridden to detach from the resource memory when the app destroys this object. +// This object can be retained in a descriptor after the app destroys it, even +// though the descriptor can't use it. But doing so retains usuable resource memory. +// In addition, a potential race condition exists if the app updates the descriptor +// on one thread at the same time it is destroying the buffer and freeing the device +// memory on another thread. The race condition occurs when the device memory calls +// back to this buffer to unbind from it. By detaching from the device memory here, +// (when the app destroys the buffer), even if this buffer is retained by a descriptor, +// when the device memory is freed by the app, it won't try to call back here to unbind. +void MVKBuffer::destroy() { + detachMemory(); + MVKResource::destroy(); +} + +// Potentially called twice, from destroy() and destructor, so ensure everything is nulled out. +void MVKBuffer::detachMemory() { if (_deviceMemory) { _deviceMemory->removeBuffer(this); } - if (_mtlBuffer) { [_mtlBuffer release]; } - if (_mtlBufferCache) { [_mtlBufferCache release]; } + _deviceMemory = nullptr; + [_mtlBuffer release]; + _mtlBuffer = nil; + [_mtlBufferCache release]; + _mtlBufferCache = nil; } @@ -342,8 +365,21 @@ MVKBufferView::MVKBufferView(MVKDevice* device, const VkBufferViewCreateInfo* pC } } +// Memory detached in destructor too, as a fail-safe. MVKBufferView::~MVKBufferView() { - [_mtlTexture release]; - _mtlTexture = nil; + detachMemory(); } +// Overridden to detach from the resource memory when the app destroys this object. +// This object can be retained in a descriptor after the app destroys it, even +// though the descriptor can't use it. But doing so retains usuable resource memory. +void MVKBufferView::destroy() { + detachMemory(); + MVKVulkanAPIDeviceObject::destroy(); +} + +// Potentially called twice, from destroy() and destructor, so ensure everything is nulled out. +void MVKBufferView::detachMemory() { + [_mtlTexture release]; + _mtlTexture = nil; +} diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKImage.h b/MoltenVK/MoltenVK/GPUObjects/MVKImage.h index 17fbf4b1..01decf8f 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKImage.h +++ b/MoltenVK/MoltenVK/GPUObjects/MVKImage.h @@ -565,14 +565,14 @@ public: #pragma mark Metal - /** Returns the Metal texture underlying this image view. */ - id getMTLTexture(uint8_t planeIndex = 0) { return _planes[planeIndex]->getMTLTexture(); } + /** Returns the Metal texture underlying this image view. */ + id getMTLTexture(uint8_t planeIndex = 0) { return planeIndex < _planes.size() ? _planes[planeIndex]->getMTLTexture() : nil; } // Guard against destroyed instance retained in a descriptor. /** Returns the Metal pixel format of this image view. */ - MTLPixelFormat getMTLPixelFormat(uint8_t planeIndex = 0) { return _planes[planeIndex]->_mtlPixFmt; } + MTLPixelFormat getMTLPixelFormat(uint8_t planeIndex = 0) { return planeIndex < _planes.size() ? _planes[planeIndex]->_mtlPixFmt : MTLPixelFormatInvalid; } // Guard against destroyed instance retained in a descriptor. /** Returns the packed component swizzle of this image view. */ - uint32_t getPackedSwizzle() { return _planes[0]->getPackedSwizzle(); } + uint32_t getPackedSwizzle() { return _planes.empty() ? 0 : _planes[0]->getPackedSwizzle(); } // Guard against destroyed instance retained in a descriptor. /** Returns the number of planes of this image view. */ uint8_t getPlaneCount() { return _planes.size(); } @@ -599,10 +599,13 @@ public: ~MVKImageView(); + void destroy() override; + protected: friend MVKImageViewPlane; void propagateDebugName() override; + void detachMemory(); MVKImage* _image; MVKSmallVector _planes; @@ -686,10 +689,13 @@ public: ~MVKSampler() override; + void destroy() override; + protected: void propagateDebugName() override {} MTLSamplerDescriptor* newMTLSamplerDescriptor(const VkSamplerCreateInfo* pCreateInfo); void initConstExprSampler(const VkSamplerCreateInfo* pCreateInfo); + void detachMemory(); id _mtlSamplerState; SPIRV_CROSS_NAMESPACE::MSLConstexprSampler _constExprSampler; diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm b/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm index 1d6d00d7..da99774a 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm @@ -1847,7 +1847,21 @@ MVKImageView::MVKImageView(MVKDevice* device, const VkImageViewCreateInfo* pCrea } } +// Memory detached in destructor too, as a fail-safe. MVKImageView::~MVKImageView() { + detachMemory(); +} + +// Overridden to detach from the resource memory when the app destroys this object. +// This object can be retained in a descriptor after the app destroys it, even +// though the descriptor can't use it. But doing so retains usuable resource memory. +void MVKImageView::destroy() { + detachMemory(); + MVKVulkanAPIDeviceObject::destroy(); +} + +// Potentially called twice, from destroy() and destructor, so ensure everything is nulled out. +void MVKImageView::detachMemory() { mvkDestroyContainerContents(_planes); } @@ -2101,8 +2115,23 @@ void MVKSampler::initConstExprSampler(const VkSamplerCreateInfo* pCreateInfo) { } } +// Memory detached in destructor too, as a fail-safe. MVKSampler::~MVKSampler() { + detachMemory(); +} + +// Overridden to detach from the resource memory when the app destroys this object. +// This object can be retained in a descriptor after the app destroys it, even +// though the descriptor can't use it. But doing so retains usuable resource memory. +void MVKSampler::destroy() { + detachMemory(); + MVKVulkanAPIDeviceObject::destroy(); +} + +// Potentially called twice, from destroy() and destructor, so ensure everything is nulled out. +void MVKSampler::detachMemory() { @synchronized (getMTLDevice()) { [_mtlSamplerState release]; + _mtlSamplerState = nil; } } diff --git a/MoltenVK/MoltenVK/Utility/MVKBaseObject.h b/MoltenVK/MoltenVK/Utility/MVKBaseObject.h index 370a55a0..f06fe105 100644 --- a/MoltenVK/MoltenVK/Utility/MVKBaseObject.h +++ b/MoltenVK/MoltenVK/Utility/MVKBaseObject.h @@ -135,6 +135,9 @@ public: * Once all references have been released, this object is free to be deleted. * If the destroy() function has already been called on this instance by the time * this function is called, this instance will be deleted. + * + * Note that the destroy() function is called on the BaseClass. + * Releasing will not call any overridden destroy() function in a descendant class. */ void release() { if (--_refCount == 0) { BaseClass::destroy(); } }