[Impeller] Give Impeller a dedicated raster priority level worker loop. #43166
Conversation
| is_gpu_disabled_sync_switch_(std::move(is_gpu_disabled_sync_switch)) { | ||
| // Validate device. | ||
| if (!device_) { | ||
| VALIDATION_LOG << "Could not setup valid Metal device."; | ||
| return; | ||
| } | ||
|
|
||
| // Worker task runner. | ||
| { | ||
| raster_message_loop_ = fml::ConcurrentMessageLoop::Create(4u); |
There was a problem hiding this comment.
Oh, so when were were doing the concurrent render pass encoding on iOS, we were pinging the worker before? Oof...
| pthread_t thread = pthread_self(); | ||
| if (!pthread_getschedparam(thread, &policy, ¶m)) { | ||
| param.sched_priority = 50; | ||
| pthread_setschedparam(thread, policy, ¶m); |
There was a problem hiding this comment.
Are both NSThread and pthread priority calls necessary? I expected only the first on to be a thing on iOS.
There was a problem hiding this comment.
This reminds me that we should probably use libDispatch with the right QoS classes on iOS. The platform will create the right number and nature of workers then. We had prototyped this on iOS in this patch but backed it out. Perhaps we can resuscitate it.
| } | ||
| worker_task_runner_ = settings.worker_task_runner; | ||
| raster_message_loop_ = fml::ConcurrentMessageLoop::Create(4u); | ||
| #ifdef FML_OS_ANDROID |
There was a problem hiding this comment.
IMO, we should isolate all the platform specific bits in at least in //impeller/base (in a later patch is fine).
|
The thread joining triggered by the concurrent message loop destructor is failing in the an aiks test. |
|
xref flutter/flutter#129392 which may be related. |
|
Ahh, so the issue is that the playgrounds will explicitly release the ContextMTL, but if there is an inflight async task this will hold onto the context. When this shared_ptr is freed, then the context will be deleted on a worker thread. This would be safe if not for the concurrent message loop, as it will try to join all threads to end the loop - but it would end up joining into itself. So there are a few possible options to fix this:
|
|
1 and 3 seem quite difficult so I'm going to look at 2 |
|
I guess another option would be to make the shell or dartvm own this message loop (separate from the worker loop) and plumb that one through instead. |
| @@ -139,6 +139,9 @@ void Playground::SetupWindow() { | |||
| } | |||
|
|
|||
| void Playground::TeardownWindow() { | |||
| if (context_) { | |||
There was a problem hiding this comment.
This is my hacky take on making sure the concurrent message loop is torn down on the raster thread and not a worker thread
| @@ -29,6 +29,7 @@ ConcurrentMessageLoop::ConcurrentMessageLoop(size_t worker_count) | |||
| ConcurrentMessageLoop::~ConcurrentMessageLoop() { | |||
| Terminate(); | |||
| for (auto& worker : workers_) { | |||
| FML_DCHECK(worker.joinable()); | |||
There was a problem hiding this comment.
This makes the failure case a bit more obvious. if you try to join a worker thread into itself then it will fail with an errorcode that may or may not be obvious without a debugger attached.
jonahwilliams
added
the
autosubmit
…129678) flutter/engine@f320b8c...7c7c45d 2023-06-27 flar@google.com Update skia includes to be more specific (flutter/engine#43284) 2023-06-27 yjbanov@google.com [web:a11y] introduce primary role responsible for ARIA roles (flutter/engine#43159) 2023-06-27 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from ytzCCSvHY1lHWEDM9... to sBFKkha8HNLZpTNwv... (flutter/engine#43277) 2023-06-27 skia-flutter-autoroll@skia.org Roll ANGLE from 9faf7059f9ef to 113f847be69f (2 revisions) (flutter/engine#43278) 2023-06-27 jacksongardner@google.com Initialize skwasm codecs before handing them back to the user. (flutter/engine#43274) 2023-06-27 skia-flutter-autoroll@skia.org Roll ANGLE from 02292814a9d3 to 9faf7059f9ef (7 revisions) (flutter/engine#43272) 2023-06-27 15619084+vashworth@users.noreply.github.com Update Xcode to 14.3.1 (flutter/engine#42930) 2023-06-27 jonahwilliams@google.com [Impeller] Add Vulkan allocator traces. (flutter/engine#43215) 2023-06-27 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from bj_X2Se1zObk_l_CC... to Bvv7TyHm_VHUkndFx... (flutter/engine#43270) 2023-06-27 chinmaygarde@google.com [Impeller] Report pipeline creation feedback to logs and traces. (flutter/engine#43227) 2023-06-27 jonahwilliams@google.com [Impeller] Give Impeller a dedicated raster priority level worker loop. (flutter/engine#43166) 2023-06-27 jason-simmons@users.noreply.github.com [Impeller] Fixes for GLES color mask setup (flutter/engine#43225) 2023-06-27 skia-flutter-autoroll@skia.org Roll ANGLE from cba77bceb26c to 02292814a9d3 (1 revision) (flutter/engine#43224) 2023-06-27 skia-flutter-autoroll@skia.org Roll Skia from 370132bcadb1 to 5209dc7702d0 (1 revision) (flutter/engine#43223) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from bj_X2Se1zObk to Bvv7TyHm_VHU fuchsia/sdk/core/mac-amd64 from ytzCCSvHY1lH to sBFKkha8HNLZ If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jimgraham@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…p. (flutter#43166) We'd like to (or already are) using the concurrent message loop for high priority rendering tasks like PSO construction and render pass encoding. The default priority level for the engine managed concurrent message loop is 2, which is a significantly lower priority than the raster thread at -5. This is almost certainly causing priority inversion. We must move back to dedicated runners so we can adjust thread priorities.
We'd like to (or already are) using the concurrent message loop for high priority rendering tasks like PSO construction and render pass encoding. The default priority level for the engine managed concurrent message loop is 2, which is a significantly lower priority than the raster thread at -5. This is almost certainly causing priority inversion.
We must move back to dedicated runners so we can adjust thread priorities.