[iOS] Create FlutterFMLTaskRunner(s)#185617
Conversation
|
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. |
There was a problem hiding this comment.
Code Review
This pull request introduces FlutterFMLTaskRunner and FlutterFMLTaskRunners to the iOS platform, providing Objective-C wrappers for FML task runners. The review feedback identifies a need to remove C++ types from public headers to maintain Swift compatibility, a potential undefined behavior when handling null labels, and a possible clock mismatch in scheduled tasks.
4585234 to
13eedca
Compare
Good point -- the underlying task runners are tested, and there's heavy testing of these in PlatformViewsController.mm (once migrated) but having distinct tests for these is useful, particularly in a future where we look at embedder API migration. Will add. |
13eedca to
7a824ee
Compare
7a824ee to
7c1590a
Compare
|
Will get some tests written for this today then this should be good to go. The code itself is ready for feedback though :) |
7c1590a to
390f67a
Compare
390f67a to
38d13f2
Compare
38d13f2 to
a9c759c
Compare
a9c759c to
5a32069
Compare
5a32069 to
e5a3eb6
Compare
fml::TaskRunner is a core C++ dependency for asynchronous task execution across the iOS embedder. It appears frequently in Objective-C API which is problematic for adding or migrating embedder code to Swift. This class also makes heavy use of fml::TimePoint and fml::TimeDelta, and thus adds further C++ to any code making use of the task runner. flutter::TaskRunners, is a container that groups together task runners for the platform thread, UI thread, and raster thread. This adds FlutterFMLTaskRunner and FlutterFMLTaskRunners. Ideally, these would be named FlutterTaskRunner but that identifier is already a key part of the Embedder API. To differentiate our FML-based run loops from the Embedder API types, we add FML to the name. The two new classes have API that is entirely free from C++ types, but for each, we introduce a category on it in the FlutterTaskRunner[s]+FML.h header that includes an initialiser that takes the associated C++ type and a getter that returns it. This allows for a stepwise migration. This patch adds, but does not yet migrate to, the new wrapper types. I'll send follow-up patches with the migrations.
e5a3eb6 to
bb25e2b
Compare
| let startTime = CACurrentMediaTime() | ||
| taskRunner.postTask(delay: 0.1) { | ||
| let endTime = CACurrentMediaTime() | ||
| XCTAssertGreaterThanOrEqual(endTime - startTime, 0.1) |
There was a problem hiding this comment.
can there be rounding issues? maybe add an epsilon?
There was a problem hiding this comment.
let epsilon = 0.01
XCTAssertGreaterThanOrEqual(endTime - startTime, 0.1 - epsilon)
There was a problem hiding this comment.
Yep this is reasonable.
Since both CACurrentMediaTime() and std::chrono::steady_clock (which is what we use in TaskRunner) from our libcxx fork are implemented on top of mach_absolute_time(), and because we're comparing a difference of CACurrentMediaTime() here, the FP error in 0.1 is ~95 nanoseconds compared to the O(microseconds) extra execution time we're going to get in the code that performs the async task execution. The odds of this flaking are vanishingly small but the 1 in a few thousand flakes are the most annoying.
Since pushing a fix would invalidate the approval and add another day to this, and because the odds of this flake occurring approach zero, I'll land and push a two-line fix in a followup.
We could fix this by selecting an inverse-power-of-two like .125 with no FP error, but then we'll hit the 1 in a billion flake when an NTP server sync adjusts the clock inside the 100ms task window 🤣.
Sent the followup in #185729.
fml::TaskRunner is a core C++ dependency for asynchronous task execution across the iOS embedder. It appears frequently in Objective-C API which is problematic for adding or migrating embedder code to Swift. This class also makes heavy use of fml::TimePoint and fml::TimeDelta, and thus adds further C++ to any code making use of the task runner.
flutter::TaskRunners, is a container that groups together task runners for the platform thread, UI thread, and raster thread.
This adds FlutterFMLTaskRunner and FlutterFMLTaskRunners. Ideally, these would be named FlutterTaskRunner but that identifier is already a key part of the Embedder API. To differentiate our FML-based run loops from the Embedder API types, we add FML to the name.
The two new classes have API that is entirely free from C++ types, but for each, we introduce a category on it in the FlutterTaskRunner[s]+FML.h header that includes an initialiser that takes the associated C++ type and a getter that returns it. This allows for a stepwise migration.
This patch adds, but does not yet migrate to, the new wrapper types. I'll send follow-up patches with the migrations.
Issue: #157140
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.