From 3c31a896c8d77efe6c39617f35844e33d06e31bd Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Tue, 1 Aug 2023 09:54:14 -0700 Subject: [PATCH] [Impeller] Started using a pool for HostBuffers. (flutter/engine#44081) This removes ~792 MB/s of allocation when scrolling around the Gallery ((358 MB - 226 MB) * 6). ## before Screenshot 2023-07-27 at 1 48 57 PM ## after Screenshot 2023-07-27 at 1 36 35 PM ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [ ] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --------- Co-authored-by: Jonah Williams --- .../flutter/ci/licenses_golden/excluded_files | 1 + .../ci/licenses_golden/licenses_flutter | 2 + .../src/flutter/impeller/base/allocation.cc | 2 +- .../src/flutter/impeller/core/host_buffer.cc | 11 ++++ .../src/flutter/impeller/core/host_buffer.h | 9 +++ engine/src/flutter/impeller/renderer/BUILD.gn | 2 + .../src/flutter/impeller/renderer/context.h | 8 +++ engine/src/flutter/impeller/renderer/pool.h | 54 ++++++++++++++++ .../impeller/renderer/pool_unittests.cc | 63 +++++++++++++++++++ .../flutter/impeller/renderer/render_pass.cc | 13 +++- 10 files changed, 162 insertions(+), 3 deletions(-) create mode 100644 engine/src/flutter/impeller/renderer/pool.h create mode 100644 engine/src/flutter/impeller/renderer/pool_unittests.cc diff --git a/engine/src/flutter/ci/licenses_golden/excluded_files b/engine/src/flutter/ci/licenses_golden/excluded_files index 1b89c0dd74a..6576b306f06 100644 --- a/engine/src/flutter/ci/licenses_golden/excluded_files +++ b/engine/src/flutter/ci/licenses_golden/excluded_files @@ -157,6 +157,7 @@ ../../../flutter/impeller/renderer/device_buffer_unittests.cc ../../../flutter/impeller/renderer/host_buffer_unittests.cc ../../../flutter/impeller/renderer/pipeline_descriptor_unittests.cc +../../../flutter/impeller/renderer/pool_unittests.cc ../../../flutter/impeller/renderer/renderer_dart_unittests.cc ../../../flutter/impeller/renderer/renderer_unittests.cc ../../../flutter/impeller/renderer/testing diff --git a/engine/src/flutter/ci/licenses_golden/licenses_flutter b/engine/src/flutter/ci/licenses_golden/licenses_flutter index 6c1b893c9e0..d0b72e31ac0 100644 --- a/engine/src/flutter/ci/licenses_golden/licenses_flutter +++ b/engine/src/flutter/ci/licenses_golden/licenses_flutter @@ -1600,6 +1600,7 @@ ORIGIN: ../../../flutter/impeller/renderer/pipeline_descriptor.cc + ../../../flu ORIGIN: ../../../flutter/impeller/renderer/pipeline_descriptor.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/renderer/pipeline_library.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/renderer/pipeline_library.h + ../../../flutter/LICENSE +ORIGIN: ../../../flutter/impeller/renderer/pool.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/renderer/prefix_sum_test.comp + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/renderer/render_pass.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/renderer/render_pass.h + ../../../flutter/LICENSE @@ -4294,6 +4295,7 @@ FILE: ../../../flutter/impeller/renderer/pipeline_descriptor.cc FILE: ../../../flutter/impeller/renderer/pipeline_descriptor.h FILE: ../../../flutter/impeller/renderer/pipeline_library.cc FILE: ../../../flutter/impeller/renderer/pipeline_library.h +FILE: ../../../flutter/impeller/renderer/pool.h FILE: ../../../flutter/impeller/renderer/prefix_sum_test.comp FILE: ../../../flutter/impeller/renderer/render_pass.cc FILE: ../../../flutter/impeller/renderer/render_pass.h diff --git a/engine/src/flutter/impeller/base/allocation.cc b/engine/src/flutter/impeller/base/allocation.cc index 06af6f22442..753a65ecaa9 100644 --- a/engine/src/flutter/impeller/base/allocation.cc +++ b/engine/src/flutter/impeller/base/allocation.cc @@ -62,7 +62,7 @@ bool Allocation::ReserveNPOT(size_t reserved) { } bool Allocation::Reserve(size_t reserved) { - if (reserved == reserved_) { + if (reserved <= reserved_) { return true; } diff --git a/engine/src/flutter/impeller/core/host_buffer.cc b/engine/src/flutter/impeller/core/host_buffer.cc index c4f6af77543..25671017391 100644 --- a/engine/src/flutter/impeller/core/host_buffer.cc +++ b/engine/src/flutter/impeller/core/host_buffer.cc @@ -87,4 +87,15 @@ std::shared_ptr HostBuffer::GetDeviceBuffer( return device_buffer_; } +void HostBuffer::Reset() { + generation_ += 1; + device_buffer_ = nullptr; + bool did_truncate = Truncate(0); + FML_CHECK(did_truncate); +} + +size_t HostBuffer::GetSize() const { + return GetReservedLength(); +} + } // namespace impeller diff --git a/engine/src/flutter/impeller/core/host_buffer.h b/engine/src/flutter/impeller/core/host_buffer.h index ec2965d75c1..c5485c73533 100644 --- a/engine/src/flutter/impeller/core/host_buffer.h +++ b/engine/src/flutter/impeller/core/host_buffer.h @@ -111,6 +111,15 @@ class HostBuffer final : public std::enable_shared_from_this, /// BufferView Emplace(size_t length, size_t align, const EmplaceProc& cb); + //---------------------------------------------------------------------------- + /// @brief Resets the contents of the HostBuffer to nothing so it can be + /// reused. + void Reset(); + + //---------------------------------------------------------------------------- + /// @brief Returns the size of the HostBuffer in memory in bytes. + size_t GetSize() const; + private: mutable std::shared_ptr device_buffer_; mutable size_t device_buffer_generation_ = 0u; diff --git a/engine/src/flutter/impeller/renderer/BUILD.gn b/engine/src/flutter/impeller/renderer/BUILD.gn index b43c71a3e34..04e6ef19f41 100644 --- a/engine/src/flutter/impeller/renderer/BUILD.gn +++ b/engine/src/flutter/impeller/renderer/BUILD.gn @@ -73,6 +73,7 @@ impeller_component("renderer") { "pipeline_descriptor.h", "pipeline_library.cc", "pipeline_library.h", + "pool.h", "render_pass.cc", "render_pass.h", "render_target.cc", @@ -125,6 +126,7 @@ impeller_component("renderer_unittests") { "device_buffer_unittests.cc", "host_buffer_unittests.cc", "pipeline_descriptor_unittests.cc", + "pool_unittests.cc", "renderer_unittests.cc", ] diff --git a/engine/src/flutter/impeller/renderer/context.h b/engine/src/flutter/impeller/renderer/context.h index 9124e23ee68..b597e4fee1a 100644 --- a/engine/src/flutter/impeller/renderer/context.h +++ b/engine/src/flutter/impeller/renderer/context.h @@ -9,7 +9,9 @@ #include "flutter/fml/macros.h" #include "impeller/core/formats.h" +#include "impeller/core/host_buffer.h" #include "impeller/renderer/capabilities.h" +#include "impeller/renderer/pool.h" namespace impeller { @@ -158,10 +160,16 @@ class Context { /// virtual void Shutdown() = 0; + //---------------------------------------------------------------------------- + /// @brief Accessor for a pool of HostBuffers. + Pool& GetHostBufferPool() const { return host_buffer_pool_; } + protected: Context(); private: + mutable Pool host_buffer_pool_ = Pool(1'000'000); + FML_DISALLOW_COPY_AND_ASSIGN(Context); }; diff --git a/engine/src/flutter/impeller/renderer/pool.h b/engine/src/flutter/impeller/renderer/pool.h new file mode 100644 index 00000000000..f215ff2a6d9 --- /dev/null +++ b/engine/src/flutter/impeller/renderer/pool.h @@ -0,0 +1,54 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#pragma once + +#include +#include + +namespace impeller { + +/// @brief A thread-safe pool with a limited byte size. +/// @tparam T The type that the pool will contain. +template +class Pool { + public: + Pool(uint32_t limit_bytes) : limit_bytes_(limit_bytes), size_(0) {} + + std::shared_ptr Grab() { + std::scoped_lock lock(mutex_); + if (pool_.empty()) { + return T::Create(); + } + std::shared_ptr result = std::move(pool_.back()); + pool_.pop_back(); + size_ -= result->GetSize(); + return result; + } + + void Recycle(std::shared_ptr object) { + std::scoped_lock lock(mutex_); + size_t object_size = object->GetSize(); + if (size_ + object_size <= limit_bytes_ && + object_size < (limit_bytes_ / 2)) { + object->Reset(); + size_ += object_size; + pool_.emplace_back(std::move(object)); + } + } + + uint32_t GetSize() const { + std::scoped_lock lock(mutex_); + return size_; + } + + private: + std::vector> pool_; + const uint32_t limit_bytes_; + uint32_t size_; + // Note: This would perform better as a lockless ring buffer. + mutable std::mutex mutex_; +}; + +} // namespace impeller diff --git a/engine/src/flutter/impeller/renderer/pool_unittests.cc b/engine/src/flutter/impeller/renderer/pool_unittests.cc new file mode 100644 index 00000000000..ec7f9958e7b --- /dev/null +++ b/engine/src/flutter/impeller/renderer/pool_unittests.cc @@ -0,0 +1,63 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "gtest/gtest.h" + +#include "impeller/renderer/pool.h" + +namespace impeller { +namespace testing { + +namespace { +class Foobar { + public: + static std::shared_ptr Create() { return std::make_shared(); } + + size_t GetSize() const { return size_; } + + void SetSize(size_t size) { size_ = size; } + + void Reset() { is_reset_ = true; } + + bool GetIsReset() const { return is_reset_; } + + void SetIsReset(bool is_reset) { is_reset_ = is_reset; } + + private: + size_t size_; + bool is_reset_ = false; +}; +} // namespace + +TEST(PoolTest, Simple) { + Pool pool(1'000); + { + auto grabbed = pool.Grab(); + grabbed->SetSize(123); + pool.Recycle(grabbed); + EXPECT_EQ(pool.GetSize(), 123u); + } + auto grabbed = pool.Grab(); + EXPECT_EQ(grabbed->GetSize(), 123u); + EXPECT_TRUE(grabbed->GetIsReset()); + EXPECT_EQ(pool.GetSize(), 0u); +} + +TEST(PoolTest, Overload) { + Pool pool(1'000); + { + std::vector> values; + for (int i = 0; i < 20; i++) { + values.push_back(pool.Grab()); + } + for (auto value : values) { + value->SetSize(100); + pool.Recycle(value); + } + } + EXPECT_EQ(pool.GetSize(), 1'000u); +} + +} // namespace testing +} // namespace impeller diff --git a/engine/src/flutter/impeller/renderer/render_pass.cc b/engine/src/flutter/impeller/renderer/render_pass.cc index 5df6af0bd53..ffc5e44904a 100644 --- a/engine/src/flutter/impeller/renderer/render_pass.cc +++ b/engine/src/flutter/impeller/renderer/render_pass.cc @@ -10,9 +10,18 @@ RenderPass::RenderPass(std::weak_ptr context, const RenderTarget& target) : context_(std::move(context)), render_target_(target), - transients_buffer_(HostBuffer::Create()) {} + transients_buffer_() { + auto strong_context = context_.lock(); + FML_DCHECK(strong_context); + transients_buffer_ = strong_context->GetHostBufferPool().Grab(); +} -RenderPass::~RenderPass() = default; +RenderPass::~RenderPass() { + auto strong_context = context_.lock(); + if (strong_context) { + strong_context->GetHostBufferPool().Recycle(transients_buffer_); + } +} const RenderTarget& RenderPass::GetRenderTarget() const { return render_target_;