[Impeller] Add interface for submitting multiple command buffers at once. #50139

jonahwilliams · 2024-01-29T16:00:21Z

The Impeller Vulkan backend benefits from batching submission to the vk graphics queue. Managing this automatically is non-trivial and adds surprising/fragile thread based behavior, see: #49870

Instead, introduce an impeller::CommandQueue object that command buffers must be submitted to in lieu of CommandBuffer->Submit, which has been made private.

TLDR

old

buffer->Submit();

new

context.GetQueue()->Submit({buffer});

The Metal and GLES implementations internally just call the private CommandBuffer->Submit, though there may be future opportunities to simplify here. The Vulkan implementation is where the meat is.

Aiks takes advantage of this by storing all command buffers on the aiks context while rendering a frame, and then performing one submit in aiks_context render. I don't think this will introduce any thread safety problems, as we don't guarantee much about aiks context - nor do we use it in a multithreaded context as far as I know.

Other tasks such as image upload still just directly submit their command buffers via the queue.

Fixes flutter/flutter#141123

chinmaygarde

Approach looks good.

chinmaygarde · 2024-01-29T18:16:01Z

impeller/renderer/backend/vulkan/graphics_queue_vk.h

+                     const CompletionCallback& callback = {}) override;
+
+ private:
+  std::weak_ptr<ContextVK> context_;


Whatever FML_DISALLOW_COPY_ASSIGN_AND_MOVE expands to. Not having to depend on the base class for deletes avoid potential expensive copies in case that interface changes but we put an expensive to copy ivar here.

chinmaygarde · 2024-01-29T18:19:43Z

impeller/renderer/context.h

  virtual std::shared_ptr<CommandBuffer> CreateCommandBuffer() const = 0;

+  /// @brief Return the graphics queue for submitting command buffers.
+  virtual const std::shared_ptr<GraphicsQueue>& GetQueue() const = 0;


GetGraphicsQueue?

Pending resolution about the comment later about the class name.

Conflicts with context_vk.h GetGraphicsQueue for the QueueVK class.

chinmaygarde · 2024-01-29T18:21:13Z

impeller/renderer/graphics_queue.h

+ public:
+  using CompletionCallback = std::function<void(CommandBuffer::Status)>;
+
+  GraphicsQueue() = default;


chinmaygarde · 2024-01-29T18:21:57Z

impeller/renderer/graphics_queue.h

+  ///        The order of the provided buffers determines the ordering in which
+  ///        they are submitted.
+  ///
+  ///        Optionally accepts a ccallback that will fire with an updated


"callback" misspelled.

chinmaygarde · 2024-01-29T18:24:03Z

impeller/renderer/graphics_queue.h

+
+/// @brief An interface for submitting command buffers to the GPU for
+///        encoding and execution.
+class GraphicsQueue {


From the name, I was expecting this to be akin to the graphics vs compute vs transfer queues. But this doesn't seem to be the case. How about just calling this CommandQueue instead?

CommandQueue SGTM, solves naming problems too.

Renamed, other stuff WIP

chinmaygarde · 2024-01-29T18:32:39Z

impeller/renderer/graphics_queue.h

+  ///        they are submitted.
+  ///
+  ///        Optionally accepts a ccallback that will fire with an updated
+  ///        status based on encoding state. Only the Metal and Vulkan backend


Perhaps a better way of putting this would be "The status only indicates if the command buffer was successfully submitted. Successful completion of the command buffer can only be checked in the completion callback.".

So, if the status is not OK, can the caller expect a completion callback invocation? Otherwise they might have to perform cleanup themselves if the callback is dropped on the floor.

Hmm, a better interface might be to guarantee that the callback always gets invoked but with CommandBuffer::Status::kError in case of failure to submit as well. This function can return void. This avoids burden on the caller to check for error and invoke the callback themselves in case the callback has cleanup they want to perform.

That change would make it much more difficult to detect immediate submission errors

chinmaygarde · 2024-01-29T18:41:20Z

impeller/renderer/backend/vulkan/graphics_queue_vk.cc

+  if (!added_fence) {
+    return fml::Status(fml::StatusCode::kCancelled, "Failed to add fence.");
+  }
+  fail_callback = false;


fml::ScopedCleanupClosure has a Release call that you can invoke instead of tracking the success case in a var.

chinmaygarde · 2024-01-29T18:43:13Z

impeller/renderer/backend/vulkan/graphics_queue_vk.cc

+fml::Status GraphicsQueueVK::Submit(
+    const std::vector<std::shared_ptr<CommandBuffer>>& buffers,
+    const CompletionCallback& callback) {
+  if (buffers.empty()) {


Don't know what if the "you'll always get the callback no matter what" behavior is, asked for clarification in the headerdoc. But if it is the case, you might need a success callback invocation here.

chinmaygarde · 2024-01-29T18:44:49Z

impeller/renderer/backend/vulkan/graphics_queue_vk.cc

+  }
+
+  vk::SubmitInfo submit_info;
+  submit_info.setCommandBuffers(vk_buffers);


Good stuff.

chinmaygarde · 2024-01-29T18:48:27Z

impeller/renderer/backend/vulkan/graphics_queue_vk.cc

+      [callback, tracked_objects = std::move(tracked_objects)]() mutable {
+        // Ensure tracked objects are destructed before calling any final
+        // callbacks.
+        for (auto& tracked_object : tracked_objects) {


tracked_objects.clear() is the same as individually resetting the items in a loop.

bdero · 2024-01-29T20:58:23Z

lib/gpu/command_buffer.cc

  }
-  return command_buffer_->SubmitCommands();
+
+  return context_->GetCommandQueue()->Submit({command_buffer_}).ok();


So I'll just need to do this in a raster task to keep it safe, right?

this has the exact same threading guarantees as CommandBuffer->Submit. Its the AiksContext batching that is not necessarily thread safe.

jonahwilliams · 2024-01-29T23:07:12Z

I'm not quite sure how to fix this compilation error:

[7460/7463] LINK ./ui_unittests
FAILED: ui_unittests exe.unstripped/ui_unittests 
/b/s/w/ir/cache/goma/client/gomacc ../../buildtools/linux-x64/clang/bin/clang++ -Wl,--fatal-warnings -m64 -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -Wl,-z,defs -pthread -Wl,--undefined-version --sysroot=../../build/linux/debian_sid_amd64-sysroot  -Wl,-O2 -Wl,--gc-sections -Wl,--as-needed -Wl,-rpath=\$ORIGIN/ -Wl,-rpath-link= -Wl,--disable-new-dtags -Wl,--dynamic-list=../../flutter/common/exported_symbols.sym -rdynamic  -o ./exe.unstripped/ui_unittests -Wl,--build-id=sha1 -Wl,--start-group @./ui_unittests.rsp  -Wl,--end-group  -ldl -lgtk-3 -lgdk-3 -lpangocairo-1.0 -lpango-1.0 -latk-1.0 -lcairo-gobject -lcairo -lgdk_pixbuf-2.0 -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lpthread  && ../../buildtools/linux-x64/clang/bin/llvm-objcopy --strip-all ./exe.unstripped/ui_unittests ./ui_unittests
ld.lld: error: undefined symbol: std::_fl::shared_ptr<impeller::CommandQueue>::~shared_ptr[abi:v15000]()
>>> referenced by image_encoding_unittests.cc
>>>               obj/flutter/lib/ui/painting/ui_unittests.image_encoding_unittests.o:(std::_fl::__function::__func<flutter::testing::(anonymous namespace)::MakeConvertDlImageToSkImageContext(std::_fl::vector<unsigned char, std::_fl::allocator<unsigned char>>&)::$_0, std::_fl::allocator<flutter::testing::(anonymous namespace)::MakeConvertDlImageToSkImageContext(std::_fl::vector<unsigned char, std::_fl::allocator<unsigned char>>&)::$_0>, std::_fl::shared_ptr<impeller::CommandQueue> const& ()>::operator()())
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

jonahwilliams · 2024-01-30T07:02:08Z

this time its gonna work, I can feel it

…ffers at once. (flutter/engine#50139)

…142515) flutter/engine@438e9b4...0e586d1 2024-01-30 [email protected] [Impeller] Add interface for submitting multiple command buffers at once. (flutter/engine#50139) 2024-01-30 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.1.0 to 4.3.0 (flutter/engine#50165) 2024-01-30 [email protected] Roll Skia from 083c1a4d9767 to 4e992fb3a9db (1 revision) (flutter/engine#50164) 2024-01-30 [email protected] Use structured logging on Fuchsia (flutter/engine#49918) 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 [email protected],[email protected],[email protected] 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

jonahwilliams · 2024-01-30T17:10:52Z

This is causing the animated backdrop blur integration test to fail on iOS in the framework. Will investigate.

…ers at once. (#50139)" This reverts commit 0e586d1.

…fers at once." (#50174) Reverts #50139 Initiated by: jonahwilliams This change reverts the following previous change: Original Description: The Impeller Vulkan backend benefits from batching submission to the vk graphics queue. Managing this automatically is non-trivial and adds surprising/fragile thread based behavior, see: #49870 Instead, introduce an impeller::CommandQueue object that command buffers must be submitted to in lieu of CommandBuffer->Submit, which has been made private. TLDR old ```c++ buffer->Submit(); ``` new ```c++ context.GetQueue()->Submit({buffer}); ``` The Metal and GLES implementations internally just call the private CommandBuffer->Submit, though there may be future opportunities to simplify here. The Vulkan implementation is where the meat is. Aiks takes advantage of this by storing all command buffers on the aiks context while rendering a frame, and then performing one submit in aiks_context render. I don't think this will introduce any thread safety problems, as we don't guarantee much about aiks context - nor do we use it in a multithreaded context as far as I know. Other tasks such as image upload still just directly submit their command buffers via the queue. Fixes flutter/flutter#141123

…ffers at once. (flutter/engine#50139)

…142543) flutter/engine@438e9b4...0e4342c 2024-01-30 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] Add interface for submitting multiple command buffers at once." (flutter/engine#50174) 2024-01-30 [email protected] Roll Skia from a3d46fac53be to 7df4d35e11cf (3 revisions) (flutter/engine#50173) 2024-01-30 [email protected] Roll Skia from c2fada52fdc4 to a3d46fac53be (2 revisions) (flutter/engine#50171) 2024-01-30 [email protected] Roll Skia from 7dc9ba2e8c90 to c2fada52fdc4 (1 revision) (flutter/engine#50170) 2024-01-30 [email protected] Roll Skia from 1c0eae94fc09 to 7dc9ba2e8c90 (1 revision) (flutter/engine#50169) 2024-01-30 [email protected] Roll Skia from 4e992fb3a9db to 1c0eae94fc09 (3 revisions) (flutter/engine#50167) 2024-01-30 [email protected] Roll Fuchsia Linux SDK from Hqi_x_A9lYsY58VSn... to Z-xFM2ILZJw22eU8q... (flutter/engine#50166) 2024-01-30 [email protected] [Impeller] Add interface for submitting multiple command buffers at once. (flutter/engine#50139) 2024-01-30 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.1.0 to 4.3.0 (flutter/engine#50165) 2024-01-30 [email protected] Roll Skia from 083c1a4d9767 to 4e992fb3a9db (1 revision) (flutter/engine#50164) 2024-01-30 [email protected] Use structured logging on Fuchsia (flutter/engine#49918) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from Hqi_x_A9lYsY to Z-xFM2ILZJw2 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 [email protected],[email protected],[email protected] 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…ers at once. (#50180) Reland of: #50139 Metal does not seem to like it when we collect 50+ command buffers at once. Adjust the aiks context logic to regularly flush the cmd buffers.

…5956) Coming back to this from the previous attempt: #50139 * Even if its slower sometimes, its also way faster on more complex scenes * The adreno bugs seem limited to older devices. Conservatively, only enable batching for Mali and newer Adreno On non vulkan backend we immediately submit.

…utter/engine#55956) Coming back to this from the previous attempt: flutter/engine#50139 * Even if its slower sometimes, its also way faster on more complex scenes * The adreno bugs seem limited to older devices. Conservatively, only enable batching for Mali and newer Adreno On non vulkan backend we immediately submit.

[Impeller] introduce queue abstraction to allow aiks to batch submit.

4c362b2

github-actions bot added platform-android e: impeller labels Jan 29, 2024

++

f6f72f5

jonahwilliams force-pushed the queue_based_submitter branch from 84d79a6 to f6f72f5 Compare January 29, 2024 16:15

jonahwilliams added 4 commits January 29, 2024 10:08

mock and license fixes.

2ac8d59

add missing include.

7ff7cab

++

0a92952

Add dtor.

aae4d11

chinmaygarde reviewed Jan 29, 2024

View reviewed changes

jonahwilliams added 2 commits January 29, 2024 12:13

rename to command queue.

1eac589

++

48220a9

bdero reviewed Jan 29, 2024

View reviewed changes

jonahwilliams added 4 commits January 29, 2024 13:06

Remove initializer list.

fc8f1a3

fix image decoder unittests.

a1cad72

fix licenses.

e591b05

++

825e669

chinmaygarde approved these changes Jan 29, 2024

View reviewed changes

jonahwilliams added 2 commits January 29, 2024 18:29

thanks Jason.

559016a

fix mocks

d777218

jonahwilliams mentioned this pull request Jan 30, 2024

[Impeller] Join Vulkan queue submissions. #49870

Closed

jonahwilliams added 7 commits January 29, 2024 20:15

fix testing.

355bb94

++

4fb5548

++

f7b7306

maybe this works on CI.

b4d81fb

++

f33a4b6

++

2c82ff4

remove uh oh

66f9d96

Merge branch 'main' into queue_based_submitter

31e0089

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 30, 2024

auto-submit bot merged commit 0e586d1 into flutter:main Jan 30, 2024

engine-flutter-autoroll mentioned this pull request Jan 30, 2024

Roll Flutter Engine from 438e9b4d7d4e to 0e586d1c28c8 (4 revisions) flutter/flutter#142515

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 30, 2024

0e586d1c2 [Impeller] Add interface for submitting multiple command bu…

2012107

…ffers at once. (flutter/engine#50139)

jonahwilliams deleted the queue_based_submitter branch January 30, 2024 16:46

jonahwilliams added the revert Label used to revert changes in a closed and merged pull request. label Jan 30, 2024

auto-submit bot pushed a commit that referenced this pull request Jan 30, 2024

Revert "[Impeller] Add interface for submitting multiple command buff…

9e85a08

…ers at once. (#50139)" This reverts commit 0e586d1.

auto-submit bot mentioned this pull request Jan 30, 2024

Reverts "[Impeller] Add interface for submitting multiple command buffers at once." #50174

Merged

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Jan 30, 2024

engine-flutter-autoroll mentioned this pull request Jan 30, 2024

Roll Flutter Engine from 438e9b4d7d4e to e5cacca9374b (9 revisions) flutter/flutter#142534

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 30, 2024

0e586d1c2 [Impeller] Add interface for submitting multiple command bu…

4c53ea3

…ffers at once. (flutter/engine#50139)

engine-flutter-autoroll mentioned this pull request Jan 30, 2024

Roll Flutter Engine from 438e9b4d7d4e to f2c191e48120 (10 revisions) flutter/flutter#142536

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 30, 2024

0e586d1c2 [Impeller] Add interface for submitting multiple command bu…

d2816a3

…ffers at once. (flutter/engine#50139)

engine-flutter-autoroll mentioned this pull request Jan 30, 2024

Roll Flutter Engine from 438e9b4d7d4e to f2c191e48120 (10 revisions) flutter/flutter#142540

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 30, 2024

0e586d1c2 [Impeller] Add interface for submitting multiple command bu…

0e69866

…ffers at once. (flutter/engine#50139)

engine-flutter-autoroll mentioned this pull request Jan 30, 2024

Roll Flutter Engine from 438e9b4d7d4e to 0e4342c5a1f2 (11 revisions) flutter/flutter#142543

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 30, 2024

0e586d1c2 [Impeller] Add interface for submitting multiple command bu…

5522be8

…ffers at once. (flutter/engine#50139)

jonahwilliams mentioned this pull request Jan 30, 2024

[Impeller] Reland: add interface for submitting multiple command buffers at once. #50180

Merged

jonahwilliams mentioned this pull request Oct 18, 2024

[Impeller] batch submit vulkan command buffers on newer hardware. #55956

Merged

[Impeller] Add interface for submitting multiple command buffers at once. #50139

[Impeller] Add interface for submitting multiple command buffers at once. #50139

Uh oh!

Conversation

jonahwilliams commented Jan 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Jan 29, 2024

Uh oh!

jonahwilliams commented Jan 30, 2024

Uh oh!

jonahwilliams commented Jan 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonahwilliams commented Jan 29, 2024 •

edited

Loading