[Impeller] Release a texture during Playground teardown #45832

matanlurey · 2023-09-14T16:52:55Z

Partial work towards flutter/flutter#133708, flutter/flutter#133506.

This gets us closer to being able to run aiks_unittests with validations enabled, removing a VMA allocation error:

Before

Note: Google Test filter = Play/AiksTest.ReleasesTextureOnTeardown/Vulkan
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from Play/AiksTest
[ RUN      ] Play/AiksTest.ReleasesTextureOnTeardown/Vulkan
[INFO:capabilities_vk.cc(50)] Vulkan validations are enabled.
[FATAL:third_party/vulkan_memory_allocator/include/vk_mem_alloc.h(6179)] Check failed: (false && "Unfreed dedicated allocations found!"). Vulkan Memory Allocator Failure!

After

Note: Google Test filter = Play/AiksTest.ReleasesTextureOnTeardown/Vulkan
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from Play/AiksTest
[ RUN      ] Play/AiksTest.ReleasesTextureOnTeardown/Vulkan
[INFO:capabilities_vk.cc(50)] Vulkan validations are enabled.
[       OK ] Play/AiksTest.ReleasesTextureOnTeardown/Vulkan (1192 ms)
[----------] 1 test from Play/AiksTest (1192 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1192 ms total)
[  PASSED  ] 1 test.

Added a test that will catch regressions.

jonahwilliams · 2023-09-14T16:55:19Z

impeller/aiks/aiks_playground_inspector.h

+  // Underlying issue: <https://github.com/flutter/flutter/issues/134678>.
+  //
+  // The tear-down code is not running in the right order; it's torn down after
+  // we shut down the |ContextVK|, which means that the Vulkan allocator still


Its not that VMA still has a reference, its that when shutting down VMA we have retained dedicated allocations without freeing them

jonahwilliams · 2023-09-14T16:56:16Z

impeller/aiks/aiks_unittests.cc

@@ -3362,5 +3362,31 @@ TEST_P(AiksTest, CaptureInactivatedByDefault) {
  ASSERT_FALSE(GetContext()->capture.IsActive());
 }

+// Regression test for https://github.com/flutter/flutter/issues/134678.
+TEST_P(AiksTest, ReleasesTextureOnTeardown) {


Does this actually work on a "real" backend - does that matter? In theory if we've submitted a frame the fence waiter should keep the weak texture alive.

Does (a) this test work on a real backend, (b) do we avoid the underlying issue on a real backend?

(a) No, at least I've not tried it. I just want to make sure we can enable validation layers for aiks_unittests.
(flutter/flutter#133506)

(b) This probably is, as you've mentioned, show a weakness in the fence waiter that needs to be fixed as well.

LGTM, though we may need to adjust this further

Oh, wait - your point is if the fence waiter was working properly we wouldn't need this :)?

I think that if the fence waiter was working this may not be released by the end of the scope

it would only be after Shutdown. So maybe manually call contextvk->Shutdown() and then check?

Just to clarify, this test does fail (100% of the time) before our patch as-is.

Do you think I should add another test or tweak this tests behavior?

I think test may be flaky unless you call context->Shutdown before checking the weak_ptr status

or, will become flaky after we fix fence waiter

Gotcha, added a comment and ran 1000 times to ensure it's fine right now.

jonahwilliams

LGTM

…aiter-termination

flutter-dashboard · 2023-09-14T18:22:06Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #45832 at sha 2849afb

…utter/engine#45832)

…134769) flutter/engine@035932d...3a3a280 2023-09-14 skia-flutter-autoroll@skia.org Roll Skia from 9bdf01416042 to 0f003d5748bc (4 revisions) (flutter/engine#45841) 2023-09-14 jacksongardner@google.com Declare the js context as nullable in skwasm surface callback (flutter/engine#45810) 2023-09-14 matanlurey@users.noreply.github.com [Impeller] Release a texture during Playground teardown (flutter/engine#45832) 2023-09-14 skia-flutter-autoroll@skia.org Roll Skia from 87025d1e162c to 9bdf01416042 (1 revision) (flutter/engine#45835) 2023-09-14 matanlurey@users.noreply.github.com Make `fml::ScopedCleanupClosure` `std::move`-able and add unit tests. (flutter/engine#45772) 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

…lutter#134769) flutter/engine@035932d...3a3a280 2023-09-14 skia-flutter-autoroll@skia.org Roll Skia from 9bdf01416042 to 0f003d5748bc (4 revisions) (flutter/engine#45841) 2023-09-14 jacksongardner@google.com Declare the js context as nullable in skwasm surface callback (flutter/engine#45810) 2023-09-14 matanlurey@users.noreply.github.com [Impeller] Release a texture during Playground teardown (flutter/engine#45832) 2023-09-14 skia-flutter-autoroll@skia.org Roll Skia from 87025d1e162c to 9bdf01416042 (1 revision) (flutter/engine#45835) 2023-09-14 matanlurey@users.noreply.github.com Make `fml::ScopedCleanupClosure` `std::move`-able and add unit tests. (flutter/engine#45772) 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

Closes flutter/flutter#134678. Partial work towards flutter/flutter#133708, flutter/flutter#133506. This gets us closer to being able to run `aiks_unittests` with validations enabled, removing a VMA allocation error: ## Before ```txt Note: Google Test filter = Play/AiksTest.ReleasesTextureOnTeardown/Vulkan [==========] Running 1 test from 1 test suite. [----------] Global test environment set-up. [----------] 1 test from Play/AiksTest [ RUN ] Play/AiksTest.ReleasesTextureOnTeardown/Vulkan [INFO:capabilities_vk.cc(50)] Vulkan validations are enabled. [FATAL:third_party/vulkan_memory_allocator/include/vk_mem_alloc.h(6179)] Check failed: (false && "Unfreed dedicated allocations found!"). Vulkan Memory Allocator Failure! ``` ## After ```txt Note: Google Test filter = Play/AiksTest.ReleasesTextureOnTeardown/Vulkan [==========] Running 1 test from 1 test suite. [----------] Global test environment set-up. [----------] 1 test from Play/AiksTest [ RUN ] Play/AiksTest.ReleasesTextureOnTeardown/Vulkan [INFO:capabilities_vk.cc(50)] Vulkan validations are enabled. [ OK ] Play/AiksTest.ReleasesTextureOnTeardown/Vulkan (1192 ms) [----------] 1 test from Play/AiksTest (1192 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test suite ran. (1192 ms total) [ PASSED ] 1 test. ``` --- Added a test that will catch regressions.

matanlurey added 2 commits September 13, 2023 17:37

Add a unit test that fails.

6ef3220

Manually reset a reference a texture during test shutdown.

4d56f95

matanlurey added the e: impeller label Sep 14, 2023

matanlurey requested a review from jonahwilliams September 14, 2023 16:52

matanlurey self-assigned this Sep 14, 2023

jonahwilliams reviewed Sep 14, 2023

View reviewed changes

Clarify.

c4db866

jonahwilliams approved these changes Sep 14, 2023

View reviewed changes

matanlurey added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Sep 14, 2023

matanlurey added 2 commits September 14, 2023 10:47

Tweak test.

e34b65d

Merge remote-tracking branch 'upstream/main' into impeller-vk-fence-w…

2849afb

…aiter-termination

matanlurey force-pushed the impeller-vk-fence-waiter-termination branch from 57f5def to 2849afb Compare September 14, 2023 17:49

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

flutter-dashboard bot added the will affect goldens label Sep 14, 2023

auto-submit bot merged commit 741d6e3 into flutter:main Sep 14, 2023
29 checks passed

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

Roll Flutter Engine from 035932d64017 to 3a3a2807c3b6 (5 revisions) flutter/flutter#134769

Merged

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

741d6e38e [Impeller] Release a texture during Playground teardown (fl…

0f6166d

…utter/engine#45832)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] Release a texture during Playground teardown #45832

[Impeller] Release a texture during Playground teardown #45832

matanlurey commented Sep 14, 2023 •

edited

jonahwilliams Sep 14, 2023

matanlurey Sep 14, 2023

jonahwilliams Sep 14, 2023

matanlurey Sep 14, 2023

jonahwilliams Sep 14, 2023

matanlurey Sep 14, 2023

jonahwilliams Sep 14, 2023

jonahwilliams Sep 14, 2023

matanlurey Sep 14, 2023

jonahwilliams Sep 14, 2023

jonahwilliams Sep 14, 2023

matanlurey Sep 14, 2023

jonahwilliams left a comment

flutter-dashboard bot commented Sep 14, 2023

[Impeller] Release a texture during Playground teardown #45832

[Impeller] Release a texture during Playground teardown #45832

Conversation

matanlurey commented Sep 14, 2023 • edited

Before

After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams left a comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Sep 14, 2023

matanlurey commented Sep 14, 2023 •

edited