Repository navigation
[Impeller] Guard the Metal pipeline library maps with a mutex - #193722
Merged
auto-submit[bot] merged 2 commits intoOct 6, 2026
Merged
auto-submit[bot] merged 2 commits into
auto-submit[bot] merged 2 commits into
Conversation
Flutter GPU looks up render pipelines from the UI thread (flutter::gpu::RenderPass::GetOrCreatePipeline) while Impeller creates pipeline variants on the raster thread. PipelineLibraryVK guards its maps with a mutex, but PipelineLibraryMTL did not, so a lookup on one thread could race an insertion or rehash on the other and crash in unordered_map::find. Guard pipelines_ and compute_pipelines_ with a mutex. The lock covers only the map accesses, not pipeline creation, and try_emplace makes concurrent requests for the same descriptor share one future. Add RendererTest.PipelineLibraryAllowsConcurrentGetPipeline, which looks up an existing pipeline on one thread while two other threads create new variants.
11 tasks
github-actions
Bot
added
engine
leehack
marked this pull request as ready for review
October 2, 2026 18:33
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces thread safety to the Metal pipeline library (PipelineLibraryMTL) in Impeller by protecting the pipeline maps (pipelines_ and compute_pipelines_) with a mutex (pipelines_mutex_). It also adds a concurrent unit test (PipelineLibraryAllowsConcurrentGetPipeline) to verify that pipelines can be safely retrieved and queried from multiple threads simultaneously. There are no review comments, and I have no feedback to provide.
This was referenced Oct 2, 2026
Member
|
fyi @bdero |
bdero
approved these changes
Oct 5, 2026
bdero
left a comment
bdero
left a comment
Member
There was a problem hiding this comment.
LGTM, thanks for tracking this down. It matches the locking PipelineLibraryVK already does, and releasing the lock before pipeline creation keeps UI-thread draws from waiting on compilation.
gaaclarke
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
This was referenced Oct 6, 2026
This was referenced Oct 8, 2026
abdulawalarif
pushed a commit
to abdulawalarif/flutter
that referenced
this pull request
Oct 11, 2026
) `SamplerLibraryMTL`, `SamplerLibraryVK` and `SamplerLibraryGLES` cache samplers in a plain vector that `GetSampler` scans and then appends to, with no lock. Flutter GPU calls `GetSampler` on the UI thread (`RenderPass.bindTexture`) while Impeller calls it on the raster thread (for example `FramebufferBlendContents::Render`). A scan can read a buffer that a concurrent `push_back` is reallocating, and two concurrent inserts can corrupt the vector or drop a sampler that was already returned. This PR guards `samplers_` with a mutex in all three backends. On Metal and Vulkan, the lock covers the lookup and the insert but not sampler creation, and the insert checks the cache again so concurrent requests share one sampler. GLES creates no device object, so it holds the lock for the whole lookup. The new test, `RendererTest.SamplerLibraryAllowsConcurrentGetSampler`, starts four threads on a latch, has each request the same 36 samplers, and checks that every thread gets the same valid sampler for each descriptor. Verified on master d03768e: Metal on macOS (M4 Max), and Vulkan (SwiftShader) and OpenGL ES (Mesa, Xvfb) on Linux x64 with the CI environment: | | Without the lock | With the lock | | --- | --- | --- | | New test, 40 runs (Metal / Vulkan / GLES) | 0 / 1 / 0 pass | 40/40 on each | | New test under TSan | sampler races on all three | no sampler race | | Repro app from the issue under TSan (Metal) | race in 4 of 6 runs | none in 6 runs | The macOS `impeller_unittests` CI filter (2426 passed, 0 failed) and the Metal backend tests pass, and the format and clang-tidy checks are clean. On Linux, the full `impeller_unittests` run was stopped early with 1154 passed and 0 failed, and the runs used the previous revision of the test, which started its threads with a spin barrier instead of a latch. Related: flutter#193721 / flutter#193722 (the same race in `PipelineLibraryMTL`). Both PRs add a test to `renderer_unittests.cc`; merging them conflicts only on the `#include <...>` lines. Fixes flutter#193737 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant in-code documentation (doc comments with `///`). - [x] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/website]: https://github.com/flutter/website [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
suryaKommana2662
pushed a commit
to suryaKommana2662/flutter
that referenced
this pull request
Oct 29, 2026
…r#193722) `PipelineLibraryMTL` reads and writes its pipeline maps without a lock, but Flutter GPU calls `GetPipeline` on the UI thread while Impeller calls it on the raster thread. A lookup can hit a concurrent insert and crash in `unordered_map::find`. `PipelineLibraryVK` already guards its maps with a mutex. This PR adds a mutex to `PipelineLibraryMTL`. In `GetPipeline` it covers only the map lookup and the insert, and is released before the Metal pipeline is created, so draws never wait on pipeline creation. The new test, `RendererTest.PipelineLibraryAllowsConcurrentGetPipeline`, looks up a pipeline on one thread while two threads add new ones. It skips GLES. Verified locally on master (macOS, M4 Max): | | Without the lock | With the lock | | --- | --- | --- | | New test, 40 runs | 20 pass, 15 fail, 4 hang, 1 crash | 40/40 pass | | New test under TSan | data races in `PipelineLibraryMTL`, then crash | no race on the maps | | Repro app from the issue under TSan | race reported | no race on the maps | The macOS `impeller_unittests` (CI filter, 2426 passed, 0 failed) and `metal_unittests` pass, and the format and clang-tidy checks are clean. flutter#189899 (on master) makes these lookups rarer but does not remove them. `SamplerLibraryMTL` and `PipelineLibraryGLES` look similar and will be reported separately. Fixes flutter#193721 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant in-code documentation (doc comments with `///`). - [x] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/website]: https://github.com/flutter/website [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
suryaKommana2662
pushed a commit
to suryaKommana2662/flutter
that referenced
this pull request
Oct 29, 2026
) `SamplerLibraryMTL`, `SamplerLibraryVK` and `SamplerLibraryGLES` cache samplers in a plain vector that `GetSampler` scans and then appends to, with no lock. Flutter GPU calls `GetSampler` on the UI thread (`RenderPass.bindTexture`) while Impeller calls it on the raster thread (for example `FramebufferBlendContents::Render`). A scan can read a buffer that a concurrent `push_back` is reallocating, and two concurrent inserts can corrupt the vector or drop a sampler that was already returned. This PR guards `samplers_` with a mutex in all three backends. On Metal and Vulkan, the lock covers the lookup and the insert but not sampler creation, and the insert checks the cache again so concurrent requests share one sampler. GLES creates no device object, so it holds the lock for the whole lookup. The new test, `RendererTest.SamplerLibraryAllowsConcurrentGetSampler`, starts four threads on a latch, has each request the same 36 samplers, and checks that every thread gets the same valid sampler for each descriptor. Verified on master d03768e: Metal on macOS (M4 Max), and Vulkan (SwiftShader) and OpenGL ES (Mesa, Xvfb) on Linux x64 with the CI environment: | | Without the lock | With the lock | | --- | --- | --- | | New test, 40 runs (Metal / Vulkan / GLES) | 0 / 1 / 0 pass | 40/40 on each | | New test under TSan | sampler races on all three | no sampler race | | Repro app from the issue under TSan (Metal) | race in 4 of 6 runs | none in 6 runs | The macOS `impeller_unittests` CI filter (2426 passed, 0 failed) and the Metal backend tests pass, and the format and clang-tidy checks are clean. On Linux, the full `impeller_unittests` run was stopped early with 1154 passed and 0 failed, and the runs used the previous revision of the test, which started its threads with a spin barrier instead of a latch. Related: flutter#193721 / flutter#193722 (the same race in `PipelineLibraryMTL`). Both PRs add a test to `renderer_unittests.cc`; merging them conflicts only on the `#include <...>` lines. Fixes flutter#193737 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant in-code documentation (doc comments with `///`). - [x] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/website]: https://github.com/flutter/website [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PipelineLibraryMTLreads and writes its pipeline maps without a lock, but Flutter GPU callsGetPipelineon the UI thread while Impeller calls it on the raster thread. A lookup can hit a concurrent insert and crash inunordered_map::find.PipelineLibraryVKalready guards its maps with a mutex.This PR adds a mutex to
PipelineLibraryMTL. InGetPipelineit covers only the map lookup and the insert, and is released before the Metal pipeline is created, so draws never wait on pipeline creation.The new test,
RendererTest.PipelineLibraryAllowsConcurrentGetPipeline, looks up a pipeline on one thread while two threads add new ones. It skips GLES.Verified locally on master (macOS, M4 Max):
PipelineLibraryMTL, then crashThe macOS
impeller_unittests(CI filter, 2426 passed, 0 failed) andmetal_unittestspass, and the format and clang-tidy checks are clean.#189899 (on master) makes these lookups rarer but does not remove them.
SamplerLibraryMTLandPipelineLibraryGLESlook similar and will be reported separately.Fixes #193721
Pre-launch Checklist
///).