From ff9a1f7b20d7ff9a56714d6d0000b5f892b65544 Mon Sep 17 00:00:00 2001 From: Loko Kung Date: Mon, 1 Nov 2021 23:42:52 +0000 Subject: [PATCH] Adds/refactors destroy handling for Buffer and QuerySet. The changes should pass through the destroy changes such that when the device is destroyed, the respective destroy functionality currently existing in the backends should be called. For buffers, destroy no longer causes validation errors since even error buffers may need to be destroyed in the case of mappedAtCreation. Bug: dawn:628, dawn:1002 Change-Id: I42a475af5d67cc60f86d95ac53c2b377a9fd2e82 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/65863 Reviewed-by: Austin Eng Reviewed-by: Corentin Wallez Commit-Queue: Loko Kung --- src/dawn_native/Buffer.cpp | 100 +++++++--------- src/dawn_native/Buffer.h | 12 +- src/dawn_native/Device.cpp | 4 +- src/dawn_native/ObjectBase.cpp | 16 +-- src/dawn_native/ObjectBase.h | 6 + src/dawn_native/QuerySet.cpp | 21 ++-- src/dawn_native/QuerySet.h | 9 +- src/dawn_native/d3d12/BufferD3D12.cpp | 6 +- src/dawn_native/d3d12/BufferD3D12.h | 2 +- src/dawn_native/d3d12/QuerySetD3D12.cpp | 7 +- src/dawn_native/d3d12/QuerySetD3D12.h | 2 +- src/dawn_native/metal/BufferMTL.h | 4 +- src/dawn_native/metal/BufferMTL.mm | 6 +- src/dawn_native/metal/QuerySetMTL.h | 2 +- src/dawn_native/metal/QuerySetMTL.mm | 6 +- src/dawn_native/null/DeviceNull.cpp | 14 +-- src/dawn_native/null/DeviceNull.h | 7 +- src/dawn_native/opengl/BufferGL.cpp | 6 +- src/dawn_native/opengl/BufferGL.h | 2 +- src/dawn_native/opengl/QuerySetGL.cpp | 6 +- src/dawn_native/opengl/QuerySetGL.h | 2 +- src/dawn_native/vulkan/BufferVk.cpp | 6 +- src/dawn_native/vulkan/BufferVk.h | 2 +- src/dawn_native/vulkan/QuerySetVk.cpp | 6 +- src/dawn_native/vulkan/QuerySetVk.h | 2 +- src/tests/BUILD.gn | 1 + .../unittests/native/DestroyObjectTests.cpp | 108 ++++++++++++++++++ src/tests/unittests/native/mocks/BufferMock.h | 46 ++++++++ .../unittests/native/mocks/QuerySetMock.h | 36 ++++++ .../validation/BufferValidationTests.cpp | 24 +++- 30 files changed, 327 insertions(+), 144 deletions(-) create mode 100644 src/tests/unittests/native/mocks/BufferMock.h create mode 100644 src/tests/unittests/native/mocks/QuerySetMock.h 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) { {