From c9bb068cfa51ac6e43b4d40872bf9864e2760eec Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Wed, 20 Nov 2024 09:41:14 -0800 Subject: [PATCH] [Impeller] flush all GLES cmd buffers together. (flutter/engine#56724) Locally this gives much better performance, about doubling frame time on the Pixel 4. This avoids multiple glbuffersubdata calls that seems to perform particularly bad on mobile devices. Thinking about it more, I'm not sure that having a separate EncodeCommands API is useful for RenderPass/BlitPass. instead they should probably just use submit. but that is a refactor for another day. https://github.com/flutter/flutter/issues/159177 --- .../renderer/backend/gles/context_gles.cc | 10 ++++++++ .../renderer/backend/gles/context_gles.h | 7 ++++++ .../renderer/backend/gles/reactor_gles.cc | 6 +++-- .../renderer/backend/gles/reactor_gles.h | 4 +++- .../renderer/backend/gles/render_pass_gles.cc | 17 ++++++------- .../backend/gles/test/reactor_unittests.cc | 24 +++++++++++++++++++ 6 files changed, 57 insertions(+), 11 deletions(-) diff --git a/engine/src/flutter/impeller/renderer/backend/gles/context_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/context_gles.cc index 640f36e3ce..ba98e78f5c 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/context_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/context_gles.cc @@ -159,6 +159,16 @@ void ContextGLES::ResetThreadLocalState() const { }); } +bool ContextGLES::EnqueueCommandBuffer( + std::shared_ptr command_buffer) { + return true; +} + +// |Context| +[[nodiscard]] bool ContextGLES::FlushCommandBuffers() { + return reactor_->React(); +} + // |Context| bool ContextGLES::AddTrackingFence( const std::shared_ptr& texture) const { diff --git a/engine/src/flutter/impeller/renderer/backend/gles/context_gles.h b/engine/src/flutter/impeller/renderer/backend/gles/context_gles.h index 138494429b..1c2587cc0d 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/context_gles.h +++ b/engine/src/flutter/impeller/renderer/backend/gles/context_gles.h @@ -99,6 +99,13 @@ class ContextGLES final : public Context, // |Context| void ResetThreadLocalState() const override; + // |Context| + [[nodiscard]] bool EnqueueCommandBuffer( + std::shared_ptr command_buffer) override; + + // |Context| + [[nodiscard]] bool FlushCommandBuffers() override; + ContextGLES(const ContextGLES&) = delete; ContextGLES& operator=(const ContextGLES&) = delete; diff --git a/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc index 945e2205fe..82266bfd99 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc @@ -166,7 +166,7 @@ std::optional ReactorGLES::GetGLFence(const HandleGLES& handle) const { return std::nullopt; } -bool ReactorGLES::AddOperation(Operation operation) { +bool ReactorGLES::AddOperation(Operation operation, bool defer) { if (!operation) { return false; } @@ -176,7 +176,9 @@ bool ReactorGLES::AddOperation(Operation 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(); + if (!defer) { + [[maybe_unused]] auto result = React(); + } return true; } diff --git a/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.h b/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.h index dc1396314e..88d77ccffe 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.h +++ b/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.h @@ -214,10 +214,12 @@ class ReactorGLES { /// torn down. /// /// @param[in] operation The operation + /// @param[in] defer If false, the reactor attempts to React after + /// adding this operation. /// /// @return If the operation was successfully queued for completion. /// - [[nodiscard]] bool AddOperation(Operation operation); + [[nodiscard]] bool AddOperation(Operation operation, bool defer = false); //---------------------------------------------------------------------------- /// @brief Register a cleanup callback that will be invokved with the diff --git a/engine/src/flutter/impeller/renderer/backend/gles/render_pass_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/render_pass_gles.cc index 5ca2f5d47f..32228d3d87 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/render_pass_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/render_pass_gles.cc @@ -585,14 +585,15 @@ bool RenderPassGLES::OnEncodeCommands(const Context& context) const { std::shared_ptr shared_this = shared_from_this(); auto tracer = ContextGLES::Cast(context).GetGPUTracer(); - return reactor_->AddOperation([pass_data, - allocator = context.GetResourceAllocator(), - render_pass = std::move(shared_this), - tracer](const auto& reactor) { - auto result = EncodeCommandsInReactor(*pass_data, allocator, reactor, - render_pass->commands_, tracer); - FML_CHECK(result) << "Must be able to encode GL commands without error."; - }); + return reactor_->AddOperation( + [pass_data, allocator = context.GetResourceAllocator(), + render_pass = std::move(shared_this), tracer](const auto& reactor) { + auto result = EncodeCommandsInReactor(*pass_data, allocator, reactor, + render_pass->commands_, tracer); + FML_CHECK(result) + << "Must be able to encode GL commands without error."; + }, + /*defer=*/true); } } // namespace impeller diff --git a/engine/src/flutter/impeller/renderer/backend/gles/test/reactor_unittests.cc b/engine/src/flutter/impeller/renderer/backend/gles/test/reactor_unittests.cc index fe589c7dbc..c43b9b1b96 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/test/reactor_unittests.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/test/reactor_unittests.cc @@ -93,5 +93,29 @@ TEST(ReactorGLES, PerThreadOperationQueues) { EXPECT_TRUE(op2_called); } +TEST(ReactorGLES, CanDeferOperations) { + 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); + + // Add operation executes tasks as long as the reactor can run tasks on + // the current thread. + bool did_run = false; + EXPECT_TRUE( + reactor->AddOperation([&](const ReactorGLES&) { did_run = true; })); + EXPECT_TRUE(did_run); + + //...unless defer=true is specified, which only enqueues in the reactor. + did_run = false; + EXPECT_TRUE(reactor->AddOperation([&](const ReactorGLES&) { did_run = true; }, + /*defer=*/true)); + EXPECT_FALSE(did_run); + EXPECT_TRUE(reactor->React()); + EXPECT_TRUE(did_run); +} + } // namespace testing } // namespace impeller