Repository navigation
iOS: Configure test task runner thread at display QoS - #190283
Conversation
The iOS unit tests for VSyncClient run it on a worker thread and block
the test thread waiting on that worker. Xcode's Thread Performance
Checker flags this as a priority inversion: the waiting thread runs at a
higher quality-of-service class than the worker it is blocked on,
producing the following error message:
Thread Performance Checker: Thread running at User-interactive
quality-of-service class waiting on a lower QoS thread running at
Default quality-of-service class. Investigate ways to avoid priority
inversions
FlutterFMLTaskRunnerTestHelper creates the worker with a bare
fml::Thread, which by default is configured only with a thread name but
leaves the thread at the default QoS. In production, the platform/UI
thread that a VSyncClient posts to is created at ThreadPriority::kDisplay
(user-interactive) by IOSPlatformThreadConfigSetter in FlutterEngine.mm,
so the test worker was running below the priority of the real thread
it's attempting to represent in the tests.
This adds a config setter to raises it to QOS_CLASS_USER_INTERACTIVE
(i.e. kDisplay). This prevents the priority inversion since the worker
is now at least as high-priority as any thread that waits on it, which
accurately reflects how it's used in the real embedder.
|
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 you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
github-actions
Bot
added
platform-ios
![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 updates FlutterFMLThreadTaskRunner to configure its worker thread with user-interactive QoS and kDisplay priority. The review feedback points out a potential crash if the label parameter is nil, which would cause undefined behavior when constructing ThreadConfig, and suggests safely falling back to an empty string.
| _thread = std::make_unique<fml::Thread>( | ||
| ConfigureThread, | ||
| fml::Thread::ThreadConfig(label.UTF8String, fml::Thread::ThreadPriority::kDisplay)); |
There was a problem hiding this comment.
If label is nil, label.UTF8String will return nullptr. Passing nullptr to the std::string constructor (which is used by fml::Thread::ThreadConfig) results in undefined behavior and will cause a crash. To prevent this, we should safely handle a nil label by falling back to an empty string "".
| _thread = std::make_unique<fml::Thread>( | |
| ConfigureThread, | |
| fml::Thread::ThreadConfig(label.UTF8String, fml::Thread::ThreadPriority::kDisplay)); | |
| _thread = std::make_unique<fml::Thread>( | |
| ConfigureThread, | |
| fml::Thread::ThreadConfig(label ? label.UTF8String : "", fml::Thread::ThreadPriority::kDisplay)); |
| // which is `kDisplay` (user-interactive). | ||
| static void ConfigureThread(const fml::Thread::ThreadConfig& config) { | ||
| fml::Thread::SetCurrentThreadName(config); | ||
| pthread_set_qos_class_self_np(QOS_CLASS_USER_INTERACTIVE, 0); |
There was a problem hiding this comment.
should this be
if config.priority == kDisplay {
use QOS_CLASS_USER_INTERACTIVE
} else if config.priority == other things {
use another qos?
}
There was a problem hiding this comment.
I'd say follow the YAGNI principle here.
This adds complexity that we're not using now. Even if we were using this elsewhere, right now we have two cases:
- Platform/UI thread: user interactive
- Raster thread: user interactive
There was a problem hiding this comment.
YAGNI sounds good. My concern is that we are passing in the config.threadPriority and then ignoring it, which can be misleading. The caller would (rightfully) assume that setting a priority actually does something.
Would it make sense to either (1) map config.threadPriority to the corresponding QoS, or (2) simplify things by removing config.threadPriority altogether?
There was a problem hiding this comment.
Fair point. What we were doing here matches how the real embedder configures things, but the real embedder also has a callback that maps that back and set the priority. If we were going to do (1), we should extract the real logic from FlutterEngine into common code shared between embedder and tests; this might be overkill though and is a much bigger change.
Going to go with (2) for now as it's simpler and this isn't code that's likely to change, thanks for pushing on this. I suspect post embedder API migration this actually ends up being much simpler.
Done.
9f1ac77 to
53755a3
Compare
…12401) Manual roll Flutter from 2757a77a73df to e52f01c920ad (47 revisions) Manual roll requested by [email protected] flutter/flutter@2757a77...e52f01c 2026-07-31 [email protected] [Impeller] Execute PipelineCompileQueue jobs in the order that they were added (flutter/flutter#190262) 2026-07-31 [email protected] Roll Packages from 7d17fc8 to 5351d8c (5 revisions) (flutter/flutter#190351) 2026-07-31 [email protected] Roll Skia from dcbd015c9646 to 7ef86a5b0eb9 (4 revisions) (flutter/flutter#190347) 2026-07-31 [email protected] [Impeller] Do not acquire an unneeded image in KHRSwapchainVK if the swapchain must be recreated due to a size change (flutter/flutter#190116) 2026-07-31 [email protected] Remove the --enable-serial-gc engine flag (flutter/flutter#190310) 2026-07-31 [email protected] Migrate platform_channel_swift example to material_ui (flutter/flutter#190308) 2026-07-31 [email protected] macOS: Set static text accessibility role for read-only text fields (flutter/flutter#190330) 2026-07-31 [email protected] Roll Dart SDK from 96f49fc43f59 to c3acfc2479f6 (1 revision) (flutter/flutter#190336) 2026-07-31 [email protected] Roll Skia from 07f08414dd66 to dcbd015c9646 (1 revision) (flutter/flutter#190333) 2026-07-31 [email protected] [web] Autofill all AutofillGroup fields in semantics mode (flutter/flutter#187108) 2026-07-31 [email protected] iOS: Configure test task runner thread at display QoS (flutter/flutter#190283) 2026-07-31 [email protected] Roll Skia from a1109355de78 to 07f08414dd66 (5 revisions) (flutter/flutter#190328) 2026-07-31 [email protected] Support reduceMotion accessibility feature in MediaQuery (flutter/flutter#190287) 2026-07-30 [email protected] Roll Dart SDK from 43f7de5f4977 to 96f49fc43f59 (4 revisions) (flutter/flutter#190312) 2026-07-30 [email protected] fix: text selection two handles directionality. (flutter/flutter#179928) 2026-07-30 [email protected] Add java/gradle check when gradle crashes (flutter/flutter#189997) 2026-07-30 [email protected] [Impeller] Add filterQuality support to ImageFilter.shader (flutter/flutter#188544) 2026-07-30 [email protected] [flutter/flutter] Migrate multiple_windows example to material_ui (flutter/flutter#190313) 2026-07-30 [email protected] Roll Skia from 50cf67910462 to a1109355de78 (6 revisions) (flutter/flutter#190306) 2026-07-30 [email protected] Migrate swift xctests to Swift testing tests (flutter/flutter#187801) 2026-07-30 [email protected] Migrates the platform_channel example to material_ui (flutter/flutter#190253) 2026-07-30 [email protected] iOS: Drop unnecessary @mainactor from VSyncClientTests (flutter/flutter#190285) 2026-07-30 [email protected] Add nullptr check to isOpaque (flutter/flutter#189796) 2026-07-30 [email protected] [Flutter GPU] Bind uniforms and textures by cached reflection index (flutter/flutter#189820) 2026-07-30 [email protected] Migrate layers example to material_ui (flutter/flutter#190237) 2026-07-30 [email protected] Add `ShapeBorder` hit test fast paths (flutter/flutter#187962) 2026-07-30 [email protected] ci: mac-must-wait part duex (flutter/flutter#190295) 2026-07-30 [email protected] Roll Skia from 491301f393d1 to 50cf67910462 (3 revisions) (flutter/flutter#190294) 2026-07-30 [email protected] Roll Packages from dd11626 to 7d17fc8 (1 revision) (flutter/flutter#190293) 2026-07-30 [email protected] Roll Skia from 177de1940bf2 to 491301f393d1 (1 revision) (flutter/flutter#190279) 2026-07-30 [email protected] Roll Skia from b8bda91f02db to 177de1940bf2 (5 revisions) (flutter/flutter#190272) 2026-07-30 [email protected] Roll Fuchsia Linux SDK from J8rVTlBnjnUpuHYk3... to _J8wM3kyQpLN9wvRD... (flutter/flutter#190273) 2026-07-30 [email protected] Roll Skia from 78afc18c9ba0 to b8bda91f02db (3 revisions) (flutter/flutter#190268) 2026-07-30 [email protected] Revert: Split the platform and cpuArch part of TargetPlatform (flutter/flutter#190258) 2026-07-30 [email protected] Return null from RenderProxyBoxMixin.computeDryBaseline when the child has no baseline (flutter/flutter#189723) 2026-07-30 [email protected] Roll Skia from 6fea713d32ec to 78afc18c9ba0 (4 revisions) (flutter/flutter#190257) 2026-07-30 [email protected] Roll pub packages (flutter/flutter#190259) 2026-07-29 [email protected] [cross imports] Check examples cross imports (flutter/flutter#187662) 2026-07-29 [email protected] ci: mac verify binaries still has to wait on linux_host_engine (flutter/flutter#190239) 2026-07-29 [email protected] Adds analytics to macos impeller opt in/out (flutter/flutter#190220) 2026-07-29 [email protected] Roll Skia from 0f35bba4945c to 6fea713d32ec (6 revisions) (flutter/flutter#190230) 2026-07-29 [email protected] iOS: Fix Swift warning in TracingTests (flutter/flutter#190203) 2026-07-29 [email protected] Convert all gradle-wrapper.properties in /dev to use distribution 'bin' (flutter/flutter#190226) 2026-07-29 [email protected] Roll Packages from 3e63635 to dd11626 (11 revisions) (flutter/flutter#190216) ...
3 participants
The iOS unit tests for VSyncClient run it on a worker thread and block the test thread waiting on that worker. Xcode's Thread Performance Checker flags this as a priority inversion: the waiting thread runs at a higher quality-of-service class than the worker it is blocked on, producing the following error message:
FlutterFMLTaskRunnerTestHelper creates the worker with a bare fml::Thread, which by default is configured only with a thread name but leaves the thread at the default QoS. In production, the platform/UI thread that a VSyncClient posts to is created at ThreadPriority::kDisplay (user-interactive) by IOSPlatformThreadConfigSetter in FlutterEngine.mm, so the test worker was running below the priority of the real thread it's attempting to represent in the tests.
This adds a config setter to raises it to QOS_CLASS_USER_INTERACTIVE (i.e. kDisplay). This prevents the priority inversion since the worker is now at least as high-priority as any thread that waits on it, which accurately reflects how it's used in the real embedder.
Issue: #181684
Issue: #112232
Pre-launch Checklist
///).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. Comments from the
gemini-code-assistbot 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.