Skip to content

Conversation

@jason-simmons
Copy link
Member

The Impeller Vulkan back end creates a thread-local map of contexts to CommandPoolVK instances for each thread that uses Vulkan. This allows a thread to obtain the CommandPoolVK that is currently in use for a given context.

When a context is shut down, the Vulkan resources used by each thread's local CommandPoolVK for that context must be freed. To do this, CommandPoolVK maintains a global map of CommandPoolVK instances.

Prior to this PR Impeller was appending to the context's pool list in the global map each time a new CommandPoolVK was created. In the original implementation this worked because Impeller was only creating one CommandPoolVK per thread for a given context.

However, CommandPoolVK later adopted a recycling scheme where each frame creates a new CommandPoolVK instance that acquires a Vulkan command pool from the CommandPoolRecyclerVK. So inserting every CommandPoolVK into the global map will cause the global map to grow unbounded.

This PR changes the structure of the global map. The global map will now associate each context with a map of thread IDs to the CommandPoolVK that is currently placed in the thread's local storage. When a thread calls CommandPoolRecyclerVK::Dispose to clear its thread-local CommandPoolVK for a context, the corresponding entry is also removed from the global map.

Fixes #169208

…hread-local command pools

The Impeller Vulkan back end creates a thread-local map of contexts to
CommandPoolVK instances for each thread that uses Vulkan.  This allows a
thread to obtain the CommandPoolVK that is currently in use for a given
context.

When a context is shut down, the Vulkan resources used by each thread's local
CommandPoolVK for that context must be freed.  To do this, CommandPoolVK
maintains a global map of CommandPoolVK instances.

Prior to this PR Impeller was appending to the context's pool list in the
global map each time a new CommandPoolVK was created.  In the original
implementation this worked because Impeller was only creating one
CommandPoolVK per thread for a given context.

However, CommandPoolVK later adopted a recycling scheme where each frame
creates a new CommandPoolVK instance that acquires a Vulkan command pool from
the CommandPoolRecyclerVK.  So inserting every CommandPoolVK into the global
map will cause the global map to grow unbounded.

This PR changes the structure of the global map.  The global map will now
associate each context with a map of thread IDs to the CommandPoolVK that
is currently placed in the thread's local storage.  When a thread calls
CommandPoolRecyclerVK::Dispose to clear its thread-local CommandPoolVK for a
context, the corresponding entry is also removed from the global map.

Fixes flutter#169208
@github-actions github-actions bot added engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels May 27, 2025
Copy link
Contributor

@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

… playground tests

On Vulkan the golden tests are using a single ContextVK instance that is shared
among all tests.  The ContextVK is never deleted by the test harness.

So when the test process exits there may still be a CommandPoolVK in the
thread-local map.  The map and the CommandPoolVK will then be destructed
when global and thread-local objects are destructed, which may be unsafe.

Calling DisposeThreadLocalCachedResources forces cleanup of the thread-local
map when each test case finishes.
@jason-simmons jason-simmons added the autosubmit Merge PR when tree becomes green via auto submit App label May 29, 2025
@auto-submit auto-submit bot added this pull request to the merge queue May 29, 2025
Merged via the queue into flutter:master with commit 8abf4e7 May 29, 2025
182 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 29, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 29, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 29, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 29, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 29, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 29, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 30, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 30, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 30, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 2, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 2, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025
@jason-simmons jason-simmons added the cp: stable cherry pick this pull request to stable release candidate branch label Jun 4, 2025
flutteractionsbot pushed a commit to flutteractionsbot/flutter that referenced this pull request Jun 4, 2025
…hread-local command pools (flutter#169548)

The Impeller Vulkan back end creates a thread-local map of contexts to
CommandPoolVK instances for each thread that uses Vulkan. This allows a
thread to obtain the CommandPoolVK that is currently in use for a given
context.

When a context is shut down, the Vulkan resources used by each thread's
local CommandPoolVK for that context must be freed. To do this,
CommandPoolVK maintains a global map of CommandPoolVK instances.

Prior to this PR Impeller was appending to the context's pool list in
the global map each time a new CommandPoolVK was created. In the
original implementation this worked because Impeller was only creating
one CommandPoolVK per thread for a given context.

However, CommandPoolVK later adopted a recycling scheme where each frame
creates a new CommandPoolVK instance that acquires a Vulkan command pool
from the CommandPoolRecyclerVK. So inserting every CommandPoolVK into
the global map will cause the global map to grow unbounded.

This PR changes the structure of the global map. The global map will now
associate each context with a map of thread IDs to the CommandPoolVK
that is currently placed in the thread's local storage. When a thread
calls CommandPoolRecyclerVK::Dispose to clear its thread-local
CommandPoolVK for a context, the corresponding entry is also removed
from the global map.

Fixes flutter#169208
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cp: stable cherry pick this pull request to stable release candidate branch e: impeller Impeller rendering backend issues and features requests engine flutter/engine related. See also e: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VideoPlayer - Memory Leak

2 participants