-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Impeller] Encode render passes concurrently on iOS. #42028
Changes from 3 commits
93b6a9c
7c9382d
e122d32
23ecc12
69ac789
b49f056
18ef717
71369e4
2648dd2
2821932
b2515af
83b0b90
1aed386
5198c2c
56c1687
929f0bc
b843862
3df1b91
07ab4e9
9670639
60b42be
9d1a0cc
ff058f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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,53 @@ static bool LogMTLCommandBufferErrorIfPresent(id<MTLCommandBuffer> buffer) { | |
return true; | ||
} | ||
|
||
bool CommandBufferMTL::SubmitCommandsAsync( | ||
std::shared_ptr<RenderPass> render_pass) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we not bump the stack size limit for worker threads? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How can I try that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, context]() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we instead capture the weak_ptr to context and make sure it's still lockable in the callback here? The advantage being that if the context has otherwise gone away, we avoid doing work. Otherwise, I think we might need to have something about the GPU sync switch in here to make sure we're not doing encoding work when the GPU is unavailable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, capturing the context seems reasonable. I'm not sure what the buffer itself will do, I guess we should probably assume it won't handle anything gracefully. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
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( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -179,6 +179,7 @@ | |
} | ||
} | ||
|
||
#if ((FML_OS_MACOSX && !FML_OS_IOS) || FML_OS_IOS_SIMULATOR) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I observed using wonderous that the waitUntilScheduled was only necessary on simulator and (speculatively) on macOS There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that with this patch or without? If you've observed this without the changes in this patch, can we move this to a separate patch? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I can split this out. Though we need both of these changes to see much of an improvement, otherwise the final scheduling just takes longer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved here: #42160 |
||
// If a transaction is present, `presentDrawable` will present too early. And | ||
// so we wait on an empty command buffer to get scheduled instead, which | ||
// forces us to also wait for all of the previous command buffers in the queue | ||
|
@@ -187,6 +188,8 @@ | |
ContextMTL::Cast(context.get())->CreateMTLCommandBuffer(); | ||
[command_buffer commit]; | ||
[command_buffer waitUntilScheduled]; | ||
#endif // (FML_OS_MACOSX && !FML_OS_IOS) || FML_OS_IOS_SIMULATOR) (not iPhone) | ||
|
||
[drawable_ present]; | ||
|
||
return true; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,8 @@ | |
|
||
FLUTTER_ASSERT_ARC | ||
|
||
static std::shared_ptr<impeller::ContextMTL> CreateImpellerContext() { | ||
static std::shared_ptr<impeller::ContextMTL> CreateImpellerContext( | ||
const std::shared_ptr<fml::ConcurrentMessageLoop>& concurrent_loop) { | ||
std::vector<std::shared_ptr<fml::Mapping>> shader_mappings = { | ||
std::make_shared<fml::NonOwnedMapping>(impeller_entity_shaders_data, | ||
impeller_entity_shaders_length), | ||
|
@@ -27,7 +28,9 @@ | |
std::make_shared<fml::NonOwnedMapping>(impeller_framebuffer_blend_shaders_data, | ||
impeller_framebuffer_blend_shaders_length), | ||
}; | ||
auto context = impeller::ContextMTL::Create(shader_mappings, "Impeller Library"); | ||
auto worker_task_runner = concurrent_loop->GetTaskRunner(); | ||
auto context = | ||
impeller::ContextMTL::Create(shader_mappings, worker_task_runner, "Impeller Library"); | ||
if (!context) { | ||
FML_LOG(ERROR) << "Could not create Metal Impeller Context."; | ||
return nullptr; | ||
|
@@ -42,7 +45,8 @@ @implementation FlutterDarwinContextMetalImpeller | |
- (instancetype)init { | ||
self = [super init]; | ||
if (self != nil) { | ||
_context = CreateImpellerContext(); | ||
_workers = fml::ConcurrentMessageLoop::Create(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't create a message loop here. Instead, we should be getting the concurrent loop the engine creates already and passing it in as a parameter here. That means we should mark the default initializer as unavailable and make a new one like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure exactly how to do this. It looks like ImpellerContext is setup without any references to the current engine. Is this something we can/should change, or should I look at providing the message loop some other way? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it'd flow through Looks like there's something left to figure out there though and we're not doing this on Android right now (we create a special concurrent loop for the context to use there in Maybe for now we can file a bug for this. We shouldn't really be creating multiple concurrent message loops. But there might need to be some refactoring in the shell/platformview/engine setup to make sure that ownership and sharing of the loop is handled properly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed flutter/flutter#127160. I'm ambivalent about whether resolving that blocks this or not, but if it turns out to be not too hard to resolve that then we should do it before creating another violating of it here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should figure out the concurrent loop stuff first. |
||
_context = CreateImpellerContext(_workers); | ||
id<MTLDevice> device = _context->GetMTLDevice(); | ||
if (!device) { | ||
FML_DLOG(ERROR) << "Could not acquire Metal device."; | ||
|
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.
ContextVK
takes 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.