Skip to content

[Impeller] Guard the sampler library caches with a mutex - #193738

Merged
auto-submit[bot] merged 4 commits into
flutter:masterfrom
leehack:impeller-sampler-library-lock
Oct 11, 2026
Merged

auto-submit[bot] merged 4 commits into
flutter:masterfrom
leehack:impeller-sampler-library-lock

Conversation

@leehack

@leehack leehack commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

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: #193721 / #193722 (the same race in PipelineLibraryMTL). Both PRs add a test to renderer_unittests.cc; merging them conflicts only on the #include <...> lines.

Fixes #193737

Pre-launch Checklist

Flutter GPU binds textures from the UI thread
(InternalFlutterGpu_RenderPass_BindTexture calls GetSampler) while
Impeller requests samplers on the raster thread (for example in
FramebufferBlendContents::Render). SamplerLibraryMTL, SamplerLibraryVK
and SamplerLibraryGLES scan and push_back a plain vector with no lock, so
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 handed out.

Guard 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 re-checks the cache so concurrent requests share one sampler.
GLES creates no device object and holds the lock for the whole lookup.

Add RendererTest.SamplerLibraryAllowsConcurrentGetSampler, which requests
the same set of samplers from four threads and checks that every thread
gets the same valid sampler for each descriptor.
@github-actions github-actions Bot added engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Oct 2, 2026
@leehack
leehack marked this pull request as ready for review October 2, 2026 21:47

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces thread safety to the sampler libraries across GLES, Metal, and Vulkan backends in Impeller. It adds mutexes to guard the sampler caches (samplers_) and implements helper methods to safely retrieve existing samplers under lock. Additionally, a concurrent unit test has been added to verify that GetSampler can be safely called from multiple threads simultaneously. There are no review comments, so no feedback is provided.

@gaaclarke gaaclarke added the team-fluttergpu Owned by Flutter GPU team label Oct 5, 2026
@gaaclarke

Copy link
Copy Markdown
Member

fyi @bdero

bdero
bdero previously approved these changes Oct 5, 2026

@bdero bdero left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks. The returned raw_ptr stays valid after the lock drops since the vector holds shared_ptrs and entries are never removed, and the recheck on insert keeps concurrent misses on one sampler.

@bdero bdero added the CICD Run CI/CD label Oct 5, 2026
@flutter-dashboard flutter-dashboard Bot removed the CICD Run CI/CD label Oct 6, 2026
@github-actions github-actions Bot removed the team-fluttergpu Owned by Flutter GPU team label Oct 6, 2026
@leehack
leehack requested a review from gaaclarke October 6, 2026 00:20
…ller-sampler-library-lock

# Conflicts:
#	engine/src/flutter/impeller/renderer/renderer_unittests.cc
@leehack

leehack commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Resolved the conflicts.

@leehack
leehack requested a review from bdero October 6, 2026 12:05

@gaaclarke gaaclarke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@gaaclarke gaaclarke added the CICD Run CI/CD label Oct 6, 2026
@bdero bdero added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 11, 2026
@auto-submit
auto-submit Bot added this pull request to the merge queue Oct 11, 2026
Merged via the queue into flutter:master with commit 6acaa6e Oct 11, 2026
29 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 11, 2026
This was referenced Oct 11, 2026
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Oct 14, 2026
…#13220)

Manual roll Flutter from d03768eef055 to 4249ccbc18a3 (158 revisions)

Manual roll requested by [email protected]

flutter/flutter@d03768e...4249ccb

2026-10-12 [email protected] [AGP 9.1.0 Migration #8] Unify add-to-app module wiring on the variant API (flutter/flutter#193717)
2026-10-12 [email protected] [master] Update Flutter DEPS to Dart 5933cb486b4aef2f8cf6b9f89a5651c0d5b0955e (flutter/flutter#193982)
2026-10-12 [email protected] [Android] Fix use-after-free of SurfaceControl.Transaction in HCPP (flutter/flutter#192969)
2026-10-12 [email protected] [windows] Add OpenGL compositor tests against ANGLE at D3D11 feature level 10_0 (flutter/flutter#194114)
2026-10-12 [email protected] Fix HDR image packing for ios (flutter/flutter#192526)
2026-10-12 [email protected] Bump customer tests to pick up material_ui and cupertino_ui (flutter/flutter#194236)
2026-10-12 [email protected] Remove no-shuffle from text_field_test.dart (flutter/flutter#193550)
2026-10-12 [email protected] [web] Clip semantics DOM text labels to stop stealing taps (flutter/flutter#194011)
2026-10-12 [email protected] Roll Skia from 9b6ceba4728d to 17f6986c226c (6 revisions) (flutter/flutter#194232)
2026-10-12 [email protected] [web] Skip non-focusable containers in Focusable.focusAsRouteDefault (flutter/flutter#194138)
2026-10-12 [email protected] [web_paragraph] Honor kTextHeightNone in TextStyle and ParagraphStyle (flutter/flutter#193733)
2026-10-12 [email protected] Roll Skia from c88b49d1d6c7 to 9b6ceba4728d (4 revisions) (flutter/flutter#194226)
2026-10-12 [email protected] Migrate remaining test to macOS 26 (flutter/flutter#193825)
2026-10-12 [email protected] Roll Dart SDK from 047ca322aa9c to 221363557163 (1 revision) (flutter/flutter#194219)
2026-10-12 [email protected] [flutter_tools] Include dev_dependencies hooks for release integration tests (flutter/flutter#193436)
2026-10-12 [email protected] Roll Skia from d8afad1b1829 to c88b49d1d6c7 (1 revision) (flutter/flutter#194198)
2026-10-12 [email protected] Roll Skia from b23ddbff1265 to d8afad1b1829 (4 revisions) (flutter/flutter#194195)
2026-10-11 [email protected] Roll Skia from a168863e44ec to b23ddbff1265 (1 revision) (flutter/flutter#194185)
2026-10-11 [email protected] Roll Skia from 0c1d93abfa04 to a168863e44ec (1 revision) (flutter/flutter#194181)
2026-10-11 [email protected] iOS: Re-enable rendering on willEnterForeground (flutter/flutter#191350)
2026-10-11 [email protected] Roll Skia from 93c9d3ffda25 to 0c1d93abfa04 (1 revision) (flutter/flutter#194179)
2026-10-11 [email protected] Remove more instances of deprecated `withOpacity` (flutter/flutter#193559)
2026-10-11 [email protected] Fix another `parameter_assignments` warning (flutter/flutter#193669)
2026-10-11 [email protected] Roll Skia from 3136abef8f58 to 93c9d3ffda25 (1 revision) (flutter/flutter#194169)
2026-10-11 [email protected] [Impeller] Guard the sampler library caches with a mutex (flutter/flutter#193738)
2026-10-11 [email protected] Roll Dart SDK from e9c89888bcc1 to 047ca322aa9c (1 revision) (flutter/flutter#194157)
2026-10-10 [email protected] Roll Dart SDK from 787baa8aae78 to e9c89888bcc1 (2 revisions) (flutter/flutter#194141)
2026-10-10 [email protected] Fix OverlayPortal layout in shrink-wrapped overlays (flutter/flutter#193079)
2026-10-10 [email protected] Roll Skia from 353076e27b1c to 3136abef8f58 (1 revision) (flutter/flutter#194130)
2026-10-10 [email protected] Roll Dart SDK from 88a07589b1ff to 787baa8aae78 (1 revision) (flutter/flutter#194124)
2026-10-10 [email protected] Route windowing view lookups through WidgetsBinding (flutter/flutter#192666)
2026-10-10 [email protected] Roll Dart SDK from 2a06cab34715 to 88a07589b1ff (1 revision) (flutter/flutter#194117)
2026-10-10 [email protected] Roll Skia from f9043b7aa798 to 353076e27b1c (2 revisions) (flutter/flutter#194115)
2026-10-10 [email protected] Refactored PipelineLibraryGLES (flutter/flutter#193985)
2026-10-09 [email protected] add shorthands for scroll physics subclasses in framework (flutter/flutter#193082)
2026-10-09 [email protected] Roll Dart SDK from ba889e3cee8a to 2a06cab34715 (1 revision) (flutter/flutter#194105)
2026-10-09 [email protected] Roll Skia from 5ea03d8a53a6 to f9043b7aa798 (2 revisions) (flutter/flutter#194099)
2026-10-09 [email protected] Add docs for using bb and led with LUCI (flutter/flutter#193859)
2026-10-09 [email protected] Roll Skia from e329bb617820 to 5ea03d8a53a6 (5 revisions) (flutter/flutter#194087)
2026-10-09 [email protected] Roll Packages from 54706bd to d8d9e97 (5 revisions) (flutter/flutter#194080)
2026-10-09 [email protected] Move the Vulkan wrapped-texture helper out of the GPU surface (flutter/flutter#193001)
2026-10-09 [email protected] Roll Skia from 3e54bcf4c963 to e329bb617820 (1 revision) (flutter/flutter#194078)
2026-10-09 [email protected] Roll Dart SDK from 23f0308f9af8 to ba889e3cee8a (2 revisions) (flutter/flutter#194077)
2026-10-09 [email protected] Bump dartdoc to 9.0.11 (flutter/flutter#194076)
...
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD 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.

[Impeller] SamplerLibrary caches are read and written from two threads without a lock (Flutter GPU)

3 participants