Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Fix thread leak in ResourceManagerVK. #45686

Merged
merged 2 commits into from
Sep 12, 2023

Conversation

matanlurey
Copy link
Contributor

@matanlurey matanlurey commented Sep 12, 2023

Fixes flutter/flutter#134482.

I wish I knew how to create the thread in ::Create(), but given the constructor is private I feel less bad about reverting part of #45474. Added a test that consistently fails before this PR and passes after.

(Unblocks flutter/flutter#133198)

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@matanlurey matanlurey merged commit 0c771e0 into flutter:main Sep 12, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 12, 2023
…134505)

flutter/engine@6ddee44...ee72155

2023-09-12 skia-flutter-autoroll@skia.org Roll Skia from c4e94d5febdc to 62aa41ee8135 (2 revisions) (flutter/engine#45690)
2023-09-12 matanlurey@users.noreply.github.com [Impeller] Fix thread leak in ResourceManagerVK. (flutter/engine#45686)

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
matanlurey added a commit that referenced this pull request Sep 12, 2023
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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Impeller] ResourceManagerVK never terminates it's thread
2 participants