Repository navigation
[skwasm] Implement Wasm worker futex semaphores - #194139
Conversation
Overrides Emscripten's no-op `-sWASM_WORKERS=1` POSIX semaphore stubs (`sem_init`, `sem_destroy`, `sem_post`, `sem_wait`, `sem_trywait`) via linker `--wrap` in multi-threaded Skwasm and WIMP builds, sleeping via `emscripten_atomic_wait_u32` on DedicatedWorker threads and using an atomic compare-exchange spin-acquire on the browser main thread. Fixes flutter#193695
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If both you and the PR author believe this PR qualifies for a test exemption, escalate to the tech lead for the changed area to review and apply the |
There was a problem hiding this comment.
Code Review
This pull request introduces a custom semaphore implementation for Emscripten WASM workers in Skwasm, wrapping standard POSIX semaphore functions via linker flags. The review feedback suggests adding a null check to sem_destroy for POSIX compliance and optimizing the atomic retry loop in sem_post by moving the initial atomic load outside the loop.
…3_HEAD_FLUTTER_HASH
harryterkelsen
left a comment
There was a problem hiding this comment.
Thanks for this fix!
…ise worker toByteData()
Replaces Emscripten's no-op `-sWASM_WORKERS=1` POSIX semaphore stubs (`sem_init`, `sem_destroy`, `sem_post`, `sem_wait`, `sem_trywait`) with a WebAssembly atomic futex implementation in multi-threaded Skwasm and Web Impeller (`wimp`) builds. ### Rationale Under Emscripten's `-sWASM_WORKERS=1` runtime without `-pthread` (`skwasm_heavy` / `wimp_heavy`), `libc-ww.a` compiles `system/lib/pthread/library_pthread_stub.c`, which stubs `sem_init`, `sem_wait`, `sem_trywait`, `sem_post`, and `sem_destroy` (as well as `emscripten_futex_wait` / `emscripten_futex_wake`) to `return 0`. In Skia (`third_party/skia/src/core/SkSemaphore.cpp`), `SkSemaphore` uses POSIX `sem_t` on `__EMSCRIPTEN__` as the contended slow-path backing for `SkMutex` and `SkSharedMutex`. Because `sem_wait` is a no-op, any contended acquisition on FreeType's global `f_t_mutex()` (`third_party/skia/src/ports/SkFontHost_FreeType.cpp:221-226`) or `SkTypeface_FreeType::fC2GCacheMutex` (`third_party/skia/src/ports/SkTypeface_FreeType.h:87`) returns immediately when the browser main UI thread performs `Paragraph::layout` / `MakeTextFrameFromTextBlobSkia` concurrently with the background render worker rasterizing glyphs from the shared `FT_Library` / `FT_Face`, causing memory corruption and worker stalls ([flutter#193695](flutter#193695)). ### Summary of Changes - [`engine/src/flutter/skwasm/wasm_worker_semaphore.cc`](https://github.com/kevmoo/flutter/blob/87e66a27b6b/engine/src/flutter/skwasm/wasm_worker_semaphore.cc) & [`engine/src/flutter/skwasm/BUILD.gn`](https://github.com/kevmoo/flutter/blob/87e66a27b6b/engine/src/flutter/skwasm/BUILD.gn): Override `sem_init`, `sem_destroy`, `sem_post`, `sem_wait`, and `sem_trywait` via linker `-Wl,--wrap=sem_*` in multi-threaded `skwasm_variant` (`#if defined(__EMSCRIPTEN_WASM_WORKERS__)`) builds: - Stores the atomic counter inline in `sem_t`'s 16-byte `__val` buffer (`cache/sysroot/include/semaphore.h:17-19`), requiring zero heap allocation or teardown in `sem_destroy`. - Background `DedicatedWorker` threads (`emscripten_wasm_worker_self_id() != 0`, cached in a `thread_local bool` to avoid a Wasm-to-JS FFI call per contended wait) sleep via `emscripten_atomic_wait_u32` and wake via `emscripten_atomic_notify`. - The browser main thread (`emscripten_wasm_worker_self_id() == 0`, where `memory.atomic.wait32` / `Atomics.wait` is disallowed by the WebAssembly Threads spec because `[[CanBlock]] == false`) uses an atomic compare-exchange spin-acquire loop. ### Verification - Built `wasm_release:skwasm/skwasm_heavy.js` and `wasm_release:skwasm/wimp_heavy.js` and verified `20/20` multi-threaded text-grid runs (`grid@100` and `grid@1000`) complete without `FT_Face` memory corruption or worker stalls in Chrome 154. Fixes flutter#193695 ## 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/tests]: https://github.com/flutter/tests [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 [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
…188628) Related: flutter#187660. Part 1 of 5 from the original hybrid-rasterizer change. This PR is now limited to the independent benchmark instrumentation. Adds `presentation_duration` around preparation and frame submission, including asynchronous raster work, and records it in the web benchmark harness. The timer stops before reporting, and benchmark listeners are cleaned up even when startup fails. The existing profile JSON schema is unchanged. Verified locally: two engine timing tests cover enabled/disabled profiling and delayed preparation/rasterization; a browser regression test covers listener cleanup after synchronous startup failures. Static analysis and formatting pass. This change is independent of the hybrid rasterizer. The remaining pieces are separate drafts: - flutter#195103: native callback IDs and optional bitmap capture. - flutter#195104: platform-view DOM commit after rasterization. - flutter#195105: opt-in hybrid presentation, depending on the two changes above. - flutter#195106: desktop default policy and benchmark results, depending on the opt-in implementation. The duplicate semaphore and codec changes were dropped on rebase because master already includes flutter#194139 and flutter#193187. ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant in-code documentation (doc comments with `///`). - [ ] 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). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- 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/CONTRIBUTING.ai.md [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/tests]: https://github.com/flutter/tests [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 [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
|
Could this get |
|
@erabti I'm digging in here! If you don't get a reply by Tuesday next week, feel free to ping me directly! |
Adds a manually dispatched "Web deploy" workflow: scripts/build_web.sh on ubuntu with the Flutter release given as an input (default 3.51.0-0.1.pre on beta, which carries the multi-threaded Skwasm futex fix, flutter/flutter#194139, for the text-layout/glyph-raster race 3.50.0 still has, #193695), then scripts/deploy_web.sh from main in the release environment. The deploy is tagged with the app version and messaged "summary; sha; version; webapp only", like the hand-run deploys, and checked against the active Worker version. Building alone works from any branch. The deploy job needs CLOUDFLARE_API_TOKEN and CLOUDFLARE_ACCOUNT_ID in the release environment. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Replaces Emscripten's no-op
-sWASM_WORKERS=1POSIX semaphore stubs (sem_init,sem_destroy,sem_post,sem_wait,sem_trywait) with a WebAssembly atomic futex implementation in multi-threaded Skwasm and Web Impeller (wimp) builds.Rationale
Under Emscripten's
-sWASM_WORKERS=1runtime without-pthread(skwasm_heavy/wimp_heavy),libc-ww.acompilessystem/lib/pthread/library_pthread_stub.c, which stubssem_init,sem_wait,sem_trywait,sem_post, andsem_destroy(as well asemscripten_futex_wait/emscripten_futex_wake) toreturn 0.In Skia (
third_party/skia/src/core/SkSemaphore.cpp),SkSemaphoreuses POSIXsem_ton__EMSCRIPTEN__as the contended slow-path backing forSkMutexandSkSharedMutex. Becausesem_waitis a no-op, any contended acquisition on FreeType's globalf_t_mutex()(third_party/skia/src/ports/SkFontHost_FreeType.cpp:221-226) orSkTypeface_FreeType::fC2GCacheMutex(third_party/skia/src/ports/SkTypeface_FreeType.h:87) returns immediately when the browser main UI thread performsParagraph::layout/MakeTextFrameFromTextBlobSkiaconcurrently with the background render worker rasterizing glyphs from the sharedFT_Library/FT_Face, causing memory corruption and worker stalls (flutter/flutter#193695).Summary of Changes
engine/src/flutter/skwasm/wasm_worker_semaphore.cc&engine/src/flutter/skwasm/BUILD.gn: Overridesem_init,sem_destroy,sem_post,sem_wait, andsem_trywaitvia linker-Wl,--wrap=sem_*in multi-threadedskwasm_variant(#if defined(__EMSCRIPTEN_WASM_WORKERS__)) builds:sem_t's 16-byte__valbuffer (cache/sysroot/include/semaphore.h:17-19), requiring zero heap allocation or teardown insem_destroy.DedicatedWorkerthreads (emscripten_wasm_worker_self_id() != 0, cached in athread_local boolto avoid a Wasm-to-JS FFI call per contended wait) sleep viaemscripten_atomic_wait_u32and wake viaemscripten_atomic_notify.emscripten_wasm_worker_self_id() == 0, wherememory.atomic.wait32/Atomics.waitis disallowed by the WebAssembly Threads spec because[[CanBlock]] == false) uses an atomic compare-exchange spin-acquire loop.Verification
wasm_release:skwasm/skwasm_heavy.jsandwasm_release:skwasm/wimp_heavy.jsand verified20/20multi-threaded text-grid runs (grid@100andgrid@1000) complete withoutFT_Facememory corruption or worker stalls in Chrome 154.Fixes #193695
Pre-launch Checklist
///).