From d3235504c108b7aeee176a9c0df4fa1fbebbf0da Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Mon, 23 Sep 2024 11:40:08 -0700 Subject: [PATCH] [Impeller] use BufferSubData to update gles device buffer. (flutter/engine#55330) Measures as substantially faster on windows. See the bufferSubData docs: > When replacing the entire data store, consider using glBufferSubData rather than completely recreating the data store with glBufferData. This avoids the cost of reallocating the data store. Tracks invalidated ranges and only updates those ranges using the Flush API. Fixes https://github.com/flutter/flutter/issues/104447 --- .../impeller/core/allocator_unittests.cc | 47 +++++++++++++++++++ engine/src/flutter/impeller/core/range.h | 14 ++++++ .../backend/gles/device_buffer_gles.cc | 35 +++++++++----- .../backend/gles/device_buffer_gles.h | 4 +- .../renderer/backend/gles/proc_table_gles.h | 1 + 5 files changed, 87 insertions(+), 14 deletions(-) diff --git a/engine/src/flutter/impeller/core/allocator_unittests.cc b/engine/src/flutter/impeller/core/allocator_unittests.cc index c0c2268dfb1..b55140ae3bf 100644 --- a/engine/src/flutter/impeller/core/allocator_unittests.cc +++ b/engine/src/flutter/impeller/core/allocator_unittests.cc @@ -79,5 +79,52 @@ TEST(AllocatorTest, TextureDescriptorCompatibility) { } } +TEST(AllocatorTest, RangeTest) { + { + Range a = Range{0, 10}; + Range b = Range{10, 20}; + auto merged = a.Merge(b); + + EXPECT_EQ(merged.offset, 0u); + EXPECT_EQ(merged.length, 30u); + } + + { + Range a = Range{0, 10}; + Range b = Range{100, 20}; + auto merged = a.Merge(b); + + EXPECT_EQ(merged.offset, 0u); + EXPECT_EQ(merged.length, 120u); + } + + { + Range a = Range{0, 10}; + Range b = Range{100, 20}; + auto merged = b.Merge(a); + + EXPECT_EQ(merged.offset, 0u); + EXPECT_EQ(merged.length, 120u); + } + + { + Range a = Range{0, 10}; + Range b = Range{100, 0}; + auto merged = b.Merge(a); + + EXPECT_EQ(merged.offset, 0u); + EXPECT_EQ(merged.length, 10u); + } + + { + Range a = Range{0, 10}; + Range b = Range{0, 10}; + auto merged = b.Merge(a); + + EXPECT_EQ(merged.offset, 0u); + EXPECT_EQ(merged.length, 10u); + } +} + } // namespace testing } // namespace impeller diff --git a/engine/src/flutter/impeller/core/range.h b/engine/src/flutter/impeller/core/range.h index 94c61baf3bb..b1208d1bb77 100644 --- a/engine/src/flutter/impeller/core/range.h +++ b/engine/src/flutter/impeller/core/range.h @@ -5,6 +5,7 @@ #ifndef FLUTTER_IMPELLER_CORE_RANGE_H_ #define FLUTTER_IMPELLER_CORE_RANGE_H_ +#include #include namespace impeller { @@ -21,6 +22,19 @@ struct Range { constexpr bool operator==(const Range& o) const { return offset == o.offset && length == o.length; } + + /// @brief Create a new range that is a union of this range and other. + constexpr Range Merge(const Range& other) { + if (other.length == 0) { + return *this; + } + if (length == 0) { + return other; + } + auto end_offset = std::max(offset + length, other.offset + other.length); + auto start_offset = std::min(offset, other.offset); + return Range{start_offset, end_offset - start_offset}; + } }; } // namespace impeller diff --git a/engine/src/flutter/impeller/renderer/backend/gles/device_buffer_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/device_buffer_gles.cc index a6a91e43ab5..38c1e59bca3 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/device_buffer_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/device_buffer_gles.cc @@ -7,10 +7,8 @@ #include #include -#include "flutter/fml/trace_event.h" #include "impeller/base/allocation.h" #include "impeller/base/config.h" -#include "impeller/base/validation.h" namespace impeller { @@ -53,13 +51,22 @@ bool DeviceBufferGLES::OnCopyHostBuffer(const uint8_t* source, std::memmove(backing_store_->GetBuffer() + offset, source + source_range.offset, source_range.length); - ++generation_; + Flush(Range{offset, source_range.length}); return true; } void DeviceBufferGLES::Flush(std::optional range) const { - generation_++; + if (!range.has_value()) { + dirty_range_ = Range{ + 0, static_cast(backing_store_->GetLength().GetByteSize())}; + } else { + if (dirty_range_.has_value()) { + dirty_range_ = dirty_range_->Merge(range.value()); + } else { + dirty_range_ = range.value(); + } + } } static GLenum ToTarget(DeviceBufferGLES::BindingType type) { @@ -86,14 +93,17 @@ bool DeviceBufferGLES::BindAndUploadDataIfNecessary(BindingType type) const { const auto& gl = reactor_->GetProcTable(); gl.BindBuffer(target_type, buffer.value()); - - if (upload_generation_ != generation_) { - TRACE_EVENT1( - "impeller", "BufferData", "Bytes", - std::to_string(backing_store_->GetLength().GetByteSize()).c_str()); + if (!initialized_) { gl.BufferData(target_type, backing_store_->GetLength().GetByteSize(), - backing_store_->GetBuffer(), GL_STATIC_DRAW); - upload_generation_ = generation_; + nullptr, GL_DYNAMIC_DRAW); + initialized_ = true; + } + + if (dirty_range_.has_value()) { + auto range = dirty_range_.value(); + gl.BufferSubData(target_type, range.offset, range.length, + backing_store_->GetBuffer() + range.offset); + dirty_range_ = std::nullopt; } return true; @@ -122,7 +132,8 @@ void DeviceBufferGLES::UpdateBufferData( if (update_buffer_data) { update_buffer_data(backing_store_->GetBuffer(), backing_store_->GetLength().GetByteSize()); - ++generation_; + Flush(Range{ + 0, static_cast(backing_store_->GetLength().GetByteSize())}); } } diff --git a/engine/src/flutter/impeller/renderer/backend/gles/device_buffer_gles.h b/engine/src/flutter/impeller/renderer/backend/gles/device_buffer_gles.h index 932a2df69be..db65d0cdd65 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/device_buffer_gles.h +++ b/engine/src/flutter/impeller/renderer/backend/gles/device_buffer_gles.h @@ -44,8 +44,8 @@ class DeviceBufferGLES final ReactorGLES::Ref reactor_; HandleGLES handle_; mutable std::shared_ptr backing_store_; - mutable uint32_t generation_ = 0; - mutable uint32_t upload_generation_ = 0; + mutable std::optional dirty_range_ = std::nullopt; + mutable bool initialized_ = false; // |DeviceBuffer| uint8_t* OnGetContents() const override; diff --git a/engine/src/flutter/impeller/renderer/backend/gles/proc_table_gles.h b/engine/src/flutter/impeller/renderer/backend/gles/proc_table_gles.h index b440f5a73e9..bfbf32fef2e 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/proc_table_gles.h +++ b/engine/src/flutter/impeller/renderer/backend/gles/proc_table_gles.h @@ -104,6 +104,7 @@ struct GLProc { PROC(BlendEquationSeparate); \ PROC(BlendFuncSeparate); \ PROC(BufferData); \ + PROC(BufferSubData); \ PROC(CheckFramebufferStatus); \ PROC(Clear); \ PROC(ClearColor); \