Repository navigation
iOS,macOS: Compile Swift with opt flags in opt builds - #191622
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 updates the compiler configuration in BUILD.gn to define Swift optimization flags (swiftflags) corresponding to different optimization levels, such as -Osize, -O, and -Onone. Additionally, in FlutterRunLoop.swift, an assert statement checking if the execution is on the main thread has been replaced with a precondition statement to ensure the check is enforced in release builds. There are no review comments, and I have no feedback to provide.
We never passed optimisation flags to `swiftc`, and the compiler's default is `-Onone`, so all of the Darwin embedders' Swift shipped unoptimised in profile and release, and `assert()`s triggered in shipping binaries, as seen in flutter#190909. This updates our optimisation gn configs to set equivalent `swiftflags` alongside the `cflags` we already set. For iOS that means `-Osize` (to match `-Os` for Obj-C/C++). For macOS, that means `-O` (to match `-O2` for Obj-C/C++, Swift just has -O, no -O2, -O3). For both, we set and `-Onone` for `:no_optimize`. When `is_debug` is true, we default to unopt. Swift `assert()` behaves like `FML_DCHECK` (active in unopt, compiled out in opt builds) and we should use `precondition()` where we'd use `FML_CHECK` -- i.e. cases we want to assert even in release builds. The following dev-specific asserts are now compiled out from shipped engines: * `DisplayLinkManager.shared` main-thread check * `KeyboardInsetManager` vsync client internal-state check * `FlutterRunLoop.mainRunLoop` the guard check already triggers * `TraceScope.deinit` ensure `end()` was called This also moves the main-thread check in `FlutterRunLoop.ensureMainLoopInitialized` to a `precondition`. Binding the main run loop to the wrong thread silently leaves everything later scheduled on it running on the wrong thread so we should continue to blow up in all modes. This change chops 130272 bytes (0.79%) off `Flutter.framework` on `ios_release`. Issue: flutter#191616 Related: flutter#190909 No new tests, since this is a compile config change: the build *is* the test, along with not breaking the existing tests.
3573d30 to
637781c
Compare
|
test-exempt: configuration change |
We never passed optimisation flags to
swiftc, and the compiler's default is-Onone, so all of the Darwin embedders' Swift shipped unoptimised in profile and release, andassert()s triggered in shipping binaries, as seen in #190909.This updates our optimisation gn configs to set equivalent
swiftflagsalongside thecflagswe already set. For iOS that means-Osize(to match-Osfor Obj-C/C++). For macOS, that means-O(to match-O2for Obj-C/C++, Swift just has -O, no -O2, -O3). For both, we set and-Ononefor:no_optimize.When
is_debugis true, we default to unopt. Swiftassert()behaves likeFML_DCHECK(active in unopt, compiled out in opt builds) and we should useprecondition()where we'd useFML_CHECK-- i.e. cases we want to assert even in release builds.The following dev-specific asserts are now compiled out from shipped engines:
DisplayLinkManager.sharedmain-thread checkKeyboardInsetManagervsync client internal-state checkFlutterRunLoop.mainRunLoopthe guard check already triggersTraceScope.deinitensureend()was calledThis also moves the main-thread check in
FlutterRunLoop.ensureMainLoopInitializedto aprecondition. Binding the main run loop to the wrong thread silently leaves everything later scheduled on it running on the wrong thread so we should continue to blow up in all modes.This change chops 130272 bytes (0.79%) off
Flutter.frameworkonios_release.Issue: #191616
Related: #190909 (demonstrates existing behaviour)
No new tests, since this is a compile config change: the build is the test, along with not breaking the existing tests.
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.