-
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] defer construction of VkCommandBuffer until encoding begins. #43605
[Impeller] defer construction of VkCommandBuffer until encoding begins. #43605
Conversation
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.
How will the CommandEncoderVK
be passed to the worker threads when using concurrent encoding?
If there is a place where the worker receives the encoder before using it, then it might make sense to instead pass a factory object. The worker can then have the factory construct an encoder that is initialized for use on that thread. This will make a clearer separation between the factory (which does not have thread affinity) and the encoder (which does).
} | ||
device_holder_ = device_holder; | ||
queue_ = queue; | ||
: fence_waiter_(std::move(fence_waiter)), context_(context) { | ||
is_valid_ = true; |
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 flag is now always true and can be removed.
std::shared_ptr<FenceWaiterVK> fence_waiter_; | ||
std::shared_ptr<TrackedObjectsVK> tracked_objects_; | ||
std::weak_ptr<const ContextVK> context_; | ||
mutable std::shared_ptr<QueueVK> queue_; |
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.
Remove the const
qualifier from the affected methods instead of making these members mutable.
My idea, which hopefully works, is to do a rough analog of what the mtl command buffer is doing: That is we'll move the impeller::CommandBuffer and impeller:RenderPass to a worker thread, then invoke impeller::RenderPass::Encode which will in turn access CommandEncoderVK to perform the actual encoding. |
auto command_buffer = command_buffer_.lock(); | ||
if (!command_buffer) { | ||
return false; | ||
} | ||
auto encoder = command_buffer->GetEncoder(); |
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've updated RenderPass/BlitPass/And ComputePass to be non-const so that the first time command_buffer->GetEncoder() is called we construct the VkCommandEncoder.
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 done via a factory class that is constructed by vkContext and held by the command buffer. When the encoder is created lazily it should already be in a valid state.
…130626) flutter/engine@21b8e68...403866d 2023-07-14 jonahwilliams@google.com [Impeller] defer construction of VkCommandBuffer until encoding begins. (flutter/engine#43605) 2023-07-14 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from LZPMbHnVPFdbXndcX... to DEENqWMCYI1SMYsYH... (flutter/engine#43702) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from LZPMbHnVPFdb to DEENqWMCYI1S 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 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
…s. (flutter#43605) VkCommandBuffers are allocated out of a thread local VkCommandPool. THis command buffer is only safe to use on the thread which allocates it. We'd like to attempt concurrent encoding on Vulkan like we've done for iOS, which means the worker threads will be accessing the VkCommandBuffer. In order for this to be safe, the VkCommandBuffer must be allocated out of a VkCommandPool from that worker thread. As a simple fix, we make the Impeller CommandBuffer defer construction of the VkCommandBuffer object, allowing us to record impeller commands on the raster thread, move the command buffer to a worker thread, create the VkCommandBuffer, and then begin encoding.
…lutter#130626) flutter/engine@21b8e68...403866d 2023-07-14 jonahwilliams@google.com [Impeller] defer construction of VkCommandBuffer until encoding begins. (flutter/engine#43605) 2023-07-14 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from LZPMbHnVPFdbXndcX... to DEENqWMCYI1SMYsYH... (flutter/engine#43702) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from LZPMbHnVPFdb to DEENqWMCYI1S 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 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
VkCommandBuffers are allocated out of a thread local VkCommandPool. THis command buffer is only safe to use on the thread which allocates it.
We'd like to attempt concurrent encoding on Vulkan like we've done for iOS, which means the worker threads will be accessing the VkCommandBuffer. In order for this to be safe, the VkCommandBuffer must be allocated out of a VkCommandPool from that worker thread. As a simple fix, we make the Impeller CommandBuffer defer construction of the VkCommandBuffer object, allowing us to record impeller commands on the raster thread, move the command buffer to a worker thread, create the VkCommandBuffer, and then begin encoding.