Repository navigation
[web] Use thread local strike caches in skwasm - #190048
auto-submit[bot] merged 13 commits into
Conversation
github-actions
Bot
added
a: text input
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for 'wasm_use_pthreads' in the build configuration and adds a concurrent text layout and rasterization test. It also dispatches resource cache limit updates to the raster worker thread in Skwasm, making 'current_callback_id_' atomic to support multi-threaded access. The reviewer recommends persistently storing the resource cache limit on the surface rather than clearing it after the initial setup, ensuring the limit is preserved if the render context is recreated.
fa1d523 to
d64e4b3
Compare
Multithreaded skwasm was built with -sWASM_WORKERS but without -pthread, which links emscripten's single-threaded stub system libraries: every lock in Skia, FreeType, and libc++ was a no-op while two threads shared linear memory, corrupting the heap under concurrent text layout and rasterization. Add a wasm_use_pthreads GN arg that links the pthread-enabled "-mt" system libraries while still creating threads as wasm workers, and enable it for the skwasm toolchains. Also make the cross-thread callback id atomic, dispatch surface_setResourceCacheLimitBytes to the raster worker that owns the render context, and add a regression test driving concurrent text layout and rasterization. Fixes #190039
d64e4b3 to
3f0a645
Compare
Renzo-Olivares
removed
the
a: text input
github-actions
Bot
added
the
a: text input
|
@ColeSpringer As I mentioned in the GitHub issue, I think we're going to try to avoid turning pthreads on, but I was wondering if you could maybe change this PR to try using the hidden https://skia.googlesource.com/skia/+/refs/heads/main/modules/skottie/src/SkottieTool.cpp#474 Skia is currently looking at making this flag more official so that it's not so ugly to use, but for the time being I'd like to see if your use case is fixed by this. |
|
Also, I was looking at the change which makes |
|
@eyebrowsoffire Went ahead and switched out the pthreads implementation with the skia flag you mentioned. Ran the tests with the flag off and the issue reappeared (tests failed). Running with the flag on resulted in all of the tests passing. |
harryterkelsen
left a comment
harryterkelsen
left a comment
There was a problem hiding this comment.
LGTM. I think it's safe to use the experimental flag here. According to discussions with the Skia team, the flag is mostly safe, but not turned on by default because it breaks specific Chrome non-standard use cases. We should be okay using it.
|
Please update the title of the PR to reflect the actual fix (enabling the experimental flag) |
eyebrowsoffire
left a comment
eyebrowsoffire
left a comment
There was a problem hiding this comment.
I agree with @harryterkelsen, I think we should actually just land this. As he mentioned, please update the PR description and title so that the commit message is accurate.
ColeSpringer
changed the title
|
PR title and description updated. |
harryterkelsen
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
…12420) Manual roll requested by [email protected] flutter/flutter@b766512...27b0988 2026-08-05 [email protected] reland(tool): remove redundant --enable-experiment=record-use flag (flutter/flutter#190591) 2026-08-05 [email protected] Windows: Propagate enabled accessibility state (flutter/flutter#190507) 2026-08-05 [email protected] Roll Fuchsia Test Scripts from ltbuIH9Z3T_yOuigu... to vcANVO8VIDQHasH1X... (flutter/flutter#190589) 2026-08-05 [email protected] Document frozen embedder API structs (flutter/flutter#186842) 2026-08-05 [email protected] Document super call order for State.didChangeDependencies (flutter/flutter#185945) 2026-08-05 [email protected] Move examples of `flutter/widgets` widgets out from `flutter/material` (flutter/flutter#189532) 2026-08-05 [email protected] [web] Use thread local strike caches in skwasm (flutter/flutter#190048) 2026-08-05 [email protected] doc: fix typo in see also section for PrimaryScrollController.maybeOf (flutter/flutter#190386) 2026-08-05 [email protected] Migrate the shell unit tests from legacy Dart native functions to FFI (flutter/flutter#190473) 2026-08-05 [email protected] render proxy box now defaults baseline calculation to null (flutter/flutter#190269) 2026-08-05 [email protected] Update Widgets Localizations from Translation Console (flutter/flutter#190503) 2026-08-04 [email protected] Revert: fix(tool): remove redundant --enable-experiment=record-use flag (flutter/flutter#190583) 2026-08-04 [email protected] Roll Fuchsia Test Scripts from 1frGe_KltAJKkeyPg... to ltbuIH9Z3T_yOuigu... (flutter/flutter#190561) 2026-08-04 [email protected] iOS,macOS: add tsan and ubsan support for Swift (flutter/flutter#190497) 2026-08-04 [email protected] fix: update on_message_ to nullptr after window destroy so that dart gets destroy message (flutter/flutter#185807) 2026-08-04 [email protected] [flutter_tools] Gracefully handle locked Windows files during clean (flutter/flutter#190095) 2026-08-04 [email protected] iOS,macOS: make Logger thread-safe, conform to Sendable (flutter/flutter#190488) 2026-08-04 [email protected] iOS: Eliminate use of IOSContextNoop in platform view tests (reland) (flutter/flutter#190509) 2026-08-04 [email protected] fix(tool): remove redundant --enable-experiment=record-use flag (flutter/flutter#190475) 2026-08-04 [email protected] android_hardware_smoke_test: Detect blank image failures or EGL initialization warnings and retry (flutter/flutter#190110) 2026-08-04 [email protected] Bumps text gamma on windows to match skia. (flutter/flutter#190477) 2026-08-04 [email protected] Roll Skia from 48b58ee222f1 to a8583a0a2c11 (2 revisions) (flutter/flutter#190537) 2026-08-04 [email protected] [tool][web] Intercept dart2wasm errors & append JS migration footers (flutter/flutter#190476) 2026-08-04 [email protected] [record_use] Migrate IconTreeShaker to `package:record_use` (flutter/flutter#190225) 2026-08-04 [email protected] Handle unexpected exceptions during Azure metadata detection (flutter/flutter#189457) 2026-08-04 [email protected] Fix merge conflict from flutter/flutter#190369 (flutter/flutter#190544) 2026-08-04 [email protected] Prepare device support symbols (flutter/flutter#190369) 2026-08-04 [email protected] Roll Packages from ac87e65 to 3498b9d (1 revision) (flutter/flutter#190532) 2026-08-04 [email protected] Roll Skia from a08d918ebd6a to 48b58ee222f1 (11 revisions) (flutter/flutter#190527) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
|
when can we expect fix of this issue release on 3.47 stable line? |
After flutter#190048, we no longer have multthreading issues with wimp, so we should turn multithreading on. This addresses flutter#178749
Reverts: [[wimp] Turn multithreading on for wimp.](flutter#190626) Initiated by: @bkonyi Reason for reverting: [failing Linux linux_web_engine_tests on tree](https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20linux_web_engine_tests/9022). Original PR Author: @eyebrowsoffire Reviewed By: @mdebbar The original PR description is provided below: After flutter#190048, we no longer have multthreading issues with wimp, so we should turn multithreading on. This addresses flutter#178749
Reverts: [[wimp] Turn multithreading on for wimp.](flutter#190626) Initiated by: @bkonyi Reason for reverting: [failing Linux linux_web_engine_tests on tree](https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20linux_web_engine_tests/9022). Original PR Author: @eyebrowsoffire Reviewed By: @mdebbar The original PR description is provided below: After flutter#190048, we no longer have multthreading issues with wimp, so we should turn multithreading on. This addresses flutter#178749
5 participants
Description
Multithreaded skwasm is built with
-sWASM_WORKERSbut without-pthread, whichlinks the single threaded emscripten system libraries where mutexes are no-ops.
Text layout on the main thread and the raster worker share the global
SkStrikeCache, so under heavy text churn the two threads corrupt the heap andthe tab ends up pinned at 100% CPU.
The fix is Skia's experimental thread local strike cache flag, enabled from a
constructor in
surface.cc. Each thread gets its own strike cache, so there is noshared state left to race on.
Two adjacent fixes:
context_lost_callback_id_was allocated on the worker by incrementingcurrent_callback_id_, which the main thread also increments. It is nowallocated on the main thread in
SetCanvas.surface_setResourceCacheLimitBytestouched the worker ownedGrDirectContextfrom the main thread. It now dispatches to the worker, and the value is stored
and reapplied when the render context is recreated. This also fixes a null deref
when Dart sets the limit before surface init.
Tests
New regression test
lib/web_ui/test/skwasm/concurrent_text_layout_raster_test.dart:text layout with never repeating font parameters on the main thread while the
worker rasterizes the previous frame, disposing everything per frame and toggling
the resource cache limit. Without the flag it fails 2 of 2 runs (one wasm timeout,
one hard page freeze); with it, 6 of 6 pass. The context loss suite passes 4 of 4,
covering the callback id and cache limit changes.
No golden churn is expected.
Fixes #190039
Possibly also fixes #184858, which looks like the same corruption from the dispose
path.
Pre-launch Checklist
///).