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 <enga@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Loko Kung <lokokung@google.com>
diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp
index abb7f1b..70343e2 100644
--- a/src/dawn_native/Buffer.cpp
+++ b/src/dawn_native/Buffer.cpp
@@ -64,13 +64,12 @@
mFakeMappedData =
std::unique_ptr<uint8_t[]>(AllocNoThrow<uint8_t>(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 @@
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<uint8_t[]> mFakeMappedData;
@@ -153,6 +155,8 @@
if ((mUsage & wgpu::BufferUsage::Indirect) && device->IsValidationEnabled()) {
mUsage |= kInternalStorageBuffer;
}
+
+ TrackInDevice();
}
BufferBase::BufferBase(DeviceBase* device,
@@ -166,11 +170,34 @@
}
}
+ 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 @@
}
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<ErrorBuffer*>(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 @@
}
void BufferBase::APIUnmap() {
+ if (GetDevice()->ConsumedError(ValidateUnmap(), "calling %s.Unmap()", this)) {
+ return;
+ }
Unmap();
}
@@ -417,17 +425,6 @@
}
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<ErrorBuffer*>(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 @@
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 @@
MaybeError BufferBase::ValidateUnmap() const {
DAWN_TRY(GetDevice()->ValidateIsAlive());
- DAWN_TRY(GetDevice()->ValidateObject(this));
switch (mState) {
case BufferState::Mapped:
@@ -559,18 +553,6 @@
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 6ba5755..cc7e32e 100644
--- a/src/dawn_native/Buffer.h
+++ b/src/dawn_native/Buffer.h
@@ -41,18 +41,18 @@
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 @@
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 @@
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 @@
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 cf33c02..f09e671 100644
--- a/src/dawn_native/Device.cpp
+++ b/src/dawn_native/Device.cpp
@@ -268,7 +268,7 @@
// TODO(dawn/628) Add types into the array as they are implemented.
// clang-format off
- static constexpr std::array<ObjectType, 8> kObjectTypeDependencyOrder = {
+ static constexpr std::array<ObjectType, 10> kObjectTypeDependencyOrder = {
ObjectType::RenderPipeline,
ObjectType::ComputePipeline,
ObjectType::PipelineLayout,
@@ -276,7 +276,9 @@
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 2a1d495..971c048 100644
--- a/src/dawn_native/ObjectBase.cpp
+++ b/src/dawn_native/ObjectBase.cpp
@@ -80,15 +80,17 @@
GetDevice()->TrackObject(this);
}
+ bool ApiObjectBase::MarkDestroyed() {
+ const std::lock_guard<std::mutex> lock(*GetDevice()->GetObjectListMutex(GetType()));
+ return RemoveFromList();
+ }
+
bool ApiObjectBase::DestroyApiObject() {
- {
- const std::lock_guard<std::mutex> 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 291e806..0059506 100644
--- a/src/dawn_native/ObjectBase.h
+++ b/src/dawn_native/ObjectBase.h
@@ -83,6 +83,12 @@
// 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 2ad7cd7..855277d 100644
--- a/src/dawn_native/QuerySet.cpp
+++ b/src/dawn_native/QuerySet.cpp
@@ -31,7 +31,7 @@
}
private:
- void DestroyImpl() override {
+ void DestroyApiObjectImpl() override {
UNREACHABLE();
}
};
@@ -110,6 +110,11 @@
}
mQueryAvailability.resize(descriptor->count);
+ TrackInDevice();
+ }
+
+ QuerySetBase::QuerySetBase(DeviceBase* device) : ApiObjectBase(device, kLabelNotImplemented) {
+ TrackInDevice();
}
QuerySetBase::QuerySetBase(DeviceBase* device, ObjectBase::ErrorTag tag)
@@ -121,6 +126,11 @@
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 @@
if (GetDevice()->ConsumedError(ValidateDestroy())) {
return;
}
- DestroyInternal();
+ DestroyApiObject();
}
MaybeError QuerySetBase::ValidateDestroy() const {
@@ -168,11 +178,4 @@
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 3ad4e7c..d2a0263 100644
--- a/src/dawn_native/QuerySet.h
+++ b/src/dawn_native/QuerySet.h
@@ -31,6 +31,7 @@
static QuerySetBase* MakeError(DeviceBase* device);
+ bool DestroyApiObject() override;
ObjectType GetType() const override;
wgpu::QueryType GetQueryType() const;
@@ -46,13 +47,13 @@
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 39fcb83..e157b41 100644
--- a/src/dawn_native/d3d12/BufferD3D12.cpp
+++ b/src/dawn_native/d3d12/BufferD3D12.cpp
@@ -185,9 +185,7 @@
return {};
}
- Buffer::~Buffer() {
- DestroyInternal();
- }
+ Buffer::~Buffer() = default;
ID3D12Resource* Buffer::GetD3D12Resource() const {
return mResourceAllocation.GetD3D12Resource();
@@ -380,7 +378,7 @@
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 d6fcbbd..eb9191f 100644
--- a/src/dawn_native/d3d12/BufferD3D12.h
+++ b/src/dawn_native/d3d12/BufferD3D12.h
@@ -58,7 +58,7 @@
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 4caf9f6..f766933 100644
--- a/src/dawn_native/d3d12/QuerySetD3D12.cpp
+++ b/src/dawn_native/d3d12/QuerySetD3D12.cpp
@@ -55,12 +55,11 @@
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 16c49d1..dc2c157 100644
--- a/src/dawn_native/d3d12/QuerySetD3D12.h
+++ b/src/dawn_native/d3d12/QuerySetD3D12.h
@@ -35,7 +35,7 @@
MaybeError Initialize();
// Dawn API
- void DestroyImpl() override;
+ void DestroyApiObjectImpl() override;
ComPtr<ID3D12QueryHeap> mQueryHeap;
};
diff --git a/src/dawn_native/metal/BufferMTL.h b/src/dawn_native/metal/BufferMTL.h
index cf6dc6f..13e89b9 100644
--- a/src/dawn_native/metal/BufferMTL.h
+++ b/src/dawn_native/metal/BufferMTL.h
@@ -26,7 +26,7 @@
class CommandRecordingContext;
class Device;
- class Buffer : public BufferBase {
+ class Buffer final : public BufferBase {
public:
static ResultOrError<Ref<Buffer>> Create(Device* device,
const BufferDescriptor* descriptor);
@@ -48,7 +48,7 @@
~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 2b9c2fe..7ac489c 100644
--- a/src/dawn_native/metal/BufferMTL.mm
+++ b/src/dawn_native/metal/BufferMTL.mm
@@ -140,9 +140,7 @@
return {};
}
- Buffer::~Buffer() {
- DestroyInternal();
- }
+ Buffer::~Buffer() = default;
id<MTLBuffer> Buffer::GetMTLBuffer() const {
return mMtlBuffer.Get();
@@ -173,7 +171,7 @@
// 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 a7b1ad7..b77bd31 100644
--- a/src/dawn_native/metal/QuerySetMTL.h
+++ b/src/dawn_native/metal/QuerySetMTL.h
@@ -40,7 +40,7 @@
MaybeError Initialize();
// Dawn API
- void DestroyImpl() override;
+ void DestroyApiObjectImpl() override;
NSPRef<id<MTLBuffer>> 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 dc06045..9dea304 100644
--- a/src/dawn_native/metal/QuerySetMTL.mm
+++ b/src/dawn_native/metal/QuerySetMTL.mm
@@ -121,11 +121,9 @@
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 6b785a1..c07b0ca 100644
--- a/src/dawn_native/null/DeviceNull.cpp
+++ b/src/dawn_native/null/DeviceNull.cpp
@@ -294,11 +294,6 @@
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 @@
void Buffer::UnmapImpl() {
}
- void Buffer::DestroyImpl() {
+ void Buffer::DestroyApiObjectImpl() {
+ ToBackend(GetDevice())->DecrementMemoryUsage(GetSize());
}
// CommandBuffer
@@ -349,11 +345,7 @@
: 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 cef718d..010d9d1 100644
--- a/src/dawn_native/null/DeviceNull.h
+++ b/src/dawn_native/null/DeviceNull.h
@@ -226,10 +226,9 @@
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 @@
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 1e102d6..f03ce49 100644
--- a/src/dawn_native/opengl/BufferGL.cpp
+++ b/src/dawn_native/opengl/BufferGL.cpp
@@ -61,9 +61,7 @@
}
}
- Buffer::~Buffer() {
- DestroyInternal();
- }
+ Buffer::~Buffer() = default;
GLuint Buffer::GetHandle() const {
return mBuffer;
@@ -176,7 +174,7 @@
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 2259161..fdba81b 100644
--- a/src/dawn_native/opengl/BufferGL.h
+++ b/src/dawn_native/opengl/BufferGL.h
@@ -42,7 +42,7 @@
~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 6ff5d20..6ec9041 100644
--- a/src/dawn_native/opengl/QuerySetGL.cpp
+++ b/src/dawn_native/opengl/QuerySetGL.cpp
@@ -22,11 +22,9 @@
: 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 2a83bdd..cd16dc8 100644
--- a/src/dawn_native/opengl/QuerySetGL.h
+++ b/src/dawn_native/opengl/QuerySetGL.h
@@ -28,7 +28,7 @@
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 75a32fd..62b6e11 100644
--- a/src/dawn_native/vulkan/BufferVk.cpp
+++ b/src/dawn_native/vulkan/BufferVk.cpp
@@ -236,9 +236,7 @@
return {};
}
- Buffer::~Buffer() {
- DestroyInternal();
- }
+ Buffer::~Buffer() = default;
VkBuffer Buffer::GetHandle() const {
return mHandle;
@@ -331,7 +329,7 @@
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 3fcab60..30703c8 100644
--- a/src/dawn_native/vulkan/BufferVk.h
+++ b/src/dawn_native/vulkan/BufferVk.h
@@ -65,7 +65,7 @@
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 ba60c3d..7a15296 100644
--- a/src/dawn_native/vulkan/QuerySetVk.cpp
+++ b/src/dawn_native/vulkan/QuerySetVk.cpp
@@ -94,11 +94,9 @@
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 80e7bef..03b0a7b 100644
--- a/src/dawn_native/vulkan/QuerySetVk.h
+++ b/src/dawn_native/vulkan/QuerySetVk.h
@@ -35,7 +35,7 @@
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 8ffd921..1fb6de0 100644
--- a/src/tests/BUILD.gn
+++ b/src/tests/BUILD.gn
@@ -147,6 +147,7 @@
"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 a82a7f4..e4dee5e 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 @@
}
}
+ 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<BufferBase> 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<BufferBase> 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 @@
}
}
+ 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<QuerySetBase> 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 @@
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 @@
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<BindGroupBase> bindGroup;
@@ -366,6 +455,14 @@
EXPECT_TRUE(bindGroupLayout->IsCachedReference());
}
+ Ref<BufferBase> 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<ComputePipelineBase> computePipeline;
{
// Compute pipelines usually set their hash values at construction, but the mock does
@@ -397,6 +494,15 @@
EXPECT_TRUE(pipelineLayout->IsCachedReference());
}
+ Ref<QuerySetBase> 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<RenderPipelineBase> renderPipeline;
{
// Render pipelines usually set their hash values at construction, but the mock does
@@ -457,8 +563,10 @@
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 0000000..4bf1ba7
--- /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 <gmock/gmock.h>
+
+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 0000000..56f2298
--- /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 <gmock/gmock.h>
+
+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 bf9050b..ff5b00a 100644
--- a/src/tests/unittests/validation/BufferValidationTests.cpp
+++ b/src/tests/unittests/validation/BufferValidationTests.cpp
@@ -467,7 +467,18 @@
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 @@
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) {
{
