[Impeller] Test `FenceWaiterVK` and fix termination bugs #45870

matanlurey · 2023-09-15T07:35:30Z

Fixes flutter/flutter#134751, and @jonahwilliams suspects it could be related to a number of other flaky/texture leak scenarios (flutter/flutter#133506 (comment)) that only happen sometimes (i.e. on CI but not real devices), i.e. stuff like:

--- Vulkan Debug Report  ----------------------------------------
|                Severity: Error
|                    Type: { Validation }
|                 ID Name: VUID-vkDestroyBuffer-buffer-00922
|               ID Number: -464217071
|       Queue Breadcrumbs: [NONE]
|  CMD Buffer Breadcrumbs: [NONE]
|         Related Objects: Device [94498356231456] [ImpellerDevice]
|                 Trigger: Validation Error: [ VUID-vkDestroyBuffer-buffer-00922 ] Object 0: handle = 0x55f21cf47d20, name = ImpellerDevice, type = VK_OBJECT_TYPE_DEVICE; | MessageID = 0xe4549c11 | Cannot call vkDestroyBuffer on VkBuffer 0xbb00000000bb[] that is currently in use by a command buffer. The Vulkan spec states: All submitted commands that refer to buffer, either directly or via a VkBufferView, must have completed execution (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-vkDestroyBuffer-buffer-00922)
-----------------------------------------------------------------

This PR will look a bit like a mess until the last 2 PRs merge in, but locally it appears to fix fence races/segfaults that I was seeing on CI, including on Linux with validations enabled. We can test it further tomorrow. EDIT: Updated.

…larke/engine into gaaclarke-mock-vulkan-context-builder

impeller/renderer/backend/vulkan/fence_waiter_vk.cc

impeller/renderer/backend/vulkan/context_vk.cc

gaaclarke

This code is tricky, it looks good to me and the additional tests are an welcome addition. You should get 2 approvals just because it's tricky threading changes.

One minor nit across the board is that auto should only be used if if makes the code easier to read, not out of convenience as per the style guide. We violate this a lot.

impeller/renderer/backend/vulkan/fence_waiter_vk.cc

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

impeller/renderer/backend/vulkan/fence_waiter_vk.cc

jonahwilliams

LGTM

matanlurey · 2023-09-15T18:50:26Z

@chinmaygarde I have 2 LGTMs, but would prefer you as a third if you're around.

impeller/renderer/backend/vulkan/fence_waiter_vk.cc

chinmaygarde

Not having static thread analysis annotated synchronization utils for condition variables really hurts the reviewability of this PR. We should patch that in //impeller/base/thread.h. Rather not have to reason about things the compiler can :/

Other than that, just nits.

chinmaygarde · 2023-09-15T21:00:04Z

impeller/renderer/backend/vulkan/context_vk.cc

@@ -377,10 +377,6 @@ void ContextVK::Setup(Settings settings) {
  ///
  auto fence_waiter =
      std::shared_ptr<FenceWaiterVK>(new FenceWaiterVK(device_holder));
-  if (!fence_waiter->IsValid()) {


An invalid fence waiter will no longer invalidate the context now. In the future, if fence waiters cannot be created reliably, the call sites of the users will have to be patched.

There is no such thing as an invalid fence waiter, that is, there is no state that ever marks the fence waiter as invalid. I found it misleading that we check a boolean that is effectively constant true - let's it add it back if/when there is such a case?

impeller/renderer/backend/vulkan/fence_waiter_vk.cc

…lutter/engine#45870)

…ions) (#134876) Manual roll requested by [email protected] flutter/engine@326faf1...30b7e9d 2023-09-15 [email protected] Roll Skia from 7f88bda24f7f to 0057898979a1 (1 revision) (flutter/engine#45904) 2023-09-15 [email protected] [Impeller] Test `FenceWaiterVK` and fix termination bugs (flutter/engine#45870) 2023-09-15 [email protected] Roll Dart SDK from a5ee0055cf20 to e9452310189b (1 revision) (flutter/engine#45902) 2023-09-15 [email protected] Roll Skia from 4f26f22daa4b to 7f88bda24f7f (5 revisions) (flutter/engine#45901) 2023-09-15 [email protected] [Impeller] dont cache failed render target allocations. (flutter/engine#45895) 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://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#134876) Manual roll requested by [email protected] flutter/engine@326faf1...30b7e9d 2023-09-15 [email protected] Roll Skia from 7f88bda24f7f to 0057898979a1 (1 revision) (flutter/engine#45904) 2023-09-15 [email protected] [Impeller] Test `FenceWaiterVK` and fix termination bugs (flutter/engine#45870) 2023-09-15 [email protected] Roll Dart SDK from a5ee0055cf20 to e9452310189b (1 revision) (flutter/engine#45902) 2023-09-15 [email protected] Roll Skia from 4f26f22daa4b to 7f88bda24f7f (5 revisions) (flutter/engine#45901) 2023-09-15 [email protected] [Impeller] dont cache failed render target allocations. (flutter/engine#45895) 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://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

@jonahwilliams

Fixes flutter/flutter#134751, and @jonahwilliams suspects it could be related to a number of other flaky/texture leak scenarios (flutter/flutter#133506 (comment)) that only happen sometimes (i.e. on CI but not real devices), i.e. stuff like: ```txt --- Vulkan Debug Report Â ---------------------------------------- | Â Â Â Â Â Â Â Â Severity: Error | Â Â Â Â Â Â Â Â Â Â Type: { Validation } | Â Â Â Â Â Â Â Â ID Name: VUID-vkDestroyBuffer-buffer-00922 | Â Â Â Â Â Â Â ID Number: -464217071 | Â Â Â Queue Breadcrumbs: [NONE] | Â CMD Buffer Breadcrumbs: [NONE] | Â Â Â Â Related Objects: Device [94498356231456] [ImpellerDevice] | Â Â Â Â Â Â Â Â Trigger: Validation Error: [ VUID-vkDestroyBuffer-buffer-00922 ] Object 0: handle = 0x55f21cf47d20, name = ImpellerDevice, type = VK_OBJECT_TYPE_DEVICE; | MessageID = 0xe4549c11 | Cannot call vkDestroyBuffer on VkBuffer 0xbb00000000bb[] that is currently in use by a command buffer. The Vulkan spec states: All submitted commands that refer to buffer, either directly or via a VkBufferView, must have completed execution (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-vkDestroyBuffer-buffer-00922) ----------------------------------------------------------------- ``` --- ~~This PR will look a bit like a mess until the last 2 PRs merge in, but locally it appears to fix fence races/segfaults that I was seeing on CI, including on Linux with validations enabled. We can test it further tomorrow.~~ EDIT: Updated.

gaaclarke and others added 13 commits September 14, 2023 11:17

[Impeller] transitioned mock vulkan context to the builder pattern

2ff38d2

made creating a context with validation layers work

60b871d

added capabilties asserts

67e9055

moved to fml thread_local

ba4bc1e

removed strcpy

004f79b

switched to strncpy

da1aada

moved to sizeof since its a bit more robust

630f915

Merge branch 'mock-vulkan-context-builder' of https://github.com/gaac…

10b7cdc

…larke/engine into gaaclarke-mock-vulkan-context-builder

Add MockFence and MockFence::SetStatus.

c89d30d

Remove unused code.

ce6a1c5

Add stub unit test, remove unused IsValid() method.

ac485e1

Add unit tests, including those that fail.

50ef1c0

Fix fence waiter and add tests.

94500af

matanlurey requested a review from jonahwilliams September 15, 2023 07:35

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

matanlurey self-assigned this Sep 15, 2023

matanlurey added 3 commits September 15, 2023 00:36

Merge branch 'main' into impeller-vk-fence-contract

3a01df4

Fix buggy fence waiter test.

5540542

Make mocks thread-safe and tweak.

2efd5e8

matanlurey mentioned this pull request Sep 15, 2023

[Impeller] Add MockFence to mock_vulkan. #45862

Closed

matanlurey added 2 commits September 15, 2023 10:36

Merge.

79998e7

Use atomics, delete unused code.

4e1861c

matanlurey changed the title ~~[📚 STACKED] Test FenceWaiterVK and fix termination bugs~~ [Impeller] Test FenceWaiterVK and fix termination bugs Sep 15, 2023

matanlurey requested review from gaaclarke and chinmaygarde September 15, 2023 17:42

Merge branch 'main' into impeller-vk-fence-contract

530af8d

matanlurey marked this pull request as ready for review September 15, 2023 17:42

jonahwilliams reviewed Sep 15, 2023

View reviewed changes

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

jonahwilliams reviewed Sep 15, 2023

View reviewed changes

impeller/renderer/backend/vulkan/context_vk.cc Show resolved Hide resolved

Tweak fence termination contract.

1460841

gaaclarke approved these changes Sep 15, 2023

View reviewed changes

false -> true

5bdcb65

jonahwilliams reviewed Sep 15, 2023

View reviewed changes

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

Address some comments and add a TODO.

e5ab21b

matanlurey requested a review from jonahwilliams September 15, 2023 18:31

Add FML_DCHECK.

110f745

jonahwilliams approved these changes Sep 15, 2023

View reviewed changes

jason-simmons reviewed Sep 15, 2023

View reviewed changes

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

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

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

Address feedback.

d973574

matanlurey requested a review from jason-simmons September 15, 2023 19:57

gaaclarke mentioned this pull request Sep 15, 2023

[Impeller] Adds playgrounds test to CI. #45857

Closed

8 tasks

jason-simmons reviewed Sep 15, 2023

View reviewed changes

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

chinmaygarde approved these changes Sep 15, 2023

View reviewed changes

Address comments.

fe3c926

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

jason-simmons approved these changes Sep 15, 2023

View reviewed changes

auto-submit bot merged commit 06567e1 into flutter:main Sep 15, 2023
27 checks passed

matanlurey mentioned this pull request Sep 16, 2023

[Impeller] Validation errors when running playgrounds with Vulkan validation enabled. flutter/flutter#133506

Closed

matanlurey deleted the impeller-vk-fence-contract branch September 16, 2023 15:36

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

Manual roll Flutter Engine from 326faf1762d6 to 30b7e9ded7a0 (5 revisions) flutter/flutter#134876

Merged

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

06567e156 [Impeller] Test FenceWaiterVK and fix termination bugs (f…

759af7c

…lutter/engine#45870)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] Test `FenceWaiterVK` and fix termination bugs #45870

[Impeller] Test `FenceWaiterVK` and fix termination bugs #45870

matanlurey commented Sep 15, 2023 •

edited

Loading

gaaclarke left a comment

jonahwilliams left a comment

matanlurey commented Sep 15, 2023

chinmaygarde left a comment •

edited

Loading

chinmaygarde Sep 15, 2023

matanlurey Sep 15, 2023

[Impeller] Test FenceWaiterVK and fix termination bugs #45870

[Impeller] Test FenceWaiterVK and fix termination bugs #45870

Conversation

matanlurey commented Sep 15, 2023 • edited Loading

gaaclarke left a comment

Choose a reason for hiding this comment

jonahwilliams left a comment

Choose a reason for hiding this comment

matanlurey commented Sep 15, 2023

chinmaygarde left a comment • edited Loading

Choose a reason for hiding this comment

chinmaygarde Sep 15, 2023

Choose a reason for hiding this comment

matanlurey Sep 15, 2023

Choose a reason for hiding this comment

[Impeller] Test `FenceWaiterVK` and fix termination bugs #45870

[Impeller] Test `FenceWaiterVK` and fix termination bugs #45870

matanlurey commented Sep 15, 2023 •

edited

Loading

chinmaygarde left a comment •

edited

Loading