From d2e73aef5bc2a56a63eb6e958461b72f0f05460e Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Tue, 8 Oct 2024 13:19:42 -0700 Subject: [PATCH] Added mutex to the pending gpu tasks deque. (flutter/engine#55748) b/371513051 The tasks are already capturing their target thread. I had just missed guarding the deque that is storing the tasks. This has been seen in the flaking of the newly added `ShellTest.EncodeImageRetryOverflows`. Example: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8734621687934134433/+/u/test:_Host_Tests_for_host_debug_unopt/stdout ## 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/metal/context_mtl.h | 4 ++- .../renderer/backend/metal/context_mtl.mm | 34 ++++++++++++++----- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.h b/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.h index dbc3806939..15bb2fd46e 100644 --- a/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.h +++ b/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.h @@ -164,7 +164,9 @@ class ContextMTL final : public Context, std::shared_ptr resource_allocator_; std::shared_ptr device_capabilities_; std::shared_ptr is_gpu_disabled_sync_switch_; - std::deque tasks_awaiting_gpu_; + Mutex tasks_awaiting_gpu_mutex_; + std::deque tasks_awaiting_gpu_ IPLR_GUARDED_BY( + tasks_awaiting_gpu_mutex_); std::unique_ptr sync_switch_observer_; std::shared_ptr command_queue_ip_; #ifdef IMPELLER_DEBUG diff --git a/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm b/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm index 2d6c432e1b..0a66c82146 100644 --- a/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm +++ b/engine/src/flutter/impeller/renderer/backend/metal/context_mtl.mm @@ -381,21 +381,39 @@ id ContextMTL::CreateMTLCommandBuffer( void ContextMTL::StoreTaskForGPU(const fml::closure& task, const fml::closure& failure) { - tasks_awaiting_gpu_.push_back(PendingTasks{task, failure}); - while (tasks_awaiting_gpu_.size() > kMaxTasksAwaitingGPU) { - const PendingTasks& front = tasks_awaiting_gpu_.front(); - if (front.failure) { - front.failure(); + std::vector failed_tasks; + { + Lock lock(tasks_awaiting_gpu_mutex_); + tasks_awaiting_gpu_.push_back(PendingTasks{task, failure}); + int32_t failed_task_count = + tasks_awaiting_gpu_.size() - kMaxTasksAwaitingGPU; + if (failed_task_count > 0) { + failed_tasks.reserve(failed_task_count); + failed_tasks.insert(failed_tasks.end(), + std::make_move_iterator(tasks_awaiting_gpu_.begin()), + std::make_move_iterator(tasks_awaiting_gpu_.begin() + + failed_task_count)); + tasks_awaiting_gpu_.erase( + tasks_awaiting_gpu_.begin(), + tasks_awaiting_gpu_.begin() + failed_task_count); + } + } + for (const PendingTasks& task : failed_tasks) { + if (task.failure) { + task.failure(); } - tasks_awaiting_gpu_.pop_front(); } } void ContextMTL::FlushTasksAwaitingGPU() { - for (const auto& task : tasks_awaiting_gpu_) { + std::deque tasks_awaiting_gpu; + { + Lock lock(tasks_awaiting_gpu_mutex_); + std::swap(tasks_awaiting_gpu, tasks_awaiting_gpu_); + } + for (const auto& task : tasks_awaiting_gpu) { task.task(); } - tasks_awaiting_gpu_.clear(); } ContextMTL::SyncSwitchObserver::SyncSwitchObserver(ContextMTL& parent)