From b23720bf683147f67816af591037b0d9dc3f2ea3 Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Sat, 23 Nov 2024 14:14:52 -0800 Subject: [PATCH] Smaller reactor handle locks (flutter/engine#56768) This reduces the amount of work that is happening inside the read/write mutex for tracking handles in the Reactor. 1) Now gl cleanup calls happen outside of the write lock 1) Now setting gl debug texts are out of the write lock 1) Now creating objects with CreateHandle are out of the write lock Jonah was seeing considerable time in the rwmutex, this should alleviate some of that. ![setlabel](https://github.com/user-attachments/assets/6ebe247b-76f4-457f-b28b-70ddcd5615a8) issue: https://github.com/flutter/flutter/issues/159177 tests: no functional change, just performance ## 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 --- .../renderer/backend/gles/reactor_gles.cc | 74 +++++++++++-------- 1 file changed, 45 insertions(+), 29 deletions(-) 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 82266bfd998..c72f0d4af83 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc @@ -203,7 +203,6 @@ HandleGLES ReactorGLES::CreateHandle(HandleType type, GLuint external_handle) { if (new_handle.IsDead()) { return HandleGLES::DeadHandle(); } - WriterLock handles_lock(handles_mutex_); std::optional gl_handle; if (external_handle != GL_NONE) { @@ -211,6 +210,8 @@ HandleGLES ReactorGLES::CreateHandle(HandleType type, GLuint external_handle) { } else if (CanReactOnCurrentThread()) { gl_handle = CreateGLHandle(GetProcTable(), type); } + + WriterLock handles_lock(handles_mutex_); handles_[new_handle] = LiveHandle{gl_handle}; return new_handle; } @@ -266,41 +267,56 @@ bool ReactorGLES::ReactOnce() { bool ReactorGLES::ConsolidateHandles() { TRACE_EVENT0("impeller", __FUNCTION__); const auto& gl = GetProcTable(); - WriterLock handles_lock(handles_mutex_); - std::vector handles_to_delete; - for (auto& handle : handles_) { - // Collect dead handles. - if (handle.second.pending_collection) { - // This could be false if the handle was created and collected without - // use. We still need to get rid of map entry. - if (handle.second.name.has_value()) { - CollectGLHandle(gl, handle.first.type, handle.second.name.value()); + std::vector>> + handles_to_delete; + std::vector> + handles_to_name; + { + WriterLock handles_lock(handles_mutex_); + for (auto& handle : handles_) { + // Collect dead handles. + if (handle.second.pending_collection) { + handles_to_delete.push_back( + std::make_tuple(handle.first, handle.second.name)); + continue; } - handles_to_delete.push_back(handle.first); - continue; - } - // Create live handles. - if (!handle.second.name.has_value()) { - auto gl_handle = CreateGLHandle(gl, handle.first.type); - if (!gl_handle) { - VALIDATION_LOG << "Could not create GL handle."; - return false; + // Create live handles. + if (!handle.second.name.has_value()) { + auto gl_handle = CreateGLHandle(gl, handle.first.type); + if (!gl_handle) { + VALIDATION_LOG << "Could not create GL handle."; + return false; + } + handle.second.name = gl_handle; } - handle.second.name = gl_handle; - } - // Set pending debug labels. - if (handle.second.pending_debug_label.has_value() && - handle.first.type != HandleType::kFence) { - if (gl.SetDebugLabel(ToDebugResourceType(handle.first.type), - handle.second.name.value().handle, - handle.second.pending_debug_label.value())) { + // Set pending debug labels. + if (handle.second.pending_debug_label.has_value() && + handle.first.type != HandleType::kFence) { + handles_to_name.push_back(std::make_tuple( + ToDebugResourceType(handle.first.type), + handle.second.name.value().handle, + std::move(handle.second.pending_debug_label.value()))); handle.second.pending_debug_label = std::nullopt; } } + for (const auto& handle_to_delete : handles_to_delete) { + handles_.erase(std::get<0>(handle_to_delete)); + } } - for (const auto& handle_to_delete : handles_to_delete) { - handles_.erase(handle_to_delete); + + for (const auto& handle : handles_to_name) { + gl.SetDebugLabel(std::get<0>(handle), std::get<1>(handle), + std::get<2>(handle)); } + for (const auto& handle : handles_to_delete) { + const std::optional& storage = std::get<1>(handle); + // This could be false if the handle was created and collected without + // use. We still need to get rid of map entry. + if (storage.has_value()) { + CollectGLHandle(gl, std::get<0>(handle).type, storage.value()); + } + } + return true; }