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 <cwallez@chromium.org>
Reviewed-by: Rafael Cintron <rafael.cintron@microsoft.com>
Commit-Queue: Austin Eng <enga@chromium.org>

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();
 }

src/tests/unittests/ToBackendTests.cpp

@@ -19,17 +19,17 @@
 
 #include <type_traits>
 
-// 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<MyBackendTraits>(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<decltype(backendCmdBuf), const MyCommandBuffer*>::value, "");
-        ASSERT_EQ(cmdBuf, backendCmdBuf);
+        auto backendAdapter = ToBackend(base);
+        static_assert(std::is_same<decltype(backendAdapter), const MyAdapter*>::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<decltype(backendCmdBuf), MyCommandBuffer*>::value, "");
-        ASSERT_EQ(cmdBuf, backendCmdBuf);
+        auto backendAdapter = ToBackend(base);
+        static_assert(std::is_same<decltype(backendAdapter), MyAdapter*>::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<CommandBufferBase> base(cmdBuf);
+        MyAdapter* adapter = new MyAdapter;
+        const Ref<AdapterBase> base(adapter);
 
-        const auto& backendCmdBuf = ToBackend(base);
-        static_assert(std::is_same<decltype(ToBackend(base)), const Ref<MyCommandBuffer>&>::value,
-                      "");
-        ASSERT_EQ(cmdBuf, backendCmdBuf.Get());
+        const auto& backendAdapter = ToBackend(base);
+        static_assert(std::is_same<decltype(ToBackend(base)), const Ref<MyAdapter>&>::value, "");
+        ASSERT_EQ(adapter, backendAdapter.Get());
 
-        cmdBuf->Release();
+        adapter->Release();
     }
     {
-        MyCommandBuffer* cmdBuf = new MyCommandBuffer;
-        Ref<CommandBufferBase> base(cmdBuf);
+        MyAdapter* adapter = new MyAdapter;
+        Ref<AdapterBase> base(adapter);
 
-        auto backendCmdBuf = ToBackend(base);
-        static_assert(std::is_same<decltype(ToBackend(base)), Ref<MyCommandBuffer>&>::value, "");
-        ASSERT_EQ(cmdBuf, backendCmdBuf.Get());
+        auto backendAdapter = ToBackend(base);
+        static_assert(std::is_same<decltype(ToBackend(base)), Ref<MyAdapter>&>::value, "");
+        ASSERT_EQ(adapter, backendAdapter.Get());
 
-        cmdBuf->Release();
+        adapter->Release();
     }
 }

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