From e40835cb89c791251258d1ff803e696f68d9f6bb Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Mon, 2 Dec 2024 10:46:29 -0800 Subject: [PATCH] [impeller] makes UniformBindData 15% faster and adds unit test (flutter/engine#56844) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit issue: https://github.com/flutter/flutter/issues/159177 The improvements come from using absl::flat_hash_map. measurements with std vs absl (`host_profile_arm64`): ``` # Std 0.041124 0.0398002 0.0396546 0.0405552 0.0397742 # Absl 0.0353708 0.0336834 0.0338616 0.033050 0.0331976 ``` Benchmark used ```c++ TEST(BufferBindingsGLESTest, BindUniformDataMicro) { BufferBindingsGLES bindings; absl::flat_hash_map uniform_bindings; uniform_bindings["SHADERMETADATA.FOOBAR"] = 1; bindings.SetUniformBindings(std::move(uniform_bindings)); std::shared_ptr mock_gl = MockGLES::Init(); MockAllocator allocator; Bindings vertex_bindings; ShaderMetadata shader_metadata = { .name = "shader_metadata", .members = {ShaderStructMemberMetadata{.type = ShaderType::kFloat, .name = "foobar", .offset = 0, .size = sizeof(float), .byte_length = sizeof(float)}}}; std::shared_ptr reactor; std::shared_ptr backing_store = std::make_shared(); ASSERT_TRUE(backing_store->Truncate(Bytes{sizeof(float)})); DeviceBufferGLES device_buffer(DeviceBufferDescriptor{.size = sizeof(float)}, reactor, backing_store); BufferView buffer_view(&device_buffer, Range(0, sizeof(float))); vertex_bindings.buffers.push_back(BufferAndUniformSlot{ .slot = ShaderUniformSlot{ .name = "foobar", .ext_res_0 = 0, .set = 0, .binding = 0}, .view = BufferResource(&shader_metadata, buffer_view)}); Bindings fragment_bindings; int32_t count = 5'000'000; auto start = std::chrono::high_resolution_clock::now(); for (int32_t i = 0; i < count; ++i) { bindings.BindUniformData(mock_gl->GetProcTable(), allocator, vertex_bindings, fragment_bindings); if (i % 100 == 0) { mock_gl->GetCapturedCalls(); } } auto end = std::chrono::high_resolution_clock::now(); auto duration = std::chrono::duration_cast(end - start) .count(); std::cout << "Execution time: " << duration / static_cast(count) << " microseconds" << std::endl; } ``` This is one of our hottest symbols on the raster thread: Screenshot 2024-11-27 at 2 27 54 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. - [x] I added new tests to check the change I am making or feature I am adding, or 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 [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [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 --- .../flutter/ci/licenses_golden/excluded_files | 1 + .../impeller/renderer/backend/gles/BUILD.gn | 1 + .../backend/gles/buffer_bindings_gles.cc | 8 ++- .../backend/gles/buffer_bindings_gles.h | 21 +++++--- .../gles/buffer_bindings_gles_unittests.cc | 50 +++++++++++++++++++ .../renderer/backend/gles/render_pass_gles.cc | 1 - .../renderer/backend/gles/test/mock_gles.cc | 9 ++++ 7 files changed, 79 insertions(+), 12 deletions(-) create mode 100644 engine/src/flutter/impeller/renderer/backend/gles/buffer_bindings_gles_unittests.cc diff --git a/engine/src/flutter/ci/licenses_golden/excluded_files b/engine/src/flutter/ci/licenses_golden/excluded_files index 955d08c7c3..ccc34d3b4f 100644 --- a/engine/src/flutter/ci/licenses_golden/excluded_files +++ b/engine/src/flutter/ci/licenses_golden/excluded_files @@ -182,6 +182,7 @@ ../../../flutter/impeller/geometry/trig_unittests.cc ../../../flutter/impeller/golden_tests/README.md ../../../flutter/impeller/playground +../../../flutter/impeller/renderer/backend/gles/buffer_bindings_gles_unittests.cc ../../../flutter/impeller/renderer/backend/gles/test ../../../flutter/impeller/renderer/backend/metal/allocator_mtl_unittests.mm ../../../flutter/impeller/renderer/backend/metal/texture_mtl_unittests.mm diff --git a/engine/src/flutter/impeller/renderer/backend/gles/BUILD.gn b/engine/src/flutter/impeller/renderer/backend/gles/BUILD.gn index f0b53263a4..13ffcd8f71 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/BUILD.gn +++ b/engine/src/flutter/impeller/renderer/backend/gles/BUILD.gn @@ -14,6 +14,7 @@ config("gles_config") { impeller_component("gles_unittests") { testonly = true sources = [ + "buffer_bindings_gles_unittests.cc", "test/capabilities_unittests.cc", "test/formats_gles_unittests.cc", "test/gpu_tracer_gles_unittests.cc", diff --git a/engine/src/flutter/impeller/renderer/backend/gles/buffer_bindings_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/buffer_bindings_gles.cc index 9deb66fe81..9ad2339572 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/buffer_bindings_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/buffer_bindings_gles.cc @@ -180,16 +180,15 @@ bool BufferBindingsGLES::BindVertexAttributes(const ProcTableGLES& gl, } bool BufferBindingsGLES::BindUniformData(const ProcTableGLES& gl, - Allocator& transients_allocator, const Bindings& vertex_bindings, const Bindings& fragment_bindings) { for (const auto& buffer : vertex_bindings.buffers) { - if (!BindUniformBuffer(gl, transients_allocator, buffer.view)) { + if (!BindUniformBuffer(gl, buffer.view)) { return false; } } for (const auto& buffer : fragment_bindings.buffers) { - if (!BindUniformBuffer(gl, transients_allocator, buffer.view)) { + if (!BindUniformBuffer(gl, buffer.view)) { return false; } } @@ -262,7 +261,7 @@ const std::vector& BufferBindingsGLES::ComputeUniformLocations( size_t element_count = member.array_elements.value_or(1); const std::string member_key = CreateUniformMemberKey(metadata->name, member.name, element_count > 1); - const std::unordered_map::iterator computed_location = + const absl::flat_hash_map::iterator computed_location = uniform_locations_.find(member_key); if (computed_location == uniform_locations_.end()) { // Uniform was not active. @@ -275,7 +274,6 @@ const std::vector& BufferBindingsGLES::ComputeUniformLocations( } bool BufferBindingsGLES::BindUniformBuffer(const ProcTableGLES& gl, - Allocator& transients_allocator, const BufferResource& buffer) { const ShaderMetadata* metadata = buffer.GetMetadata(); const DeviceBuffer* device_buffer = buffer.resource.GetBuffer(); diff --git a/engine/src/flutter/impeller/renderer/backend/gles/buffer_bindings_gles.h b/engine/src/flutter/impeller/renderer/backend/gles/buffer_bindings_gles.h index 65642d8db3..07bae42d49 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/buffer_bindings_gles.h +++ b/engine/src/flutter/impeller/renderer/backend/gles/buffer_bindings_gles.h @@ -8,6 +8,7 @@ #include #include +#include "flutter/third_party/abseil-cpp/absl/container/flat_hash_map.h" #include "impeller/core/shader_types.h" #include "impeller/renderer/backend/gles/gles.h" #include "impeller/renderer/backend/gles/proc_table_gles.h" @@ -15,6 +16,10 @@ namespace impeller { +namespace testing { +FML_TEST_CLASS(BufferBindingsGLESTest, BindUniformData); +} // namespace testing + //------------------------------------------------------------------------------ /// @brief Sets up stage bindings for single draw call in the OpenGLES /// backend. @@ -37,13 +42,13 @@ class BufferBindingsGLES { size_t vertex_offset); bool BindUniformData(const ProcTableGLES& gl, - Allocator& transients_allocator, const Bindings& vertex_bindings, const Bindings& fragment_bindings); bool UnbindVertexAttributes(const ProcTableGLES& gl); private: + FML_FRIEND_TEST(testing::BufferBindingsGLESTest, BindUniformData); //---------------------------------------------------------------------------- /// @brief The arguments to glVertexAttribPointer. /// @@ -57,9 +62,9 @@ class BufferBindingsGLES { }; std::vector> vertex_attrib_arrays_; - std::unordered_map uniform_locations_; + absl::flat_hash_map uniform_locations_; - using BindingMap = std::unordered_map>; + using BindingMap = absl::flat_hash_map>; BindingMap binding_map_ = {}; GLuint vertex_array_object_ = 0; @@ -68,9 +73,7 @@ class BufferBindingsGLES { GLint ComputeTextureLocation(const ShaderMetadata* metadata); - bool BindUniformBuffer(const ProcTableGLES& gl, - Allocator& transients_allocator, - const BufferResource& buffer); + bool BindUniformBuffer(const ProcTableGLES& gl, const BufferResource& buffer); std::optional BindTextures(const ProcTableGLES& gl, const Bindings& bindings, @@ -80,6 +83,12 @@ class BufferBindingsGLES { BufferBindingsGLES(const BufferBindingsGLES&) = delete; BufferBindingsGLES& operator=(const BufferBindingsGLES&) = delete; + + // For testing. + void SetUniformBindings( + absl::flat_hash_map uniform_locations) { + uniform_locations_ = std::move(uniform_locations); + } }; } // namespace impeller diff --git a/engine/src/flutter/impeller/renderer/backend/gles/buffer_bindings_gles_unittests.cc b/engine/src/flutter/impeller/renderer/backend/gles/buffer_bindings_gles_unittests.cc new file mode 100644 index 0000000000..1f7721c23f --- /dev/null +++ b/engine/src/flutter/impeller/renderer/backend/gles/buffer_bindings_gles_unittests.cc @@ -0,0 +1,50 @@ +// 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 "flutter/testing/testing.h" // IWYU pragma: keep +#include "gtest/gtest.h" +#include "impeller/renderer/backend/gles/buffer_bindings_gles.h" +#include "impeller/renderer/backend/gles/device_buffer_gles.h" +#include "impeller/renderer/backend/gles/test/mock_gles.h" +#include "impeller/renderer/testing/mocks.h" + +namespace impeller { +namespace testing { + +TEST(BufferBindingsGLESTest, BindUniformData) { + BufferBindingsGLES bindings; + absl::flat_hash_map uniform_bindings; + uniform_bindings["SHADERMETADATA.FOOBAR"] = 1; + bindings.SetUniformBindings(std::move(uniform_bindings)); + std::shared_ptr mock_gl = MockGLES::Init(); + Bindings vertex_bindings; + + ShaderMetadata shader_metadata = { + .name = "shader_metadata", + .members = {ShaderStructMemberMetadata{.type = ShaderType::kFloat, + .name = "foobar", + .offset = 0, + .size = sizeof(float), + .byte_length = sizeof(float)}}}; + std::shared_ptr reactor; + std::shared_ptr backing_store = std::make_shared(); + ASSERT_TRUE(backing_store->Truncate(Bytes{sizeof(float)})); + DeviceBufferGLES device_buffer(DeviceBufferDescriptor{.size = sizeof(float)}, + reactor, backing_store); + BufferView buffer_view(&device_buffer, Range(0, sizeof(float))); + vertex_bindings.buffers.push_back(BufferAndUniformSlot{ + .slot = + ShaderUniformSlot{ + .name = "foobar", .ext_res_0 = 0, .set = 0, .binding = 0}, + .view = BufferResource(&shader_metadata, buffer_view)}); + Bindings fragment_bindings; + EXPECT_TRUE(bindings.BindUniformData(mock_gl->GetProcTable(), vertex_bindings, + fragment_bindings)); + std::vector captured_calls = mock_gl->GetCapturedCalls(); + EXPECT_TRUE(std::find(captured_calls.begin(), captured_calls.end(), + "glUniform1fv") != captured_calls.end()); +} + +} // namespace testing +} // namespace impeller 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 594fcfab07..c172b29fe3 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 @@ -435,7 +435,6 @@ void RenderPassGLES::ResetGLState(const ProcTableGLES& gl) { /// Bind uniform data. /// if (!vertex_desc_gles->BindUniformData(gl, // - *transients_allocator, // command.vertex_bindings, // command.fragment_bindings // )) { diff --git a/engine/src/flutter/impeller/renderer/backend/gles/test/mock_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/test/mock_gles.cc index 7a8399cede..e94e74dc28 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/test/mock_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/test/mock_gles.cc @@ -167,6 +167,13 @@ void mockDeleteTextures(GLsizei size, const GLuint* queries) { static_assert(CheckSameSignature::value); +void mockUniform1fv(GLint location, GLsizei count, const GLfloat* value) { + RecordGLCall("glUniform1fv"); +} + +static_assert(CheckSameSignature::value); + std::shared_ptr MockGLES::Init( const std::optional>& extensions, const char* version_string, @@ -208,6 +215,8 @@ const ProcTableGLES::Resolver kMockResolverGLES = [](const char* name) { return reinterpret_cast(mockGetQueryObjectui64vEXT); } else if (strcmp(name, "glGetQueryObjectuivEXT") == 0) { return reinterpret_cast(mockGetQueryObjectuivEXT); + } else if (strcmp(name, "glUniform1fv") == 0) { + return reinterpret_cast(mockUniform1fv); } else { return reinterpret_cast(&doNothing); }