[Impeller] `vk::CommandPool` resets via `ResourceManager` #45654

matanlurey · 2023-09-11T17:05:05Z

Fixes flutter/flutter#133198.

Major changes:

CommandPoolVK is now created indirectly through CommandPoolRecyclerVK.
Via destructor, CommandPoolVK resets on a background thread via ResourceManager.
Removed all of the code trying to reuse/reset individual command buffers (no longer needed).
After every frame, the current/active command pool is recycled.

Tests secured.

…ol-vk-recycle

impeller/renderer/backend/vulkan/command_pool_vk.cc

…ol-vk-recycle

impeller/renderer/backend/vulkan/test/mock_vulkan.cc

impeller/renderer/backend/vulkan/command_pool_vk.h

chinmaygarde · 2023-09-11T23:20:19Z

impeller/renderer/backend/vulkan/command_pool_vk.h

+///
+/// A single instance should be created per |ContextVK|.
+///
+/// Every "frame", a single |CommandPoolResourceVk| is made available for each


Please make a note of the the threading restrictions you've accounted for here. Specifically, the spec says:

Command pools are externally synchronized, meaning that a command pool must not be used concurrently in multiple threads. That includes use via recording commands on any command buffers allocated from the pool, as well as operations that allocate, free, and reset command buffers or the pool itself.

The things I would look for are:

Are the commands in the command pool done executing when recycle is called?

Does the swap and recycle happen in a thread safe manner.

Ack. I did find a problem (in unit tests) and addressed them. Did you want the docs updated?

It would be good to have at least 1 answered clearly in the docs. 2 is an implementation detail I am fine leaving out.

Sounds good, added!

impeller/renderer/backend/vulkan/command_pool_vk.h

impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc

impeller/renderer/backend/vulkan/command_pool_vk.cc

matanlurey · 2023-09-12T00:34:55Z

I ran into some multi-threading problems with CreateMockVulkanContext, so I'm going to send a patch to @gaaclarke on that first before addressing the comments above (likely tomorrow AM at this point). Thanks!!!

matanlurey · 2023-09-12T02:23:08Z

I found two other sources of bugs (other than code in this PR) blocking and have fixes out:

Once those are both in I'll update to main and address @chinmaygarde's comments/re-run CI.

I started using `CreateMockVulkanContext()` in #45654, and discovered that it was not thread-safe. This PR does a little bit of refactoring/TLC to ensure that functions and buffers can be accessed across threads (like the _real_ `ContextVK`). The attached test (_test for a test fixture, hoorah!_) fails consistently before the changes, and passes after. Note that if you try testing this with `--gtest_repeat=10000` (at least that's the threshold for me), it'll fail - that's actually due to flutter/flutter#134482 which in turn is being fixed in #45686. (Unblocks flutter/flutter#133198) --------- Co-authored-by: gaaclarke <30870216+gaaclarke@users.noreply.github.com>

jonahwilliams · 2023-09-12T19:53:20Z

impeller/renderer/backend/vulkan/command_pool_vk.cc


-  buffers_to_collect_.clear();
+void CommandPoolRecyclerVK::Recycle() {


Seems more like disposal than recycling?

Ah fair enough. Renamed!

jonahwilliams

Overall LGTM, though not to CI 😄

impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc

chinmaygarde · 2023-09-12T19:24:55Z

impeller/renderer/backend/vulkan/command_pool_vk.h

+///
+/// A single instance should be created per |ContextVK|.
+///
+/// Every "frame", a single |CommandPoolResourceVk| is made available for each


It would be good to have at least 1 answered clearly in the docs. 2 is an implementation detail I am fine leaving out.

impeller/renderer/backend/vulkan/command_pool_vk.cc

impeller/renderer/backend/vulkan/fence_waiter_vk.cc

…lutter/engine#45654)

…ions) (#134882) Manual roll requested by zra@google.com flutter/engine@4909256...8c2203b 2023-09-16 skia-flutter-autoroll@skia.org Roll Skia from b33e0e6706b0 to 2eaaa5bf7dae (1 revision) (flutter/engine#45926) 2023-09-16 skia-flutter-autoroll@skia.org Roll Skia from a1f8fb54299a to b33e0e6706b0 (1 revision) (flutter/engine#45923) 2023-09-16 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from -_edKGA5GTkDFxVgl... to 3Tb9CQTTiBv1lb673... (flutter/engine#45922) 2023-09-16 skia-flutter-autoroll@skia.org Roll Dart SDK from 029d6d73c860 to a0ca39c63c16 (1 revision) (flutter/engine#45921) 2023-09-16 matanlurey@users.noreply.github.com [Impeller] `vk::CommandPool` resets via `ResourceManager` (flutter/engine#45654) 2023-09-16 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from wWzXsy6kx1sp8Km34... to ZhY53WD7bFJSA3xoO... (flutter/engine#45919) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from wWzXsy6kx1sp to ZhY53WD7bFJS fuchsia/sdk/core/mac-amd64 from -_edKGA5GTkD to 3Tb9CQTTiBv1 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 bdero@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

…ions) (flutter#134882) Manual roll requested by zra@google.com flutter/engine@4909256...8c2203b 2023-09-16 skia-flutter-autoroll@skia.org Roll Skia from b33e0e6706b0 to 2eaaa5bf7dae (1 revision) (flutter/engine#45926) 2023-09-16 skia-flutter-autoroll@skia.org Roll Skia from a1f8fb54299a to b33e0e6706b0 (1 revision) (flutter/engine#45923) 2023-09-16 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from -_edKGA5GTkDFxVgl... to 3Tb9CQTTiBv1lb673... (flutter/engine#45922) 2023-09-16 skia-flutter-autoroll@skia.org Roll Dart SDK from 029d6d73c860 to a0ca39c63c16 (1 revision) (flutter/engine#45921) 2023-09-16 matanlurey@users.noreply.github.com [Impeller] `vk::CommandPool` resets via `ResourceManager` (flutter/engine#45654) 2023-09-16 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from wWzXsy6kx1sp8Km34... to ZhY53WD7bFJSA3xoO... (flutter/engine#45919) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from wWzXsy6kx1sp to ZhY53WD7bFJS fuchsia/sdk/core/mac-amd64 from -_edKGA5GTkD to 3Tb9CQTTiBv1 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 bdero@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

Fixes flutter/flutter#133198. Major changes: - `CommandPoolVK` is now created indirectly through `CommandPoolRecyclerVK`. - Via destructor, `CommandPoolVK` resets on a background thread via `ResourceManager`. - Removed all of the code trying to reuse/reset individual command buffers (no longer needed). - After every frame, the current/active command pool is recycled. --- _Tests secured._

matanlurey added 9 commits September 8, 2023 11:49

WIP for discussion.

bf2238a

The hackiest hacks that ever hacked, but it works.

4f8d143

Checkpoint.

2603a87

Checkpoint: Runs, but segfaults in EncoderVK::Track.

3e7cb5e

Checkpoint: Runs, but segfaults in EncoderVK::Track.

f35dc1f

Ah, the old forgot to finish implementing... Fun.

bfe727e

Back in action. Good night for now.

1af1878

Remove FML_LOG statements.

df64736

Merge remote-tracking branch 'upstream/main' into impeller-command-po…

2264669

…ol-vk-recycle

matanlurey requested a review from jonahwilliams September 11, 2023 17:05

matanlurey self-assigned this Sep 11, 2023

github-actions bot added the e: impeller label Sep 11, 2023

matanlurey changed the title ~~[Impeller] vk::CommandPool is now reset off-thread using ResourceManager~~ [Impeller] vk::CommandPool resets via ResourceManager Sep 11, 2023

jonahwilliams reviewed Sep 11, 2023

View reviewed changes

impeller/renderer/backend/vulkan/command_pool_vk.cc Outdated Show resolved Hide resolved

matanlurey added 3 commits September 11, 2023 13:40

Rewrite comment, fix race condition in destruction of ContextVK.

8d22f90

Restore commented-out test.

12d456d

Add unit tests, fix bugs.

443ac71

matanlurey marked this pull request as ready for review September 11, 2023 22:35

Merge branch 'main' into impeller-command-pool-vk-recycle

8fdd836

matanlurey requested review from chinmaygarde and gaaclarke September 11, 2023 22:35

matanlurey added 2 commits September 11, 2023 15:50

Fix licenses.

2ca3c67

Merge remote-tracking branch 'upstream/main' into impeller-command-po…

538e5a3

…ol-vk-recycle

chinmaygarde reviewed Sep 11, 2023

View reviewed changes

matanlurey mentioned this pull request Sep 12, 2023

[Impeller] Make CreateMockVulkanContext() thread-safe #45687

Merged

Merge.

be87e63

matanlurey mentioned this pull request Sep 12, 2023

[Impeller] CreateMockVulkanContext() should become a test harness class. flutter/flutter#134561

Open

Fix renames.

74438e6

jonahwilliams reviewed Sep 12, 2023

View reviewed changes

jonahwilliams approved these changes Sep 12, 2023

View reviewed changes

chinmaygarde reviewed Sep 12, 2023

View reviewed changes

matanlurey added 4 commits September 12, 2023 13:22

Re-order ptr resets.

a4c6c1d

Re-order fence waiter loop to termintae last.

852482f

Address feedback, tweak the waiter.

c27bedd

Tweak fence waiter and try on CI again.

15472ac

chinmaygarde approved these changes Sep 12, 2023

View reviewed changes

matanlurey added 3 commits September 12, 2023 16:38

Try another CI tweak.

82a8944

Command encoder tests are passing.

d2ab1c4

Try another variant.

869d5c0

jonahwilliams reviewed Sep 13, 2023

View reviewed changes

impeller/renderer/backend/vulkan/fence_waiter_vk.cc Outdated Show resolved Hide resolved

Avoid holding on to buffers on collected pools.

3bc4f0a

matanlurey mentioned this pull request Sep 13, 2023

[Impeller] Playgrounds leak a VMA-allocated texture flutter/flutter#134678

Closed

Merge branch 'main' into impeller-command-pool-vk-recycle

692b85d

matanlurey mentioned this pull request Sep 15, 2023

[Impeller] Test FenceWaiterVK and fix termination bugs #45870

Merged

matanlurey added 3 commits September 15, 2023 20:32

Merge.

46e6d80

Fix test.

6e1a89b

Revert fence waiter changes.

2d04529

matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 16, 2023

auto-submit bot merged commit cc79017 into flutter:main Sep 16, 2023

engine-flutter-autoroll mentioned this pull request Sep 16, 2023

Manual roll Flutter Engine from 490925676b91 to 8c2203bb6c3b (6 revisions) flutter/flutter#134882

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 16, 2023

cc79017b6 [Impeller] vk::CommandPool resets via ResourceManager (f…

9b48733

…lutter/engine#45654)

matanlurey deleted the impeller-command-pool-vk-recycle branch September 19, 2023 16:44

matanlurey mentioned this pull request Sep 20, 2023

[Impeller] Vulkan Validation errors present due to Swapchain flutter/flutter#135086

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Impeller] `vk::CommandPool` resets via `ResourceManager` #45654

[Impeller] `vk::CommandPool` resets via `ResourceManager` #45654

Uh oh!

matanlurey commented Sep 11, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chinmaygarde Sep 11, 2023

Uh oh!

matanlurey Sep 12, 2023

Uh oh!

chinmaygarde Sep 12, 2023

Uh oh!

matanlurey Sep 12, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matanlurey commented Sep 12, 2023

Uh oh!

matanlurey commented Sep 12, 2023

Uh oh!

jonahwilliams Sep 12, 2023

Uh oh!

matanlurey Sep 12, 2023

Uh oh!

jonahwilliams left a comment

Uh oh!

Uh oh!

chinmaygarde Sep 12, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!


		buffers_to_collect_.clear();
		void CommandPoolRecyclerVK::Recycle() {

[Impeller] vk::CommandPool resets via ResourceManager #45654

[Impeller] vk::CommandPool resets via ResourceManager #45654

Uh oh!

Conversation

matanlurey commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chinmaygarde Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

matanlurey Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

matanlurey Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matanlurey commented Sep 12, 2023

Uh oh!

matanlurey commented Sep 12, 2023

Uh oh!

jonahwilliams Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

matanlurey Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chinmaygarde Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[Impeller] `vk::CommandPool` resets via `ResourceManager` #45654

[Impeller] `vk::CommandPool` resets via `ResourceManager` #45654

matanlurey commented Sep 11, 2023 •

edited

Loading