From 76d9e34bbcb2353b9644add8e6bbbe61faee41c7 Mon Sep 17 00:00:00 2001 From: Enrico Galli Date: Thu, 13 Aug 2020 20:25:39 +0000 Subject: [PATCH] Eagerly destroy CommandBuffer commands after submission Command buffers hold references to all encoded objects. Freeing them eagerly significantly reduces the amount memory held before the JS GC clears the command buffers. Bug: dawn:262, dawn:372 Change-Id: I68dfa973f980fba8d94611ed1de3c593bdb91a63 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/26562 Reviewed-by: Corentin Wallez Reviewed-by: Rafael Cintron Commit-Queue: Austin Eng --- src/dawn_native/CommandAllocator.cpp | 33 ++++----- src/dawn_native/CommandAllocator.h | 8 +- src/dawn_native/CommandBuffer.cpp | 23 +++++- src/dawn_native/CommandBuffer.h | 12 +++ src/dawn_native/Commands.cpp | 3 +- src/dawn_native/Queue.cpp | 42 +++++++---- src/dawn_native/Queue.h | 2 + src/dawn_native/d3d12/CommandBufferD3D12.cpp | 6 +- src/dawn_native/d3d12/CommandBufferD3D12.h | 4 - src/dawn_native/metal/CommandBufferMTL.h | 4 - src/dawn_native/metal/CommandBufferMTL.mm | 6 +- src/dawn_native/null/DeviceNull.cpp | 6 +- src/dawn_native/null/DeviceNull.h | 4 - src/dawn_native/opengl/CommandBufferGL.cpp | 6 +- src/dawn_native/opengl/CommandBufferGL.h | 4 - src/dawn_native/vulkan/CommandBufferVk.cpp | 6 +- src/dawn_native/vulkan/CommandBufferVk.h | 4 - src/tests/unittests/CommandAllocatorTests.cpp | 28 +++---- src/tests/unittests/ToBackendTests.cpp | 57 +++++++------- .../validation/QueueSubmitValidationTests.cpp | 74 ++++++++++++++++++- 20 files changed, 203 insertions(+), 129 deletions(-) diff --git a/src/dawn_native/CommandAllocator.cpp b/src/dawn_native/CommandAllocator.cpp index 553f889657..7c94520e07 100644 --- a/src/dawn_native/CommandAllocator.cpp +++ b/src/dawn_native/CommandAllocator.cpp @@ -30,13 +30,7 @@ namespace dawn_native { } CommandIterator::~CommandIterator() { - ASSERT(mDataWasDestroyed); - - if (!IsEmpty()) { - for (auto& block : mBlocks) { - free(block.block); - } - } + ASSERT(IsEmpty()); } CommandIterator::CommandIterator(CommandIterator&& other) { @@ -44,18 +38,13 @@ namespace dawn_native { mBlocks = std::move(other.mBlocks); other.Reset(); } - other.DataWasDestroyed(); Reset(); } CommandIterator& CommandIterator::operator=(CommandIterator&& other) { - if (!other.IsEmpty()) { - mBlocks = std::move(other.mBlocks); - other.Reset(); - } else { - mBlocks.clear(); - } - other.DataWasDestroyed(); + ASSERT(IsEmpty()); + mBlocks = std::move(other.mBlocks); + other.Reset(); Reset(); return *this; } @@ -66,6 +55,7 @@ namespace dawn_native { } CommandIterator& CommandIterator::operator=(CommandAllocator&& allocator) { + ASSERT(IsEmpty()); mBlocks = allocator.AcquireBlocks(); Reset(); return *this; @@ -97,8 +87,17 @@ namespace dawn_native { } } - void CommandIterator::DataWasDestroyed() { - mDataWasDestroyed = true; + void CommandIterator::MakeEmptyAsDataWasDestroyed() { + if (IsEmpty()) { + return; + } + + for (auto& block : mBlocks) { + free(block.block); + } + mBlocks.clear(); + Reset(); + ASSERT(IsEmpty()); } bool CommandIterator::IsEmpty() const { diff --git a/src/dawn_native/CommandAllocator.h b/src/dawn_native/CommandAllocator.h index 82de05c1a4..0383dc1639 100644 --- a/src/dawn_native/CommandAllocator.h +++ b/src/dawn_native/CommandAllocator.h @@ -91,10 +91,13 @@ namespace dawn_native { return static_cast(NextData(sizeof(T) * count, alignof(T))); } - // Needs to be called if iteration was stopped early. + // Sets iterator to the beginning of the commands without emptying the list. This method can + // be used if iteration was stopped early and the iterator needs to be restarted. void Reset(); - void DataWasDestroyed(); + // This method must to be called after commands have been deleted. This indicates that the + // commands have been submitted and they are no longer valid. + void MakeEmptyAsDataWasDestroyed(); private: bool IsEmpty() const; @@ -139,7 +142,6 @@ namespace dawn_native { size_t mCurrentBlock = 0; // Used to avoid a special case for empty iterators. uint32_t mEndOfBlock = detail::kEndOfBlock; - bool mDataWasDestroyed = false; }; class CommandAllocator { diff --git a/src/dawn_native/CommandBuffer.cpp b/src/dawn_native/CommandBuffer.cpp index 57ff4ecce4..91fec29f18 100644 --- a/src/dawn_native/CommandBuffer.cpp +++ b/src/dawn_native/CommandBuffer.cpp @@ -24,18 +24,39 @@ namespace dawn_native { CommandBufferBase::CommandBufferBase(CommandEncoder* encoder, const CommandBufferDescriptor*) - : ObjectBase(encoder->GetDevice()), mResourceUsages(encoder->AcquireResourceUsages()) { + : ObjectBase(encoder->GetDevice()), + mCommands(encoder->AcquireCommands()), + mResourceUsages(encoder->AcquireResourceUsages()) { } CommandBufferBase::CommandBufferBase(DeviceBase* device, ObjectBase::ErrorTag tag) : ObjectBase(device, tag) { } + CommandBufferBase::~CommandBufferBase() { + Destroy(); + } + // static CommandBufferBase* CommandBufferBase::MakeError(DeviceBase* device) { return new CommandBufferBase(device, ObjectBase::kError); } + MaybeError CommandBufferBase::ValidateCanUseInSubmitNow() const { + ASSERT(!IsError()); + + if (mDestroyed) { + return DAWN_VALIDATION_ERROR("Command buffer reused in submit"); + } + return {}; + } + + void CommandBufferBase::Destroy() { + FreeCommands(&mCommands); + mResourceUsages = {}; + mDestroyed = true; + } + const CommandBufferResourceUsage& CommandBufferBase::GetResourceUsages() const { return mResourceUsages; } diff --git a/src/dawn_native/CommandBuffer.h b/src/dawn_native/CommandBuffer.h index 11076a16b4..54a8f7649f 100644 --- a/src/dawn_native/CommandBuffer.h +++ b/src/dawn_native/CommandBuffer.h @@ -17,6 +17,8 @@ #include "dawn_native/dawn_platform.h" +#include "dawn_native/CommandAllocator.h" +#include "dawn_native/Error.h" #include "dawn_native/Forward.h" #include "dawn_native/ObjectBase.h" #include "dawn_native/PassResourceUsage.h" @@ -31,14 +33,24 @@ namespace dawn_native { class CommandBufferBase : public ObjectBase { public: CommandBufferBase(CommandEncoder* encoder, const CommandBufferDescriptor* descriptor); + static CommandBufferBase* MakeError(DeviceBase* device); + MaybeError ValidateCanUseInSubmitNow() const; + void Destroy(); + const CommandBufferResourceUsage& GetResourceUsages() const; + protected: + ~CommandBufferBase(); + + CommandIterator mCommands; + private: CommandBufferBase(DeviceBase* device, ObjectBase::ErrorTag tag); CommandBufferResourceUsage mResourceUsages; + bool mDestroyed = false; }; bool IsCompleteSubresourceCopiedTo(const TextureBase* texture, diff --git a/src/dawn_native/Commands.cpp b/src/dawn_native/Commands.cpp index 9528e82ed8..621a7434ab 100644 --- a/src/dawn_native/Commands.cpp +++ b/src/dawn_native/Commands.cpp @@ -188,7 +188,8 @@ namespace dawn_native { } } } - commands->DataWasDestroyed(); + + commands->MakeEmptyAsDataWasDestroyed(); } void SkipCommand(CommandIterator* commands, Command type) { diff --git a/src/dawn_native/Queue.cpp b/src/dawn_native/Queue.cpp index 027ee58e5d..bf7a0fde50 100644 --- a/src/dawn_native/Queue.cpp +++ b/src/dawn_native/Queue.cpp @@ -51,24 +51,11 @@ namespace dawn_native { } void QueueBase::Submit(uint32_t commandCount, CommandBufferBase* const* commands) { - DeviceBase* device = GetDevice(); - if (device->ConsumedError(device->ValidateIsAlive())) { - // If device is lost, don't let any commands be submitted - return; - } + SubmitInternal(commandCount, commands); - TRACE_EVENT0(device->GetPlatform(), General, "Queue::Submit"); - if (device->IsValidationEnabled() && - device->ConsumedError(ValidateSubmit(commandCount, commands))) { - return; + for (uint32_t i = 0; i < commandCount; ++i) { + commands[i]->Destroy(); } - ASSERT(!IsError()); - - if (device->ConsumedError(SubmitImpl(commandCount, commands))) { - return; - } - device->GetErrorScopeTracker()->TrackUntilLastSubmitComplete( - device->GetCurrentErrorScope()); } void QueueBase::Signal(Fence* fence, uint64_t signalValue) { @@ -170,6 +157,7 @@ namespace dawn_native { for (uint32_t i = 0; i < commandCount; ++i) { DAWN_TRY(GetDevice()->ValidateObject(commands[i])); + DAWN_TRY(commands[i]->ValidateCanUseInSubmitNow()); const CommandBufferResourceUsage& usages = commands[i]->GetResourceUsages(); @@ -286,6 +274,28 @@ namespace dawn_native { return {}; } + void QueueBase::SubmitInternal(uint32_t commandCount, CommandBufferBase* const* commands) { + DeviceBase* device = GetDevice(); + if (device->ConsumedError(device->ValidateIsAlive())) { + // If device is lost, don't let any commands be submitted + return; + } + + TRACE_EVENT0(device->GetPlatform(), General, "Queue::Submit"); + if (device->IsValidationEnabled() && + device->ConsumedError(ValidateSubmit(commandCount, commands))) { + return; + } + ASSERT(!IsError()); + + if (device->ConsumedError(SubmitImpl(commandCount, commands))) { + return; + } + + device->GetErrorScopeTracker()->TrackUntilLastSubmitComplete( + device->GetCurrentErrorScope()); + } + void CopyTextureData(uint8_t* dstPointer, const uint8_t* srcPointer, uint32_t depth, diff --git a/src/dawn_native/Queue.h b/src/dawn_native/Queue.h index 441fa1b9b1..be1ce62b57 100644 --- a/src/dawn_native/Queue.h +++ b/src/dawn_native/Queue.h @@ -73,6 +73,8 @@ namespace dawn_native { size_t dataSize, const TextureDataLayout* dataLayout, const Extent3D* writeSize) const; + + void SubmitInternal(uint32_t commandCount, CommandBufferBase* const* commands); }; // A helper function used in Queue::WriteTexture. The destination data layout must not diff --git a/src/dawn_native/d3d12/CommandBufferD3D12.cpp b/src/dawn_native/d3d12/CommandBufferD3D12.cpp index 684a5e965b..8ca8fc2716 100644 --- a/src/dawn_native/d3d12/CommandBufferD3D12.cpp +++ b/src/dawn_native/d3d12/CommandBufferD3D12.cpp @@ -535,11 +535,7 @@ namespace dawn_native { namespace d3d12 { } // anonymous namespace CommandBuffer::CommandBuffer(CommandEncoder* encoder, const CommandBufferDescriptor* descriptor) - : CommandBufferBase(encoder, descriptor), mCommands(encoder->AcquireCommands()) { - } - - CommandBuffer::~CommandBuffer() { - FreeCommands(&mCommands); + : CommandBufferBase(encoder, descriptor) { } MaybeError CommandBuffer::RecordCommands(CommandRecordingContext* commandContext) { diff --git a/src/dawn_native/d3d12/CommandBufferD3D12.h b/src/dawn_native/d3d12/CommandBufferD3D12.h index cc53fd54cf..f4d858c858 100644 --- a/src/dawn_native/d3d12/CommandBufferD3D12.h +++ b/src/dawn_native/d3d12/CommandBufferD3D12.h @@ -16,7 +16,6 @@ #define DAWNNATIVE_D3D12_COMMANDBUFFERD3D12_H_ #include "common/Constants.h" -#include "dawn_native/CommandAllocator.h" #include "dawn_native/CommandBuffer.h" #include "dawn_native/Error.h" #include "dawn_native/d3d12/Forward.h" @@ -43,7 +42,6 @@ namespace dawn_native { namespace d3d12 { MaybeError RecordCommands(CommandRecordingContext* commandContext); private: - ~CommandBuffer() override; MaybeError RecordComputePass(CommandRecordingContext* commandContext, BindGroupStateTracker* bindingTracker); MaybeError RecordRenderPass(CommandRecordingContext* commandContext, @@ -55,8 +53,6 @@ namespace dawn_native { namespace d3d12 { RenderPassBuilder* renderPassBuilder); void EmulateBeginRenderPass(CommandRecordingContext* commandContext, const RenderPassBuilder* renderPassBuilder) const; - - CommandIterator mCommands; }; }} // namespace dawn_native::d3d12 diff --git a/src/dawn_native/metal/CommandBufferMTL.h b/src/dawn_native/metal/CommandBufferMTL.h index cbaf037186..3410e57954 100644 --- a/src/dawn_native/metal/CommandBufferMTL.h +++ b/src/dawn_native/metal/CommandBufferMTL.h @@ -15,7 +15,6 @@ #ifndef DAWNNATIVE_METAL_COMMANDBUFFERMTL_H_ #define DAWNNATIVE_METAL_COMMANDBUFFERMTL_H_ -#include "dawn_native/CommandAllocator.h" #include "dawn_native/CommandBuffer.h" #include "dawn_native/Error.h" @@ -37,7 +36,6 @@ namespace dawn_native { namespace metal { MaybeError FillCommands(CommandRecordingContext* commandContext); private: - ~CommandBuffer() override; MaybeError EncodeComputePass(CommandRecordingContext* commandContext); MaybeError EncodeRenderPass(CommandRecordingContext* commandContext, MTLRenderPassDescriptor* mtlRenderPass, @@ -48,8 +46,6 @@ namespace dawn_native { namespace metal { MTLRenderPassDescriptor* mtlRenderPass, uint32_t width, uint32_t height); - - CommandIterator mCommands; }; }} // namespace dawn_native::metal diff --git a/src/dawn_native/metal/CommandBufferMTL.mm b/src/dawn_native/metal/CommandBufferMTL.mm index f35448a851..7390b92c17 100644 --- a/src/dawn_native/metal/CommandBufferMTL.mm +++ b/src/dawn_native/metal/CommandBufferMTL.mm @@ -524,11 +524,7 @@ namespace dawn_native { namespace metal { } // anonymous namespace CommandBuffer::CommandBuffer(CommandEncoder* encoder, const CommandBufferDescriptor* descriptor) - : CommandBufferBase(encoder, descriptor), mCommands(encoder->AcquireCommands()) { - } - - CommandBuffer::~CommandBuffer() { - FreeCommands(&mCommands); + : CommandBufferBase(encoder, descriptor) { } MaybeError CommandBuffer::FillCommands(CommandRecordingContext* commandContext) { diff --git a/src/dawn_native/null/DeviceNull.cpp b/src/dawn_native/null/DeviceNull.cpp index 9b04aeee80..d382bb8874 100644 --- a/src/dawn_native/null/DeviceNull.cpp +++ b/src/dawn_native/null/DeviceNull.cpp @@ -329,11 +329,7 @@ namespace dawn_native { namespace null { // CommandBuffer CommandBuffer::CommandBuffer(CommandEncoder* encoder, const CommandBufferDescriptor* descriptor) - : CommandBufferBase(encoder, descriptor), mCommands(encoder->AcquireCommands()) { - } - - CommandBuffer::~CommandBuffer() { - FreeCommands(&mCommands); + : CommandBufferBase(encoder, descriptor) { } // QuerySet diff --git a/src/dawn_native/null/DeviceNull.h b/src/dawn_native/null/DeviceNull.h index 9f39cba205..b2dccfe918 100644 --- a/src/dawn_native/null/DeviceNull.h +++ b/src/dawn_native/null/DeviceNull.h @@ -216,10 +216,6 @@ namespace dawn_native { namespace null { public: CommandBuffer(CommandEncoder* encoder, const CommandBufferDescriptor* descriptor); - private: - ~CommandBuffer() override; - - CommandIterator mCommands; }; class QuerySet final : public QuerySetBase { diff --git a/src/dawn_native/opengl/CommandBufferGL.cpp b/src/dawn_native/opengl/CommandBufferGL.cpp index 695e9396ff..c88aef412c 100644 --- a/src/dawn_native/opengl/CommandBufferGL.cpp +++ b/src/dawn_native/opengl/CommandBufferGL.cpp @@ -442,11 +442,7 @@ namespace dawn_native { namespace opengl { } // namespace CommandBuffer::CommandBuffer(CommandEncoder* encoder, const CommandBufferDescriptor* descriptor) - : CommandBufferBase(encoder, descriptor), mCommands(encoder->AcquireCommands()) { - } - - CommandBuffer::~CommandBuffer() { - FreeCommands(&mCommands); + : CommandBufferBase(encoder, descriptor) { } void CommandBuffer::Execute() { diff --git a/src/dawn_native/opengl/CommandBufferGL.h b/src/dawn_native/opengl/CommandBufferGL.h index 482f10d09d..eceb5335e9 100644 --- a/src/dawn_native/opengl/CommandBufferGL.h +++ b/src/dawn_native/opengl/CommandBufferGL.h @@ -15,7 +15,6 @@ #ifndef DAWNNATIVE_OPENGL_COMMANDBUFFERGL_H_ #define DAWNNATIVE_OPENGL_COMMANDBUFFERGL_H_ -#include "dawn_native/CommandAllocator.h" #include "dawn_native/CommandBuffer.h" namespace dawn_native { @@ -33,11 +32,8 @@ namespace dawn_native { namespace opengl { void Execute(); private: - ~CommandBuffer() override; void ExecuteComputePass(); void ExecuteRenderPass(BeginRenderPassCmd* renderPass); - - CommandIterator mCommands; }; }} // namespace dawn_native::opengl diff --git a/src/dawn_native/vulkan/CommandBufferVk.cpp b/src/dawn_native/vulkan/CommandBufferVk.cpp index 27f92b716e..ce86612b7e 100644 --- a/src/dawn_native/vulkan/CommandBufferVk.cpp +++ b/src/dawn_native/vulkan/CommandBufferVk.cpp @@ -330,11 +330,7 @@ namespace dawn_native { namespace vulkan { } CommandBuffer::CommandBuffer(CommandEncoder* encoder, const CommandBufferDescriptor* descriptor) - : CommandBufferBase(encoder, descriptor), mCommands(encoder->AcquireCommands()) { - } - - CommandBuffer::~CommandBuffer() { - FreeCommands(&mCommands); + : CommandBufferBase(encoder, descriptor) { } void CommandBuffer::RecordCopyImageWithTemporaryBuffer( diff --git a/src/dawn_native/vulkan/CommandBufferVk.h b/src/dawn_native/vulkan/CommandBufferVk.h index fa2dd4b2d2..c5476d38ad 100644 --- a/src/dawn_native/vulkan/CommandBufferVk.h +++ b/src/dawn_native/vulkan/CommandBufferVk.h @@ -15,7 +15,6 @@ #ifndef DAWNNATIVE_VULKAN_COMMANDBUFFERVK_H_ #define DAWNNATIVE_VULKAN_COMMANDBUFFERVK_H_ -#include "dawn_native/CommandAllocator.h" #include "dawn_native/CommandBuffer.h" #include "dawn_native/Error.h" @@ -40,7 +39,6 @@ namespace dawn_native { namespace vulkan { private: CommandBuffer(CommandEncoder* encoder, const CommandBufferDescriptor* descriptor); - ~CommandBuffer() override; MaybeError RecordComputePass(CommandRecordingContext* recordingContext); MaybeError RecordRenderPass(CommandRecordingContext* recordingContext, @@ -49,8 +47,6 @@ namespace dawn_native { namespace vulkan { const TextureCopy& srcCopy, const TextureCopy& dstCopy, const Extent3D& copySize); - - CommandIterator mCommands; }; }} // namespace dawn_native::vulkan diff --git a/src/tests/unittests/CommandAllocatorTests.cpp b/src/tests/unittests/CommandAllocatorTests.cpp index bc1d736cd8..b01c3a5f00 100644 --- a/src/tests/unittests/CommandAllocatorTests.cpp +++ b/src/tests/unittests/CommandAllocatorTests.cpp @@ -63,7 +63,7 @@ TEST(CommandAllocator, DoNothingAllocator) { TEST(CommandAllocator, DoNothingAllocatorWithIterator) { CommandAllocator allocator; CommandIterator iterator(std::move(allocator)); - iterator.DataWasDestroyed(); + iterator.MakeEmptyAsDataWasDestroyed(); } // Test basic usage of allocator + iterator @@ -108,7 +108,7 @@ TEST(CommandAllocator, Basic) { hasNext = iterator.NextCommandId(&type); ASSERT_FALSE(hasNext); - iterator.DataWasDestroyed(); + iterator.MakeEmptyAsDataWasDestroyed(); } } @@ -152,7 +152,7 @@ TEST(CommandAllocator, BasicWithData) { hasNext = iterator.NextCommandId(&type); ASSERT_FALSE(hasNext); - iterator.DataWasDestroyed(); + iterator.MakeEmptyAsDataWasDestroyed(); } } @@ -197,7 +197,7 @@ TEST(CommandAllocator, MultipleIterations) { hasNext = iterator.NextCommandId(&type); ASSERT_FALSE(hasNext); - iterator.DataWasDestroyed(); + iterator.MakeEmptyAsDataWasDestroyed(); } } // Test large commands work @@ -230,7 +230,7 @@ TEST(CommandAllocator, LargeCommands) { } ASSERT_EQ(numCommands, kCommandCount); - iterator.DataWasDestroyed(); + iterator.MakeEmptyAsDataWasDestroyed(); } // Test many small commands work @@ -260,7 +260,7 @@ TEST(CommandAllocator, ManySmallCommands) { } ASSERT_EQ(numCommands, kCommandCount); - iterator.DataWasDestroyed(); + iterator.MakeEmptyAsDataWasDestroyed(); } /* ________ @@ -325,7 +325,7 @@ TEST(CommandAllocator, IteratorReset) { hasNext = iterator.NextCommandId(&type); ASSERT_FALSE(hasNext); - iterator.DataWasDestroyed(); + iterator.MakeEmptyAsDataWasDestroyed(); } } @@ -339,7 +339,7 @@ TEST(CommandAllocator, EmptyIterator) { bool hasNext = iterator.NextCommandId(&type); ASSERT_FALSE(hasNext); - iterator.DataWasDestroyed(); + iterator.MakeEmptyAsDataWasDestroyed(); } { CommandAllocator allocator; @@ -350,8 +350,8 @@ TEST(CommandAllocator, EmptyIterator) { bool hasNext = iterator2.NextCommandId(&type); ASSERT_FALSE(hasNext); - iterator1.DataWasDestroyed(); - iterator2.DataWasDestroyed(); + iterator1.MakeEmptyAsDataWasDestroyed(); + iterator2.MakeEmptyAsDataWasDestroyed(); } { CommandIterator iterator1; @@ -361,8 +361,8 @@ TEST(CommandAllocator, EmptyIterator) { bool hasNext = iterator2.NextCommandId(&type); ASSERT_FALSE(hasNext); - iterator1.DataWasDestroyed(); - iterator2.DataWasDestroyed(); + iterator1.MakeEmptyAsDataWasDestroyed(); + iterator2.MakeEmptyAsDataWasDestroyed(); } } @@ -425,7 +425,7 @@ TEST(CommandAllocator, AllocateDefaultInitializes) { ASSERT_EQ(int44->value, 44); CommandIterator iterator(std::move(allocator)); - iterator.DataWasDestroyed(); + iterator.MakeEmptyAsDataWasDestroyed(); } // Test that the allcator correctly defaults initalizes data for AllocateData @@ -445,5 +445,5 @@ TEST(CommandAllocator, AllocateDataDefaultInitializes) { ASSERT_EQ(int35[2].value, 35); CommandIterator iterator(std::move(allocator)); - iterator.DataWasDestroyed(); + iterator.MakeEmptyAsDataWasDestroyed(); } diff --git a/src/tests/unittests/ToBackendTests.cpp b/src/tests/unittests/ToBackendTests.cpp index 5234e40f18..01bbc17127 100644 --- a/src/tests/unittests/ToBackendTests.cpp +++ b/src/tests/unittests/ToBackendTests.cpp @@ -19,17 +19,17 @@ #include -// Make our own Base - Backend object pair, reusing the CommandBuffer name +// Make our own Base - Backend object pair, reusing the AdapterBase name namespace dawn_native { - class CommandBufferBase : public RefCounted {}; + class AdapterBase : public RefCounted {}; } // namespace dawn_native using namespace dawn_native; -class MyCommandBuffer : public CommandBufferBase {}; +class MyAdapter : public AdapterBase {}; struct MyBackendTraits { - using CommandBufferType = MyCommandBuffer; + using AdapterType = MyAdapter; }; // Instanciate ToBackend for our "backend" @@ -41,48 +41,47 @@ auto ToBackend(T&& common) -> decltype(ToBackendBase(common)) { // Test that ToBackend correctly converts pointers to base classes. TEST(ToBackend, Pointers) { { - MyCommandBuffer* cmdBuf = new MyCommandBuffer; - const CommandBufferBase* base = cmdBuf; + MyAdapter* adapter = new MyAdapter; + const AdapterBase* base = adapter; - auto backendCmdBuf = ToBackend(base); - static_assert(std::is_same::value, ""); - ASSERT_EQ(cmdBuf, backendCmdBuf); + auto backendAdapter = ToBackend(base); + static_assert(std::is_same::value, ""); + ASSERT_EQ(adapter, backendAdapter); - cmdBuf->Release(); + adapter->Release(); } { - MyCommandBuffer* cmdBuf = new MyCommandBuffer; - CommandBufferBase* base = cmdBuf; + MyAdapter* adapter = new MyAdapter; + AdapterBase* base = adapter; - auto backendCmdBuf = ToBackend(base); - static_assert(std::is_same::value, ""); - ASSERT_EQ(cmdBuf, backendCmdBuf); + auto backendAdapter = ToBackend(base); + static_assert(std::is_same::value, ""); + ASSERT_EQ(adapter, backendAdapter); - cmdBuf->Release(); + adapter->Release(); } } // Test that ToBackend correctly converts Refs to base classes. TEST(ToBackend, Ref) { { - MyCommandBuffer* cmdBuf = new MyCommandBuffer; - const Ref base(cmdBuf); + MyAdapter* adapter = new MyAdapter; + const Ref base(adapter); - const auto& backendCmdBuf = ToBackend(base); - static_assert(std::is_same&>::value, - ""); - ASSERT_EQ(cmdBuf, backendCmdBuf.Get()); + const auto& backendAdapter = ToBackend(base); + static_assert(std::is_same&>::value, ""); + ASSERT_EQ(adapter, backendAdapter.Get()); - cmdBuf->Release(); + adapter->Release(); } { - MyCommandBuffer* cmdBuf = new MyCommandBuffer; - Ref base(cmdBuf); + MyAdapter* adapter = new MyAdapter; + Ref base(adapter); - auto backendCmdBuf = ToBackend(base); - static_assert(std::is_same&>::value, ""); - ASSERT_EQ(cmdBuf, backendCmdBuf.Get()); + auto backendAdapter = ToBackend(base); + static_assert(std::is_same&>::value, ""); + ASSERT_EQ(adapter, backendAdapter.Get()); - cmdBuf->Release(); + adapter->Release(); } } diff --git a/src/tests/unittests/validation/QueueSubmitValidationTests.cpp b/src/tests/unittests/validation/QueueSubmitValidationTests.cpp index fe9911cf50..b4ee859329 100644 --- a/src/tests/unittests/validation/QueueSubmitValidationTests.cpp +++ b/src/tests/unittests/validation/QueueSubmitValidationTests.cpp @@ -23,9 +23,10 @@ namespace { // Test submitting with a mapped buffer is disallowed TEST_F(QueueSubmitValidationTest, SubmitWithMappedBuffer) { // Create a map-write buffer. + const uint64_t kBufferSize = 4; wgpu::BufferDescriptor descriptor; descriptor.usage = wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::CopySrc; - descriptor.size = 4; + descriptor.size = kBufferSize; wgpu::Buffer buffer = device.CreateBuffer(&descriptor); // Create a fake copy destination buffer @@ -36,7 +37,7 @@ namespace { wgpu::CommandBuffer commands; { wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - encoder.CopyBufferToBuffer(buffer, 0, targetBuffer, 0, 4); + encoder.CopyBufferToBuffer(buffer, 0, targetBuffer, 0, kBufferSize); commands = encoder.Finish(); } @@ -45,17 +46,35 @@ namespace { // Submitting when the buffer has never been mapped should succeed queue.Submit(1, &commands); + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.CopyBufferToBuffer(buffer, 0, targetBuffer, 0, kBufferSize); + commands = encoder.Finish(); + } + // Map the buffer, submitting when the buffer is mapped should fail - buffer.MapWriteAsync(nullptr, nullptr); + buffer.MapAsync(wgpu::MapMode::Write, 0, kBufferSize, nullptr, nullptr); // Try submitting before the callback is fired. ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); WaitForAllOperations(device); + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.CopyBufferToBuffer(buffer, 0, targetBuffer, 0, kBufferSize); + commands = encoder.Finish(); + } + // Try submitting after the callback is fired. ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.CopyBufferToBuffer(buffer, 0, targetBuffer, 0, kBufferSize); + commands = encoder.Finish(); + } + // Unmap the buffer, queue submit should succeed buffer.Unmap(); queue.Submit(1, &commands); @@ -182,4 +201,53 @@ namespace { } } + // Test it is invalid to submit a command buffer twice + TEST_F(QueueSubmitValidationTest, CommandBufferSubmittedTwice) { + wgpu::CommandBuffer commandBuffer = device.CreateCommandEncoder().Finish(); + wgpu::Queue queue = device.GetDefaultQueue(); + + // Should succeed + queue.Submit(1, &commandBuffer); + + // Should fail because command buffer was already submitted + ASSERT_DEVICE_ERROR(queue.Submit(1, &commandBuffer)); + } + + // Test resubmitting failed command buffers + TEST_F(QueueSubmitValidationTest, CommandBufferSubmittedFailed) { + // Create a map-write buffer + const uint64_t kBufferSize = 4; + wgpu::BufferDescriptor descriptor; + descriptor.usage = wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::CopySrc; + descriptor.size = kBufferSize; + wgpu::Buffer buffer = device.CreateBuffer(&descriptor); + + // Create a destination buffer for the b2b copy + descriptor.usage = wgpu::BufferUsage::CopyDst; + descriptor.size = kBufferSize; + wgpu::Buffer targetBuffer = device.CreateBuffer(&descriptor); + + // Create a command buffer that reads from the mappable buffer + wgpu::CommandBuffer commands; + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.CopyBufferToBuffer(buffer, 0, targetBuffer, 0, kBufferSize); + commands = encoder.Finish(); + } + + wgpu::Queue queue = device.GetDefaultQueue(); + + // Map the source buffer to force a failure + buffer.MapAsync(wgpu::MapMode::Write, 0, kBufferSize, nullptr, nullptr); + + // Submitting a command buffer with a mapped buffer should fail + ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); + + // Unmap buffer to fix the failure + buffer.Unmap(); + + // Resubmitting any command buffer, even if the problem was fixed, should fail + ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); + } + } // anonymous namespace