[Impeller] Encode render passes concurrently on iOS. #42028
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,13 @@ | |
|
|
||
| #include "impeller/renderer/backend/metal/command_buffer_mtl.h" | ||
|
|
||
| #include "flutter/fml/make_copyable.h" | ||
| #include "flutter/fml/synchronization/semaphore.h" | ||
| #include "flutter/fml/trace_event.h" | ||
|
|
||
| #include "impeller/renderer/backend/metal/blit_pass_mtl.h" | ||
| #include "impeller/renderer/backend/metal/compute_pass_mtl.h" | ||
| #include "impeller/renderer/backend/metal/context_mtl.h" | ||
| #include "impeller/renderer/backend/metal/render_pass_mtl.h" | ||
|
|
||
| namespace impeller { | ||
|
|
@@ -171,6 +176,58 @@ static bool LogMTLCommandBufferErrorIfPresent(id<MTLCommandBuffer> buffer) { | |
| return true; | ||
| } | ||
|
|
||
| bool CommandBufferMTL::SubmitCommandsAsync( | ||
| std::shared_ptr<RenderPass> render_pass) { | ||
|
There was a problem hiding this comment. Currently this only works for the render pass but could optionally take the blit and compute passes too. I moved this functionality to the command buffer to make ownership of the render pass data more obvious - the closure keeps the shared_ptr for the render pass alive while the command buffer itself only requires the buffer. |
||
| TRACE_EVENT0("impeller", "CommandBufferMTL::SubmitCommandsAsync"); | ||
| if (!IsValid() || !render_pass->IsValid()) { | ||
| return false; | ||
| } | ||
| auto context = context_.lock(); | ||
|
There was a problem hiding this comment. I think this is more or less what ended up being the fix for problems around ownership in Vulkan backend. |
||
| if (!context) { | ||
| return false; | ||
| } | ||
| [buffer_ enqueue]; | ||
|
There was a problem hiding this comment. enquing on the main therad ensures the order |
||
| auto buffer = buffer_; | ||
| buffer_ = nil; | ||
|
|
||
| auto worker_task_runner = ContextMTL::Cast(*context).GetWorkerTaskRunner(); | ||
|
|
||
| auto mtl_render_pass = static_cast<RenderPassMTL*>(render_pass.get()); | ||
|
|
||
| // Render command encoder creation has been observed to exceed the stack size | ||
| // limit for worker threads, and therefore is intentionally constructed on the | ||
| // raster thread. | ||
|
There was a problem hiding this comment. Can we not bump the stack size limit for worker threads? There was a problem hiding this comment. How can I try that? There was a problem hiding this comment. It's platform specific code, we'd need a pthreads and a win32 implementation at the least. |
||
| auto render_command_encoder = | ||
|
There was a problem hiding this comment. This was a frustrating discovery because it prevents us from doing the obvious thing and just calling RenderPass::Encode in the closure. |
||
| [buffer renderCommandEncoderWithDescriptor:mtl_render_pass->desc_]; | ||
| if (!render_command_encoder) { | ||
| return false; | ||
| } | ||
|
|
||
| auto task = fml::MakeCopyable( | ||
| [render_pass, buffer, render_command_encoder, weak_context = context_]() { | ||
| auto context = weak_context.lock(); | ||
| if (!context) { | ||
| return; | ||
|
There was a problem hiding this comment. I'm realizing this will not protect us from potentially encoding command in the background. We need the gpu sync switch in here so we can fizzle if GPU access is disabled. That's available via There was a problem hiding this comment. which operations are forbidden in the background? Is just submitting the cmd buffer? I'm curious what stops us from doing that now on the single raster thread There was a problem hiding this comment. (that is, do we need to use the sync switch elsewhere too?) There was a problem hiding this comment. Committing encoding work is forbidden: https://developer.apple.com/documentation/metal/gpu_devices_and_work_submission/preparing_your_metal_app_to_run_in_the_background?language=objc Before this change, the work would all be guarded by the rasterizer - but now, spawning new threads, it's harder to predict how scheduling will occur and the rasterizer may get to the end of the function it's calling but the task spawned to encode new work happens after. There was a problem hiding this comment. Specifically, At any rate, we can't trust that we're on the raster thread with the rasterizer having checked whether GPU access is allowed in this closure. |
||
| } | ||
|
|
||
| auto mtl_render_pass = static_cast<RenderPassMTL*>(render_pass.get()); | ||
| if (!mtl_render_pass->label_.empty()) { | ||
| [render_command_encoder setLabel:@(mtl_render_pass->label_.c_str())]; | ||
| } | ||
|
|
||
| auto result = mtl_render_pass->EncodeCommands( | ||
| context->GetResourceAllocator(), render_command_encoder); | ||
| [render_command_encoder endEncoding]; | ||
| if (result) { | ||
| [buffer commit]; | ||
| } else { | ||
| VALIDATION_LOG << "Failed to encode command buffer"; | ||
| } | ||
| }); | ||
| worker_task_runner->PostTask(task); | ||
| return true; | ||
| } | ||
|
|
||
| void CommandBufferMTL::OnWaitUntilScheduled() {} | ||
|
|
||
| std::shared_ptr<RenderPass> CommandBufferMTL::OnCreateRenderPass( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more or less duplicating the problem that the VKContext has wherein we should only be creating a single concurrent message loop once per engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jason-simmons @gaaclarke fyi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW We switched around ContextVK so that it is made to support sharing a concurrent message loop. There's no known problem. This came from Chinmay's design goal of wanting to share these. I'm not sure what that means for your PR, I'm just clarifying that point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look at ContextVK then, when I started this PR ContextVK was still creating its own message loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ContextVK is still creating its own concurrent message loop?
engine/shell/platform/android/android_context_vulkan_impeller.cc
Line 44 in 0ae3719
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ContextVKtakes in a shared_ptr to a task queue which can be shared across multiple engines, but currently is not:engine/impeller/renderer/backend/vulkan/context_vk.h
Line 43 in 0ae3719
I had a PR out that made ContextVK own a single concurrent message loop but we decided we didn't want to shut the door on sharing concurrent message loops between engines. I haven't read this PR through, I'm just responding to the comment "the problem that the VKContext has wherein we should only be creating a single concurrent message loop once per engine". There is no problem that I know of with Vulkan and we made sure of that with our recent work. Let me know if you want me review this PR if you think it would be helpful.