diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp index abb7f1bcf1..70343e2a44 100644 --- a/src/dawn_native/Buffer.cpp +++ b/src/dawn_native/Buffer.cpp @@ -64,13 +64,12 @@ namespace dawn_native { mFakeMappedData = std::unique_ptr(AllocNoThrow(descriptor->size)); } + // Since error buffers in this case may allocate memory, we need to track them + // for destruction on the device. + TrackInDevice(); } } - void ClearMappedData() { - mFakeMappedData.reset(); - } - private: bool IsCPUWritableAtCreation() const override { UNREACHABLE(); @@ -83,14 +82,17 @@ namespace dawn_native { MaybeError MapAsyncImpl(wgpu::MapMode mode, size_t offset, size_t size) override { UNREACHABLE(); } + void* GetMappedPointerImpl() override { return mFakeMappedData.get(); } + void UnmapImpl() override { - UNREACHABLE(); + mFakeMappedData.reset(); } - void DestroyImpl() override { - UNREACHABLE(); + + void DestroyApiObjectImpl() override { + mFakeMappedData.reset(); } std::unique_ptr mFakeMappedData; @@ -153,6 +155,8 @@ namespace dawn_native { if ((mUsage & wgpu::BufferUsage::Indirect) && device->IsValidationEnabled()) { mUsage |= kInternalStorageBuffer; } + + TrackInDevice(); } BufferBase::BufferBase(DeviceBase* device, @@ -166,11 +170,34 @@ namespace dawn_native { } } + BufferBase::BufferBase(DeviceBase* device, BufferState state) + : ApiObjectBase(device, kLabelNotImplemented), mState(state) { + TrackInDevice(); + } + BufferBase::~BufferBase() { - if (mState == BufferState::Mapped) { - ASSERT(!IsError()); - CallMapCallback(mLastMapID, WGPUBufferMapAsyncStatus_DestroyedBeforeCallback); + ASSERT(mState == BufferState::Unmapped || mState == BufferState::Destroyed); + } + + bool BufferBase::DestroyApiObject() { + bool marked = MarkDestroyed(); + if (!marked) { + return false; } + + if (mState == BufferState::Mapped) { + UnmapInternal(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback); + } else if (mState == BufferState::MappedAtCreation) { + if (mStagingBuffer != nullptr) { + mStagingBuffer.reset(); + } else if (mSize != 0) { + UnmapInternal(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback); + } + } + + DestroyApiObjectImpl(); + mState = BufferState::Destroyed; + return true; } // static @@ -366,29 +393,7 @@ namespace dawn_native { } void BufferBase::APIDestroy() { - if (IsError()) { - // It is an error to call Destroy() on an ErrorBuffer, but we still need to reclaim the - // fake mapped staging data. - static_cast(this)->ClearMappedData(); - mState = BufferState::Destroyed; - } - if (GetDevice()->ConsumedError(ValidateDestroy(), "calling %s.Destroy()", this)) { - return; - } - ASSERT(!IsError()); - - if (mState == BufferState::Mapped) { - UnmapInternal(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback); - } else if (mState == BufferState::MappedAtCreation) { - if (mStagingBuffer != nullptr) { - mStagingBuffer.reset(); - } else if (mSize != 0) { - ASSERT(IsCPUWritableAtCreation()); - UnmapInternal(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback); - } - } - - DestroyInternal(); + DestroyApiObject(); } MaybeError BufferBase::CopyFromStagingBuffer() { @@ -409,6 +414,9 @@ namespace dawn_native { } void BufferBase::APIUnmap() { + if (GetDevice()->ConsumedError(ValidateUnmap(), "calling %s.Unmap()", this)) { + return; + } Unmap(); } @@ -417,17 +425,6 @@ namespace dawn_native { } void BufferBase::UnmapInternal(WGPUBufferMapAsyncStatus callbackStatus) { - if (IsError()) { - // It is an error to call Unmap() on an ErrorBuffer, but we still need to reclaim the - // fake mapped staging data. - static_cast(this)->ClearMappedData(); - mState = BufferState::Unmapped; - } - if (GetDevice()->ConsumedError(ValidateUnmap(), "calling %s.Unmap()", this)) { - return; - } - ASSERT(!IsError()); - if (mState == BufferState::Mapped) { // A map request can only be called once, so this will fire only if the request wasn't // completed before the Unmap. @@ -438,12 +435,10 @@ namespace dawn_native { mMapCallback = nullptr; mMapUserdata = 0; - } else if (mState == BufferState::MappedAtCreation) { if (mStagingBuffer != nullptr) { GetDevice()->ConsumedError(CopyFromStagingBuffer()); } else if (mSize != 0) { - ASSERT(IsCPUWritableAtCreation()); UnmapImpl(); } } @@ -543,7 +538,6 @@ namespace dawn_native { MaybeError BufferBase::ValidateUnmap() const { DAWN_TRY(GetDevice()->ValidateIsAlive()); - DAWN_TRY(GetDevice()->ValidateObject(this)); switch (mState) { case BufferState::Mapped: @@ -559,18 +553,6 @@ namespace dawn_native { UNREACHABLE(); } - MaybeError BufferBase::ValidateDestroy() const { - DAWN_TRY(GetDevice()->ValidateObject(this)); - return {}; - } - - void BufferBase::DestroyInternal() { - if (mState != BufferState::Destroyed) { - DestroyImpl(); - } - mState = BufferState::Destroyed; - } - void BufferBase::OnMapRequestCompleted(MapRequestID mapID, WGPUBufferMapAsyncStatus status) { CallMapCallback(mapID, status); } diff --git a/src/dawn_native/Buffer.h b/src/dawn_native/Buffer.h index 6ba57553c3..cc7e32e148 100644 --- a/src/dawn_native/Buffer.h +++ b/src/dawn_native/Buffer.h @@ -41,18 +41,18 @@ namespace dawn_native { wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite; class BufferBase : public ApiObjectBase { + public: enum class BufferState { Unmapped, Mapped, MappedAtCreation, Destroyed, }; - - public: BufferBase(DeviceBase* device, const BufferDescriptor* descriptor); static BufferBase* MakeError(DeviceBase* device, const BufferDescriptor* descriptor); + bool DestroyApiObject() override; ObjectType GetType() const override; uint64_t GetSize() const; @@ -86,9 +86,11 @@ namespace dawn_native { BufferBase(DeviceBase* device, const BufferDescriptor* descriptor, ObjectBase::ErrorTag tag); - ~BufferBase() override; - void DestroyInternal(); + // Constructor used only for mocking and testing. + BufferBase(DeviceBase* device, BufferState state); + + ~BufferBase() override; MaybeError MapAtCreationInternal(); @@ -98,7 +100,6 @@ namespace dawn_native { virtual MaybeError MapAtCreationImpl() = 0; virtual MaybeError MapAsyncImpl(wgpu::MapMode mode, size_t offset, size_t size) = 0; virtual void UnmapImpl() = 0; - virtual void DestroyImpl() = 0; virtual void* GetMappedPointerImpl() = 0; virtual bool IsCPUWritableAtCreation() const = 0; @@ -110,7 +111,6 @@ namespace dawn_native { size_t size, WGPUBufferMapAsyncStatus* status) const; MaybeError ValidateUnmap() const; - MaybeError ValidateDestroy() const; bool CanGetMappedRange(bool writable, size_t offset, size_t size) const; void UnmapInternal(WGPUBufferMapAsyncStatus callbackStatus); diff --git a/src/dawn_native/Device.cpp b/src/dawn_native/Device.cpp index cf33c02a71..f09e671b05 100644 --- a/src/dawn_native/Device.cpp +++ b/src/dawn_native/Device.cpp @@ -268,7 +268,7 @@ namespace dawn_native { // TODO(dawn/628) Add types into the array as they are implemented. // clang-format off - static constexpr std::array kObjectTypeDependencyOrder = { + static constexpr std::array kObjectTypeDependencyOrder = { ObjectType::RenderPipeline, ObjectType::ComputePipeline, ObjectType::PipelineLayout, @@ -276,7 +276,9 @@ namespace dawn_native { ObjectType::BindGroup, ObjectType::BindGroupLayout, ObjectType::ShaderModule, + ObjectType::QuerySet, ObjectType::Sampler, + ObjectType::Buffer, }; // clang-format on diff --git a/src/dawn_native/ObjectBase.cpp b/src/dawn_native/ObjectBase.cpp index 2a1d495a8d..971c0484fa 100644 --- a/src/dawn_native/ObjectBase.cpp +++ b/src/dawn_native/ObjectBase.cpp @@ -80,15 +80,17 @@ namespace dawn_native { GetDevice()->TrackObject(this); } + bool ApiObjectBase::MarkDestroyed() { + const std::lock_guard lock(*GetDevice()->GetObjectListMutex(GetType())); + return RemoveFromList(); + } + bool ApiObjectBase::DestroyApiObject() { - { - const std::lock_guard lock(*GetDevice()->GetObjectListMutex(GetType())); - if (!RemoveFromList()) { - return false; - } + bool marked = MarkDestroyed(); + if (marked) { + DestroyApiObjectImpl(); } - DestroyApiObjectImpl(); - return true; + return marked; } void ApiObjectBase::DestroyApiObjectImpl() { diff --git a/src/dawn_native/ObjectBase.h b/src/dawn_native/ObjectBase.h index 291e806613..00595060d9 100644 --- a/src/dawn_native/ObjectBase.h +++ b/src/dawn_native/ObjectBase.h @@ -83,6 +83,12 @@ namespace dawn_native { // somewhere. void DeleteThis() override; void TrackInDevice(); + + // Thread-safe manner to mark an object as destroyed. Returns true iff the call was the + // "winning" attempt to destroy the object. This is useful when sub-classes may have extra + // pre-destruction steps that need to occur only once, i.e. Buffer needs to be unmapped + // before being destroyed. + bool MarkDestroyed(); virtual void DestroyApiObjectImpl(); private: diff --git a/src/dawn_native/QuerySet.cpp b/src/dawn_native/QuerySet.cpp index 2ad7cd75ae..855277dcdc 100644 --- a/src/dawn_native/QuerySet.cpp +++ b/src/dawn_native/QuerySet.cpp @@ -31,7 +31,7 @@ namespace dawn_native { } private: - void DestroyImpl() override { + void DestroyApiObjectImpl() override { UNREACHABLE(); } }; @@ -110,6 +110,11 @@ namespace dawn_native { } mQueryAvailability.resize(descriptor->count); + TrackInDevice(); + } + + QuerySetBase::QuerySetBase(DeviceBase* device) : ApiObjectBase(device, kLabelNotImplemented) { + TrackInDevice(); } QuerySetBase::QuerySetBase(DeviceBase* device, ObjectBase::ErrorTag tag) @@ -121,6 +126,11 @@ namespace dawn_native { ASSERT(mState == QuerySetState::Unavailable || mState == QuerySetState::Destroyed); } + bool QuerySetBase::DestroyApiObject() { + mState = QuerySetState::Destroyed; + return ApiObjectBase::DestroyApiObject(); + } + // static QuerySetBase* QuerySetBase::MakeError(DeviceBase* device) { return new ErrorQuerySet(device); @@ -160,7 +170,7 @@ namespace dawn_native { if (GetDevice()->ConsumedError(ValidateDestroy())) { return; } - DestroyInternal(); + DestroyApiObject(); } MaybeError QuerySetBase::ValidateDestroy() const { @@ -168,11 +178,4 @@ namespace dawn_native { return {}; } - void QuerySetBase::DestroyInternal() { - if (mState != QuerySetState::Destroyed) { - DestroyImpl(); - } - mState = QuerySetState::Destroyed; - } - } // namespace dawn_native diff --git a/src/dawn_native/QuerySet.h b/src/dawn_native/QuerySet.h index 3ad4e7ca4c..d2a0263a80 100644 --- a/src/dawn_native/QuerySet.h +++ b/src/dawn_native/QuerySet.h @@ -31,6 +31,7 @@ namespace dawn_native { static QuerySetBase* MakeError(DeviceBase* device); + bool DestroyApiObject() override; ObjectType GetType() const override; wgpu::QueryType GetQueryType() const; @@ -46,13 +47,13 @@ namespace dawn_native { protected: QuerySetBase(DeviceBase* device, ObjectBase::ErrorTag tag); + + // Constructor used only for mocking and testing. + QuerySetBase(DeviceBase* device); + ~QuerySetBase() override; - void DestroyInternal(); - private: - virtual void DestroyImpl() = 0; - MaybeError ValidateDestroy() const; wgpu::QueryType mQueryType; diff --git a/src/dawn_native/d3d12/BufferD3D12.cpp b/src/dawn_native/d3d12/BufferD3D12.cpp index 39fcb83980..e157b411b1 100644 --- a/src/dawn_native/d3d12/BufferD3D12.cpp +++ b/src/dawn_native/d3d12/BufferD3D12.cpp @@ -185,9 +185,7 @@ namespace dawn_native { namespace d3d12 { return {}; } - Buffer::~Buffer() { - DestroyInternal(); - } + Buffer::~Buffer() = default; ID3D12Resource* Buffer::GetD3D12Resource() const { return mResourceAllocation.GetD3D12Resource(); @@ -380,7 +378,7 @@ namespace dawn_native { namespace d3d12 { return mMappedData; } - void Buffer::DestroyImpl() { + void Buffer::DestroyApiObjectImpl() { if (mMappedData != nullptr) { // If the buffer is currently mapped, unmap without flushing the writes to the GPU // since the buffer cannot be used anymore. UnmapImpl checks mWrittenRange to know diff --git a/src/dawn_native/d3d12/BufferD3D12.h b/src/dawn_native/d3d12/BufferD3D12.h index d6fcbbdc06..eb9191f42d 100644 --- a/src/dawn_native/d3d12/BufferD3D12.h +++ b/src/dawn_native/d3d12/BufferD3D12.h @@ -58,7 +58,7 @@ namespace dawn_native { namespace d3d12 { MaybeError Initialize(bool mappedAtCreation); MaybeError MapAsyncImpl(wgpu::MapMode mode, size_t offset, size_t size) override; void UnmapImpl() override; - void DestroyImpl() override; + void DestroyApiObjectImpl() override; bool IsCPUWritableAtCreation() const override; virtual MaybeError MapAtCreationImpl() override; void* GetMappedPointerImpl() override; diff --git a/src/dawn_native/d3d12/QuerySetD3D12.cpp b/src/dawn_native/d3d12/QuerySetD3D12.cpp index 4caf9f6eb0..f7669331c0 100644 --- a/src/dawn_native/d3d12/QuerySetD3D12.cpp +++ b/src/dawn_native/d3d12/QuerySetD3D12.cpp @@ -55,12 +55,11 @@ namespace dawn_native { namespace d3d12 { return mQueryHeap.Get(); } - QuerySet::~QuerySet() { - DestroyInternal(); - } + QuerySet::~QuerySet() = default; - void QuerySet::DestroyImpl() { + void QuerySet::DestroyApiObjectImpl() { ToBackend(GetDevice())->ReferenceUntilUnused(mQueryHeap); + mQueryHeap = nullptr; } }} // namespace dawn_native::d3d12 diff --git a/src/dawn_native/d3d12/QuerySetD3D12.h b/src/dawn_native/d3d12/QuerySetD3D12.h index 16c49d199e..dc2c157773 100644 --- a/src/dawn_native/d3d12/QuerySetD3D12.h +++ b/src/dawn_native/d3d12/QuerySetD3D12.h @@ -35,7 +35,7 @@ namespace dawn_native { namespace d3d12 { MaybeError Initialize(); // Dawn API - void DestroyImpl() override; + void DestroyApiObjectImpl() override; ComPtr mQueryHeap; }; diff --git a/src/dawn_native/metal/BufferMTL.h b/src/dawn_native/metal/BufferMTL.h index cf6dc6f6c6..13e89b971b 100644 --- a/src/dawn_native/metal/BufferMTL.h +++ b/src/dawn_native/metal/BufferMTL.h @@ -26,7 +26,7 @@ namespace dawn_native { namespace metal { class CommandRecordingContext; class Device; - class Buffer : public BufferBase { + class Buffer final : public BufferBase { public: static ResultOrError> Create(Device* device, const BufferDescriptor* descriptor); @@ -48,7 +48,7 @@ namespace dawn_native { namespace metal { ~Buffer() override; MaybeError MapAsyncImpl(wgpu::MapMode mode, size_t offset, size_t size) override; void UnmapImpl() override; - void DestroyImpl() override; + void DestroyApiObjectImpl() override; void* GetMappedPointerImpl() override; bool IsCPUWritableAtCreation() const override; MaybeError MapAtCreationImpl() override; diff --git a/src/dawn_native/metal/BufferMTL.mm b/src/dawn_native/metal/BufferMTL.mm index 2b9c2fee01..7ac489cb47 100644 --- a/src/dawn_native/metal/BufferMTL.mm +++ b/src/dawn_native/metal/BufferMTL.mm @@ -140,9 +140,7 @@ namespace dawn_native { namespace metal { return {}; } - Buffer::~Buffer() { - DestroyInternal(); - } + Buffer::~Buffer() = default; id Buffer::GetMTLBuffer() const { return mMtlBuffer.Get(); @@ -173,7 +171,7 @@ namespace dawn_native { namespace metal { // Nothing to do, Metal StorageModeShared buffers are always mapped. } - void Buffer::DestroyImpl() { + void Buffer::DestroyApiObjectImpl() { mMtlBuffer = nullptr; } diff --git a/src/dawn_native/metal/QuerySetMTL.h b/src/dawn_native/metal/QuerySetMTL.h index a7b1ad7fa3..b77bd317b9 100644 --- a/src/dawn_native/metal/QuerySetMTL.h +++ b/src/dawn_native/metal/QuerySetMTL.h @@ -40,7 +40,7 @@ namespace dawn_native { namespace metal { MaybeError Initialize(); // Dawn API - void DestroyImpl() override; + void DestroyApiObjectImpl() override; NSPRef> mVisibilityBuffer; // Note that mCounterSampleBuffer cannot be an NSRef because the API_AVAILABLE macros don't diff --git a/src/dawn_native/metal/QuerySetMTL.mm b/src/dawn_native/metal/QuerySetMTL.mm index dc06045e20..9dea304153 100644 --- a/src/dawn_native/metal/QuerySetMTL.mm +++ b/src/dawn_native/metal/QuerySetMTL.mm @@ -121,11 +121,9 @@ namespace dawn_native { namespace metal { return mCounterSampleBuffer; } - QuerySet::~QuerySet() { - DestroyInternal(); - } + QuerySet::~QuerySet() = default; - void QuerySet::DestroyImpl() { + void QuerySet::DestroyApiObjectImpl() { mVisibilityBuffer = nullptr; // mCounterSampleBuffer isn't an NSRef because API_AVAILABLE doesn't work will with diff --git a/src/dawn_native/null/DeviceNull.cpp b/src/dawn_native/null/DeviceNull.cpp index 6b785a18a4..c07b0ca6dc 100644 --- a/src/dawn_native/null/DeviceNull.cpp +++ b/src/dawn_native/null/DeviceNull.cpp @@ -294,11 +294,6 @@ namespace dawn_native { namespace null { mAllocatedSize = GetSize(); } - Buffer::~Buffer() { - DestroyInternal(); - ToBackend(GetDevice())->DecrementMemoryUsage(GetSize()); - } - bool Buffer::IsCPUWritableAtCreation() const { // Only return true for mappable buffers so we can test cases that need / don't need a // staging buffer. @@ -334,7 +329,8 @@ namespace dawn_native { namespace null { void Buffer::UnmapImpl() { } - void Buffer::DestroyImpl() { + void Buffer::DestroyApiObjectImpl() { + ToBackend(GetDevice())->DecrementMemoryUsage(GetSize()); } // CommandBuffer @@ -349,11 +345,7 @@ namespace dawn_native { namespace null { : QuerySetBase(device, descriptor) { } - QuerySet::~QuerySet() { - DestroyInternal(); - } - - void QuerySet::DestroyImpl() { + void QuerySet::DestroyApiObjectImpl() { } // Queue diff --git a/src/dawn_native/null/DeviceNull.h b/src/dawn_native/null/DeviceNull.h index cef718d285..010d9d1a7b 100644 --- a/src/dawn_native/null/DeviceNull.h +++ b/src/dawn_native/null/DeviceNull.h @@ -226,10 +226,9 @@ namespace dawn_native { namespace null { void DoWriteBuffer(uint64_t bufferOffset, const void* data, size_t size); private: - ~Buffer() override; MaybeError MapAsyncImpl(wgpu::MapMode mode, size_t offset, size_t size) override; void UnmapImpl() override; - void DestroyImpl() override; + void DestroyApiObjectImpl() override; bool IsCPUWritableAtCreation() const override; MaybeError MapAtCreationImpl() override; void* GetMappedPointerImpl() override; @@ -247,9 +246,7 @@ namespace dawn_native { namespace null { QuerySet(Device* device, const QuerySetDescriptor* descriptor); private: - ~QuerySet() override; - - void DestroyImpl() override; + void DestroyApiObjectImpl() override; }; class Queue final : public QueueBase { diff --git a/src/dawn_native/opengl/BufferGL.cpp b/src/dawn_native/opengl/BufferGL.cpp index 1e102d63c5..f03ce49a1c 100644 --- a/src/dawn_native/opengl/BufferGL.cpp +++ b/src/dawn_native/opengl/BufferGL.cpp @@ -61,9 +61,7 @@ namespace dawn_native { namespace opengl { } } - Buffer::~Buffer() { - DestroyInternal(); - } + Buffer::~Buffer() = default; GLuint Buffer::GetHandle() const { return mBuffer; @@ -176,7 +174,7 @@ namespace dawn_native { namespace opengl { mMappedData = nullptr; } - void Buffer::DestroyImpl() { + void Buffer::DestroyApiObjectImpl() { ToBackend(GetDevice())->gl.DeleteBuffers(1, &mBuffer); mBuffer = 0; } diff --git a/src/dawn_native/opengl/BufferGL.h b/src/dawn_native/opengl/BufferGL.h index 2259161e2d..fdba81b29e 100644 --- a/src/dawn_native/opengl/BufferGL.h +++ b/src/dawn_native/opengl/BufferGL.h @@ -42,7 +42,7 @@ namespace dawn_native { namespace opengl { ~Buffer() override; MaybeError MapAsyncImpl(wgpu::MapMode mode, size_t offset, size_t size) override; void UnmapImpl() override; - void DestroyImpl() override; + void DestroyApiObjectImpl() override; bool IsCPUWritableAtCreation() const override; MaybeError MapAtCreationImpl() override; void* GetMappedPointerImpl() override; diff --git a/src/dawn_native/opengl/QuerySetGL.cpp b/src/dawn_native/opengl/QuerySetGL.cpp index 6ff5d20603..6ec90413a2 100644 --- a/src/dawn_native/opengl/QuerySetGL.cpp +++ b/src/dawn_native/opengl/QuerySetGL.cpp @@ -22,11 +22,9 @@ namespace dawn_native { namespace opengl { : QuerySetBase(device, descriptor) { } - QuerySet::~QuerySet() { - DestroyInternal(); - } + QuerySet::~QuerySet() = default; - void QuerySet::DestroyImpl() { + void QuerySet::DestroyApiObjectImpl() { } }} // namespace dawn_native::opengl diff --git a/src/dawn_native/opengl/QuerySetGL.h b/src/dawn_native/opengl/QuerySetGL.h index 2a83bdd046..cd16dc893b 100644 --- a/src/dawn_native/opengl/QuerySetGL.h +++ b/src/dawn_native/opengl/QuerySetGL.h @@ -28,7 +28,7 @@ namespace dawn_native { namespace opengl { private: ~QuerySet() override; - void DestroyImpl() override; + void DestroyApiObjectImpl() override; }; }} // namespace dawn_native::opengl diff --git a/src/dawn_native/vulkan/BufferVk.cpp b/src/dawn_native/vulkan/BufferVk.cpp index 75a32fd6ad..62b6e113f9 100644 --- a/src/dawn_native/vulkan/BufferVk.cpp +++ b/src/dawn_native/vulkan/BufferVk.cpp @@ -236,9 +236,7 @@ namespace dawn_native { namespace vulkan { return {}; } - Buffer::~Buffer() { - DestroyInternal(); - } + Buffer::~Buffer() = default; VkBuffer Buffer::GetHandle() const { return mHandle; @@ -331,7 +329,7 @@ namespace dawn_native { namespace vulkan { return memory; } - void Buffer::DestroyImpl() { + void Buffer::DestroyApiObjectImpl() { ToBackend(GetDevice())->GetResourceMemoryAllocator()->Deallocate(&mMemoryAllocation); if (mHandle != VK_NULL_HANDLE) { diff --git a/src/dawn_native/vulkan/BufferVk.h b/src/dawn_native/vulkan/BufferVk.h index 3fcab6075a..30703c896e 100644 --- a/src/dawn_native/vulkan/BufferVk.h +++ b/src/dawn_native/vulkan/BufferVk.h @@ -65,7 +65,7 @@ namespace dawn_native { namespace vulkan { MaybeError MapAsyncImpl(wgpu::MapMode mode, size_t offset, size_t size) override; void UnmapImpl() override; - void DestroyImpl() override; + void DestroyApiObjectImpl() override; bool IsCPUWritableAtCreation() const override; MaybeError MapAtCreationImpl() override; void* GetMappedPointerImpl() override; diff --git a/src/dawn_native/vulkan/QuerySetVk.cpp b/src/dawn_native/vulkan/QuerySetVk.cpp index ba60c3dc7c..7a15296243 100644 --- a/src/dawn_native/vulkan/QuerySetVk.cpp +++ b/src/dawn_native/vulkan/QuerySetVk.cpp @@ -94,11 +94,9 @@ namespace dawn_native { namespace vulkan { return mHandle; } - QuerySet::~QuerySet() { - DestroyInternal(); - } + QuerySet::~QuerySet() = default; - void QuerySet::DestroyImpl() { + void QuerySet::DestroyApiObjectImpl() { if (mHandle != VK_NULL_HANDLE) { ToBackend(GetDevice())->GetFencedDeleter()->DeleteWhenUnused(mHandle); mHandle = VK_NULL_HANDLE; diff --git a/src/dawn_native/vulkan/QuerySetVk.h b/src/dawn_native/vulkan/QuerySetVk.h index 80e7befa1d..03b0a7b18e 100644 --- a/src/dawn_native/vulkan/QuerySetVk.h +++ b/src/dawn_native/vulkan/QuerySetVk.h @@ -35,7 +35,7 @@ namespace dawn_native { namespace vulkan { using QuerySetBase::QuerySetBase; MaybeError Initialize(); - void DestroyImpl() override; + void DestroyApiObjectImpl() override; VkQueryPool mHandle = VK_NULL_HANDLE; }; diff --git a/src/tests/BUILD.gn b/src/tests/BUILD.gn index 8ffd921524..1fb6de0d5d 100644 --- a/src/tests/BUILD.gn +++ b/src/tests/BUILD.gn @@ -147,6 +147,7 @@ source_set("dawn_native_mocks_sources") { "unittests/native/mocks/ComputePipelineMock.h", "unittests/native/mocks/DeviceMock.h", "unittests/native/mocks/PipelineLayoutMock.h", + "unittests/native/mocks/QuerySetMock.h", "unittests/native/mocks/RenderPipelineMock.h", "unittests/native/mocks/SamplerMock.h", "unittests/native/mocks/ShaderModuleMock.cpp", diff --git a/src/tests/unittests/native/DestroyObjectTests.cpp b/src/tests/unittests/native/DestroyObjectTests.cpp index a82a7f4aa9..e4dee5edb1 100644 --- a/src/tests/unittests/native/DestroyObjectTests.cpp +++ b/src/tests/unittests/native/DestroyObjectTests.cpp @@ -17,9 +17,11 @@ #include "dawn_native/Toggles.h" #include "mocks/BindGroupLayoutMock.h" #include "mocks/BindGroupMock.h" +#include "mocks/BufferMock.h" #include "mocks/ComputePipelineMock.h" #include "mocks/DeviceMock.h" #include "mocks/PipelineLayoutMock.h" +#include "mocks/QuerySetMock.h" #include "mocks/RenderPipelineMock.h" #include "mocks/SamplerMock.h" #include "mocks/ShaderModuleMock.h" @@ -139,6 +141,64 @@ namespace dawn_native { namespace { } } + TEST_F(DestroyObjectTests, BufferExplicit) { + { + BufferMock bufferMock(&mDevice, BufferBase::BufferState::Unmapped); + EXPECT_CALL(bufferMock, DestroyApiObjectImpl).Times(1); + + EXPECT_TRUE(bufferMock.IsAlive()); + bufferMock.DestroyApiObject(); + EXPECT_FALSE(bufferMock.IsAlive()); + } + { + BufferMock bufferMock(&mDevice, BufferBase::BufferState::Mapped); + { + InSequence seq; + EXPECT_CALL(bufferMock, UnmapImpl).Times(1); + EXPECT_CALL(bufferMock, DestroyApiObjectImpl).Times(1); + } + + EXPECT_TRUE(bufferMock.IsAlive()); + bufferMock.DestroyApiObject(); + EXPECT_FALSE(bufferMock.IsAlive()); + } + } + + // If the reference count on API objects reach 0, they should delete themselves. Note that GTest + // will also complain if there is a memory leak. + TEST_F(DestroyObjectTests, BufferImplicit) { + { + BufferMock* bufferMock = new BufferMock(&mDevice, BufferBase::BufferState::Unmapped); + EXPECT_CALL(*bufferMock, DestroyApiObjectImpl).Times(1); + { + BufferDescriptor desc = {}; + Ref buffer; + EXPECT_CALL(mDevice, CreateBufferImpl) + .WillOnce(Return(ByMove(AcquireRef(bufferMock)))); + DAWN_ASSERT_AND_ASSIGN(buffer, mDevice.CreateBuffer(&desc)); + + EXPECT_TRUE(buffer->IsAlive()); + } + } + { + BufferMock* bufferMock = new BufferMock(&mDevice, BufferBase::BufferState::Mapped); + { + InSequence seq; + EXPECT_CALL(*bufferMock, UnmapImpl).Times(1); + EXPECT_CALL(*bufferMock, DestroyApiObjectImpl).Times(1); + } + { + BufferDescriptor desc = {}; + Ref buffer; + EXPECT_CALL(mDevice, CreateBufferImpl) + .WillOnce(Return(ByMove(AcquireRef(bufferMock)))); + DAWN_ASSERT_AND_ASSIGN(buffer, mDevice.CreateBuffer(&desc)); + + EXPECT_TRUE(buffer->IsAlive()); + } + } + } + TEST_F(DestroyObjectTests, ComputePipelineExplicit) { ComputePipelineMock computePipelineMock(&mDevice); EXPECT_CALL(computePipelineMock, DestroyApiObjectImpl).Times(1); @@ -203,6 +263,31 @@ namespace dawn_native { namespace { } } + TEST_F(DestroyObjectTests, QuerySetExplicit) { + QuerySetMock querySetMock(&mDevice); + EXPECT_CALL(querySetMock, DestroyApiObjectImpl).Times(1); + + EXPECT_TRUE(querySetMock.IsAlive()); + querySetMock.DestroyApiObject(); + EXPECT_FALSE(querySetMock.IsAlive()); + } + + // If the reference count on API objects reach 0, they should delete themselves. Note that GTest + // will also complain if there is a memory leak. + TEST_F(DestroyObjectTests, QuerySetImplicit) { + QuerySetMock* querySetMock = new QuerySetMock(&mDevice); + EXPECT_CALL(*querySetMock, DestroyApiObjectImpl).Times(1); + { + QuerySetDescriptor desc = {}; + Ref querySet; + EXPECT_CALL(mDevice, CreateQuerySetImpl) + .WillOnce(Return(ByMove(AcquireRef(querySetMock)))); + DAWN_ASSERT_AND_ASSIGN(querySet, mDevice.CreateQuerySet(&desc)); + + EXPECT_TRUE(querySet->IsAlive()); + } + } + TEST_F(DestroyObjectTests, RenderPipelineExplicit) { RenderPipelineMock renderPipelineMock(&mDevice); EXPECT_CALL(renderPipelineMock, DestroyApiObjectImpl).Times(1); @@ -329,8 +414,10 @@ namespace dawn_native { namespace { TEST_F(DestroyObjectTests, DestroyObjects) { BindGroupMock* bindGroupMock = new BindGroupMock(&mDevice); BindGroupLayoutMock* bindGroupLayoutMock = new BindGroupLayoutMock(&mDevice); + BufferMock* bufferMock = new BufferMock(&mDevice, BufferBase::BufferState::Unmapped); ComputePipelineMock* computePipelineMock = new ComputePipelineMock(&mDevice); PipelineLayoutMock* pipelineLayoutMock = new PipelineLayoutMock(&mDevice); + QuerySetMock* querySetMock = new QuerySetMock(&mDevice); RenderPipelineMock* renderPipelineMock = new RenderPipelineMock(&mDevice); SamplerMock* samplerMock = new SamplerMock(&mDevice); ShaderModuleMock* shaderModuleMock = new ShaderModuleMock(&mDevice); @@ -344,7 +431,9 @@ namespace dawn_native { namespace { EXPECT_CALL(*bindGroupMock, DestroyApiObjectImpl).Times(1); EXPECT_CALL(*bindGroupLayoutMock, DestroyApiObjectImpl).Times(1); EXPECT_CALL(*shaderModuleMock, DestroyApiObjectImpl).Times(1); + EXPECT_CALL(*querySetMock, DestroyApiObjectImpl).Times(1); EXPECT_CALL(*samplerMock, DestroyApiObjectImpl).Times(1); + EXPECT_CALL(*bufferMock, DestroyApiObjectImpl).Times(1); } Ref bindGroup; @@ -366,6 +455,14 @@ namespace dawn_native { namespace { EXPECT_TRUE(bindGroupLayout->IsCachedReference()); } + Ref buffer; + { + BufferDescriptor desc = {}; + EXPECT_CALL(mDevice, CreateBufferImpl).WillOnce(Return(ByMove(AcquireRef(bufferMock)))); + DAWN_ASSERT_AND_ASSIGN(buffer, mDevice.CreateBuffer(&desc)); + EXPECT_TRUE(buffer->IsAlive()); + } + Ref computePipeline; { // Compute pipelines usually set their hash values at construction, but the mock does @@ -397,6 +494,15 @@ namespace dawn_native { namespace { EXPECT_TRUE(pipelineLayout->IsCachedReference()); } + Ref querySet; + { + QuerySetDescriptor desc = {}; + EXPECT_CALL(mDevice, CreateQuerySetImpl) + .WillOnce(Return(ByMove(AcquireRef(querySetMock)))); + DAWN_ASSERT_AND_ASSIGN(querySet, mDevice.CreateQuerySet(&desc)); + EXPECT_TRUE(querySet->IsAlive()); + } + Ref renderPipeline; { // Render pipelines usually set their hash values at construction, but the mock does @@ -457,8 +563,10 @@ namespace dawn_native { namespace { mDevice.DestroyObjects(); EXPECT_FALSE(bindGroup->IsAlive()); EXPECT_FALSE(bindGroupLayout->IsAlive()); + EXPECT_FALSE(buffer->IsAlive()); EXPECT_FALSE(computePipeline->IsAlive()); EXPECT_FALSE(pipelineLayout->IsAlive()); + EXPECT_FALSE(querySet->IsAlive()); EXPECT_FALSE(renderPipeline->IsAlive()); EXPECT_FALSE(sampler->IsAlive()); EXPECT_FALSE(shaderModule->IsAlive()); diff --git a/src/tests/unittests/native/mocks/BufferMock.h b/src/tests/unittests/native/mocks/BufferMock.h new file mode 100644 index 0000000000..4bf1ba7c45 --- /dev/null +++ b/src/tests/unittests/native/mocks/BufferMock.h @@ -0,0 +1,46 @@ +// Copyright 2021 The Dawn Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef TESTS_UNITTESTS_NATIVE_MOCKS_BUFFER_MOCK_H_ +#define TESTS_UNITTESTS_NATIVE_MOCKS_BUFFER_MOCK_H_ + +#include "dawn_native/Buffer.h" +#include "dawn_native/Device.h" + +#include + +namespace dawn_native { + + class BufferMock : public BufferBase { + public: + BufferMock(DeviceBase* device, BufferBase::BufferState state) : BufferBase(device, state) { + } + ~BufferMock() override = default; + + MOCK_METHOD(void, DestroyApiObjectImpl, (), (override)); + + MOCK_METHOD(MaybeError, MapAtCreationImpl, (), (override)); + MOCK_METHOD(MaybeError, + MapAsyncImpl, + (wgpu::MapMode mode, size_t offset, size_t size), + (override)); + MOCK_METHOD(void, UnmapImpl, (), (override)); + MOCK_METHOD(void*, GetMappedPointerImpl, (), (override)); + + MOCK_METHOD(bool, IsCPUWritableAtCreation, (), (const, override)); + }; + +} // namespace dawn_native + +#endif // TESTS_UNITTESTS_NATIVE_MOCKS_BINDGROUP_MOCK_H_ diff --git a/src/tests/unittests/native/mocks/QuerySetMock.h b/src/tests/unittests/native/mocks/QuerySetMock.h new file mode 100644 index 0000000000..56f2298369 --- /dev/null +++ b/src/tests/unittests/native/mocks/QuerySetMock.h @@ -0,0 +1,36 @@ +// Copyright 2021 The Dawn Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef TESTS_UNITTESTS_NATIVE_MOCKS_QUERYSET_MOCK_H_ +#define TESTS_UNITTESTS_NATIVE_MOCKS_QUERYSET_MOCK_H_ + +#include "dawn_native/Device.h" +#include "dawn_native/QuerySet.h" + +#include + +namespace dawn_native { + + class QuerySetMock : public QuerySetBase { + public: + QuerySetMock(DeviceBase* device) : QuerySetBase(device) { + } + ~QuerySetMock() override = default; + + MOCK_METHOD(void, DestroyApiObjectImpl, (), (override)); + }; + +} // namespace dawn_native + +#endif // TESTS_UNITTESTS_NATIVE_MOCKS_QUERYSET_MOCK_H_ diff --git a/src/tests/unittests/validation/BufferValidationTests.cpp b/src/tests/unittests/validation/BufferValidationTests.cpp index bf9050bc5f..ff5b00afc9 100644 --- a/src/tests/unittests/validation/BufferValidationTests.cpp +++ b/src/tests/unittests/validation/BufferValidationTests.cpp @@ -467,7 +467,18 @@ TEST_F(BufferValidationTest, MappedAtCreationSizeAlignment) { ASSERT_DEVICE_ERROR(BufferMappedAtCreation(2, wgpu::BufferUsage::MapWrite)); } -// Test that it is valid to destroy an unmapped buffer +// Test that it is valid to destroy an error buffer +TEST_F(BufferValidationTest, DestroyErrorBuffer) { + wgpu::BufferDescriptor desc; + desc.size = 4; + desc.usage = wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite; + wgpu::Buffer buf; + ASSERT_DEVICE_ERROR(buf = device.CreateBuffer(&desc)); + + buf.Destroy(); +} + +// Test that it is valid to Destroy an unmapped buffer TEST_F(BufferValidationTest, DestroyUnmappedBuffer) { { wgpu::Buffer buf = CreateMapReadBuffer(4); @@ -486,6 +497,17 @@ TEST_F(BufferValidationTest, DestroyDestroyedBuffer) { buf.Destroy(); } +// Test that it is invalid to Unmap an error buffer +TEST_F(BufferValidationTest, UnmapErrorBuffer) { + wgpu::BufferDescriptor desc; + desc.size = 4; + desc.usage = wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite; + wgpu::Buffer buf; + ASSERT_DEVICE_ERROR(buf = device.CreateBuffer(&desc)); + + ASSERT_DEVICE_ERROR(buf.Unmap()); +} + // Test that it is invalid to Unmap a destroyed buffer TEST_F(BufferValidationTest, UnmapDestroyedBuffer) { {