From 644f85eee17e95f75a4bae99fdd524cf4d5d8d60 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Fri, 15 Nov 2024 08:19:56 -0800 Subject: [PATCH] [Impeller] Maintain separate queues of GLES operations for each thread in the reactor (#56573) Threads that add operations to the ReactorGLES assume that those operations will be executed serially. But prior to this change, the ReactorGLES added all operations into one queue. The reactor would then execute those operations on any thread that can react. This could cause operations that were added to the reactor on the raster thread to be submitted to the GPU on the IO thread (or vice versa). The reactor does not wait for the GPU to finish execution of those operations. So other operations added on the raster thread could be submitted by a reaction before the GPU has completed the operation that was submitted on the IO thread. This PR ensures that operations added to the reactor on a given thread will be executed during a reaction on that same thread. If the thread can not currently react, then the operations will be queued until the thread enables reactions. This also adds a call to CommandBuffer::WaitUntilScheduled to ImageDecoderImpeller. This ensures that the command buffer submitted on the IO thread is flushed before the image is returned. Fixes https://github.com/flutter/flutter/issues/158535 Fixes https://github.com/flutter/flutter/issues/158388 Fixes https://github.com/flutter/flutter/issues/158390 --- .../backend/gles/command_buffer_gles.cc | 4 +-- .../backend/gles/command_buffer_gles.h | 2 +- .../renderer/backend/gles/proc_table_gles.h | 1 + .../renderer/backend/gles/reactor_gles.cc | 16 ++++----- impeller/renderer/backend/gles/reactor_gles.h | 6 ++-- .../backend/gles/test/reactor_unittests.cc | 33 +++++++++++++++++++ .../backend/metal/command_buffer_mtl.h | 2 +- .../backend/metal/command_buffer_mtl.mm | 2 +- .../backend/vulkan/command_buffer_vk.cc | 2 +- .../backend/vulkan/command_buffer_vk.h | 2 +- impeller/renderer/command_buffer.cc | 4 +-- impeller/renderer/command_buffer.h | 7 ++-- impeller/renderer/testing/mocks.h | 2 +- lib/ui/painting/image_decoder_impeller.cc | 4 +++ 14 files changed, 63 insertions(+), 24 deletions(-) diff --git a/impeller/renderer/backend/gles/command_buffer_gles.cc b/impeller/renderer/backend/gles/command_buffer_gles.cc index d570cbc6e972e..2ade0db37f821 100644 --- a/impeller/renderer/backend/gles/command_buffer_gles.cc +++ b/impeller/renderer/backend/gles/command_buffer_gles.cc @@ -39,8 +39,8 @@ bool CommandBufferGLES::OnSubmitCommands(CompletionCallback callback) { } // |CommandBuffer| -void CommandBufferGLES::OnWaitUntilScheduled() { - reactor_->GetProcTable().Flush(); +void CommandBufferGLES::OnWaitUntilCompleted() { + reactor_->GetProcTable().Finish(); } // |CommandBuffer| diff --git a/impeller/renderer/backend/gles/command_buffer_gles.h b/impeller/renderer/backend/gles/command_buffer_gles.h index 2188ccdd3fa53..a1536d9433271 100644 --- a/impeller/renderer/backend/gles/command_buffer_gles.h +++ b/impeller/renderer/backend/gles/command_buffer_gles.h @@ -35,7 +35,7 @@ class CommandBufferGLES final : public CommandBuffer { bool OnSubmitCommands(CompletionCallback callback) override; // |CommandBuffer| - void OnWaitUntilScheduled() override; + void OnWaitUntilCompleted() override; // |CommandBuffer| std::shared_ptr OnCreateRenderPass(RenderTarget target) override; diff --git a/impeller/renderer/backend/gles/proc_table_gles.h b/impeller/renderer/backend/gles/proc_table_gles.h index eadcb4f333057..f8503b52f9599 100644 --- a/impeller/renderer/backend/gles/proc_table_gles.h +++ b/impeller/renderer/backend/gles/proc_table_gles.h @@ -168,6 +168,7 @@ struct GLProc { PROC(DrawElements); \ PROC(Enable); \ PROC(EnableVertexAttribArray); \ + PROC(Finish); \ PROC(Flush); \ PROC(FramebufferRenderbuffer); \ PROC(FramebufferTexture2D); \ diff --git a/impeller/renderer/backend/gles/reactor_gles.cc b/impeller/renderer/backend/gles/reactor_gles.cc index 4ccbb47891b38..6f6e7288aa53c 100644 --- a/impeller/renderer/backend/gles/reactor_gles.cc +++ b/impeller/renderer/backend/gles/reactor_gles.cc @@ -105,8 +105,10 @@ bool ReactorGLES::RemoveWorker(WorkerID worker) { } bool ReactorGLES::HasPendingOperations() const { + auto thread_id = std::this_thread::get_id(); Lock ops_lock(ops_mutex_); - return !ops_.empty(); + auto it = ops_.find(thread_id); + return it != ops_.end() ? !it->second.empty() : false; } const ProcTableGLES& ReactorGLES::GetProcTable() const { @@ -136,9 +138,10 @@ bool ReactorGLES::AddOperation(Operation operation) { if (!operation) { return false; } + auto thread_id = std::this_thread::get_id(); { Lock ops_lock(ops_mutex_); - ops_.emplace_back(std::move(operation)); + ops_[thread_id].emplace_back(std::move(operation)); } // Attempt a reaction if able but it is not an error if this isn't possible. [[maybe_unused]] auto result = React(); @@ -191,10 +194,6 @@ bool ReactorGLES::React() { } TRACE_EVENT0("impeller", "ReactorGLES::React"); while (HasPendingOperations()) { - // Both the raster thread and the IO thread can flush queued operations. - // Ensure that execution of the ops is serialized. - Lock execution_lock(ops_execution_mutex_); - if (!ReactOnce()) { return false; } @@ -279,10 +278,11 @@ bool ReactorGLES::FlushOps() { // Do NOT hold the ops or handles locks while performing operations in case // the ops enqueue more ops. - decltype(ops_) ops; + decltype(ops_)::mapped_type ops; + auto thread_id = std::this_thread::get_id(); { Lock ops_lock(ops_mutex_); - std::swap(ops_, ops); + std::swap(ops_[thread_id], ops); } for (const auto& op : ops) { TRACE_EVENT0("impeller", "ReactorGLES::Operation"); diff --git a/impeller/renderer/backend/gles/reactor_gles.h b/impeller/renderer/backend/gles/reactor_gles.h index 80fb6877365bb..1b86a8ed328c1 100644 --- a/impeller/renderer/backend/gles/reactor_gles.h +++ b/impeller/renderer/backend/gles/reactor_gles.h @@ -260,9 +260,9 @@ class ReactorGLES { std::unique_ptr proc_table_; - Mutex ops_execution_mutex_; mutable Mutex ops_mutex_; - std::vector ops_ IPLR_GUARDED_BY(ops_mutex_); + std::map> ops_ IPLR_GUARDED_BY( + ops_mutex_); // Make sure the container is one where erasing items during iteration doesn't // invalidate other iterators. @@ -280,7 +280,7 @@ class ReactorGLES { bool can_set_debug_labels_ = false; bool is_valid_ = false; - bool ReactOnce() IPLR_REQUIRES(ops_execution_mutex_); + bool ReactOnce(); bool HasPendingOperations() const; diff --git a/impeller/renderer/backend/gles/test/reactor_unittests.cc b/impeller/renderer/backend/gles/test/reactor_unittests.cc index efa7b576be82b..fe589c7dbcaae 100644 --- a/impeller/renderer/backend/gles/test/reactor_unittests.cc +++ b/impeller/renderer/backend/gles/test/reactor_unittests.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include +#include "flutter/fml/synchronization/waitable_event.h" #include "flutter/testing/testing.h" // IWYU pragma: keep #include "gtest/gtest.h" #include "impeller/renderer/backend/gles/handle_gles.h" @@ -60,5 +61,37 @@ TEST(ReactorGLES, DeletesHandlesDuringShutdown) { calls.end()); } +TEST(ReactorGLES, PerThreadOperationQueues) { + auto mock_gles = MockGLES::Init(); + ProcTableGLES::Resolver resolver = kMockResolverGLES; + auto proc_table = std::make_unique(resolver); + auto worker = std::make_shared(); + auto reactor = std::make_shared(std::move(proc_table)); + reactor->AddWorker(worker); + + bool op1_called = false; + EXPECT_TRUE( + reactor->AddOperation([&](const ReactorGLES&) { op1_called = true; })); + + fml::AutoResetWaitableEvent event; + bool op2_called = false; + std::thread thread([&] { + EXPECT_TRUE( + reactor->AddOperation([&](const ReactorGLES&) { op2_called = true; })); + event.Wait(); + EXPECT_TRUE(reactor->React()); + }); + + // Reacting on the main thread should only run the main thread's operation. + EXPECT_TRUE(reactor->React()); + EXPECT_TRUE(op1_called); + EXPECT_FALSE(op2_called); + + // Reacting on the second thread will run the second thread's operation. + event.Signal(); + thread.join(); + EXPECT_TRUE(op2_called); +} + } // namespace testing } // namespace impeller diff --git a/impeller/renderer/backend/metal/command_buffer_mtl.h b/impeller/renderer/backend/metal/command_buffer_mtl.h index 6c5c948593713..c644ea114f457 100644 --- a/impeller/renderer/backend/metal/command_buffer_mtl.h +++ b/impeller/renderer/backend/metal/command_buffer_mtl.h @@ -37,7 +37,7 @@ class CommandBufferMTL final : public CommandBuffer { bool OnSubmitCommands(CompletionCallback callback) override; // |CommandBuffer| - void OnWaitUntilScheduled() override; + void OnWaitUntilCompleted() override; // |CommandBuffer| std::shared_ptr OnCreateRenderPass(RenderTarget target) override; diff --git a/impeller/renderer/backend/metal/command_buffer_mtl.mm b/impeller/renderer/backend/metal/command_buffer_mtl.mm index 93bee9c7c1182..f6b907da54eff 100644 --- a/impeller/renderer/backend/metal/command_buffer_mtl.mm +++ b/impeller/renderer/backend/metal/command_buffer_mtl.mm @@ -187,7 +187,7 @@ static bool LogMTLCommandBufferErrorIfPresent(id buffer) { return true; } -void CommandBufferMTL::OnWaitUntilScheduled() {} +void CommandBufferMTL::OnWaitUntilCompleted() {} std::shared_ptr CommandBufferMTL::OnCreateRenderPass( RenderTarget target) { diff --git a/impeller/renderer/backend/vulkan/command_buffer_vk.cc b/impeller/renderer/backend/vulkan/command_buffer_vk.cc index 7abea7ab7604c..cbce517d435e9 100644 --- a/impeller/renderer/backend/vulkan/command_buffer_vk.cc +++ b/impeller/renderer/backend/vulkan/command_buffer_vk.cc @@ -47,7 +47,7 @@ bool CommandBufferVK::OnSubmitCommands(CompletionCallback callback) { FML_UNREACHABLE() } -void CommandBufferVK::OnWaitUntilScheduled() {} +void CommandBufferVK::OnWaitUntilCompleted() {} std::shared_ptr CommandBufferVK::OnCreateRenderPass( RenderTarget target) { diff --git a/impeller/renderer/backend/vulkan/command_buffer_vk.h b/impeller/renderer/backend/vulkan/command_buffer_vk.h index 09bcadeca764b..e4ac9138859b1 100644 --- a/impeller/renderer/backend/vulkan/command_buffer_vk.h +++ b/impeller/renderer/backend/vulkan/command_buffer_vk.h @@ -100,7 +100,7 @@ class CommandBufferVK final bool OnSubmitCommands(CompletionCallback callback) override; // |CommandBuffer| - void OnWaitUntilScheduled() override; + void OnWaitUntilCompleted() override; // |CommandBuffer| std::shared_ptr OnCreateRenderPass(RenderTarget target) override; diff --git a/impeller/renderer/command_buffer.cc b/impeller/renderer/command_buffer.cc index 5ff1113c100ec..918e590da2beb 100644 --- a/impeller/renderer/command_buffer.cc +++ b/impeller/renderer/command_buffer.cc @@ -30,8 +30,8 @@ bool CommandBuffer::SubmitCommands() { return SubmitCommands(nullptr); } -void CommandBuffer::WaitUntilScheduled() { - return OnWaitUntilScheduled(); +void CommandBuffer::WaitUntilCompleted() { + return OnWaitUntilCompleted(); } std::shared_ptr CommandBuffer::CreateRenderPass( diff --git a/impeller/renderer/command_buffer.h b/impeller/renderer/command_buffer.h index 958b4d0bd41f1..cbfd406895c5f 100644 --- a/impeller/renderer/command_buffer.h +++ b/impeller/renderer/command_buffer.h @@ -61,9 +61,10 @@ class CommandBuffer { virtual void SetLabel(std::string_view label) const = 0; //---------------------------------------------------------------------------- - /// @brief Force execution of pending GPU commands. + /// @brief Block the current thread until the GPU has completed execution + /// of the commands. /// - void WaitUntilScheduled(); + void WaitUntilCompleted(); //---------------------------------------------------------------------------- /// @brief Create a render pass to record render commands into. @@ -102,7 +103,7 @@ class CommandBuffer { [[nodiscard]] virtual bool OnSubmitCommands(CompletionCallback callback) = 0; - virtual void OnWaitUntilScheduled() = 0; + virtual void OnWaitUntilCompleted() = 0; virtual std::shared_ptr OnCreateComputePass() = 0; diff --git a/impeller/renderer/testing/mocks.h b/impeller/renderer/testing/mocks.h index b389668135815..99464fa9e7597 100644 --- a/impeller/renderer/testing/mocks.h +++ b/impeller/renderer/testing/mocks.h @@ -128,7 +128,7 @@ class MockCommandBuffer : public CommandBuffer { OnSubmitCommands, (CompletionCallback callback), (override)); - MOCK_METHOD(void, OnWaitUntilScheduled, (), (override)); + MOCK_METHOD(void, OnWaitUntilCompleted, (), (override)); MOCK_METHOD(std::shared_ptr, OnCreateComputePass, (), diff --git a/lib/ui/painting/image_decoder_impeller.cc b/lib/ui/painting/image_decoder_impeller.cc index 1355653d59a7e..656b3a4aa165c 100644 --- a/lib/ui/painting/image_decoder_impeller.cc +++ b/lib/ui/painting/image_decoder_impeller.cc @@ -384,6 +384,10 @@ ImageDecoderImpeller::UnsafeUploadTextureToPrivate( return std::make_pair(nullptr, decode_error); } + // Flush the pending command buffer to ensure that its output becomes visible + // to the raster thread. + command_buffer->WaitUntilCompleted(); + context->DisposeThreadLocalCachedResources(); return std::make_pair(