This repository was archived by the owner on Feb 25, 2025. It is now read-only.
iOS: Eliminate needless profiler metrics ivar #55957
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
iOS profiling is implemented in ProfileMetricsIOS, which has a default constructor, non member fields, and no superclass. i.e. it's roughly zero cost to create and holds no state. There's therefore no reason to make it an ivar on FlutterEngine. Further, making it an ivar on FlutterEngine means that the profiling sampler lambda needs to capture self which, in the case where all other FlutterEngine references go out of scope during sampling, means that the last FlutterEngine reference exists on a profiling thread, and when the sampling lambda block completes, the engine is dealloc'ed. The engine *must* be deallocated on the platform (main in Apple terminology) thread because FlutterEngine holds a reference to a PlatformViewsController, which owns a WeakPtrFactory. WeakPtrFactory's dtor asserts that it executes on the thread on which it was created, in this case, the platform thread. Since there's no need for ProfileMetricsIOS to be tied to the engine, we now create it on the fly in the sampler. No test changes because no semantic changes. Uncovered by ARC migration. Issue: flutter/flutter#137801 Issue: flutter/flutter#156177
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). 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. 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. |
Contributor
|
test-exempt: code refactor with no semantic change |
stuartmorgan-g
approved these changes
Oct 18, 2024
Contributor
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
LGTM
I'd love to see the class replaced with a static function, but no need for that in this PR.
cbracken
added
the
autosubmit
hellohuanlin
approved these changes
Oct 18, 2024
This was referenced Oct 18, 2024
auto-submit bot
pushed a commit
that referenced
this pull request
Oct 18, 2024
This reverts commit 7525656. This relands commit 02fc8a4. That commit was a reland of 449df25. Migrates `FlutterEngine` from manual reference counting to ARC. Migrates properties from `retain` to strong and `assign` to `weak` (where referencing an Obj-C object). This has the following change from the previous patch: * startProfiler was updated to eliminate the self-capture in #55957 which prevents the possibility of dealloc on the profiling thread. No semantic changes, therefore no changes to tests. Issue: flutter/flutter#137801 Issue: flutter/flutter#156177 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Oct 18, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Oct 19, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Oct 19, 2024
jonahwilliams
pushed a commit
to flutter/flutter
that referenced
this pull request
Oct 19, 2024
…157215) flutter/engine@76d310e...dd37446 2024-10-19 [email protected] Move keymap from FlKeyboardViewDelegate to FlKeyboardManager (flutter/engine#55942) 2024-10-19 [email protected] [UI] fix scene builder parameter naming. (flutter/engine#55969) 2024-10-19 [email protected] iOS: Improve thread safety of first frame callback (flutter/engine#55966) 2024-10-18 [email protected] iOS: Fix flaky tests (remove timeouts) (flutter/engine#55961) 2024-10-18 [email protected] [Impeller] allocate the impeller onscreen texture from the render target cache. (flutter/engine#55943) 2024-10-18 [email protected] Roll Fuchsia Linux SDK from 9F_NaKPd2twhbPwP7... to tNQZ8d5mRYpe3--lk... (flutter/engine#55963) 2024-10-18 [email protected] Started filtering out close line segments in rrect polylines. (flutter/engine#55929) 2024-10-18 [email protected] [Impeller] libImpeller: Allow custom font registrations. (flutter/engine#55934) 2024-10-18 [email protected] Re-reland "iOS: Migrate FlutterEngine to ARC" (flutter/engine#55962) 2024-10-18 [email protected] [Impeller] libImpeller: Add a README. (flutter/engine#55940) 2024-10-18 [email protected] iOS: Eliminate needless profiler metrics ivar (flutter/engine#55957) 2024-10-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] one descriptor pool per frame. (#55939)" (flutter/engine#55959) 2024-10-18 [email protected] Revert "Reland "iOS: Migrate FlutterEngine to ARC" (#55937)" (flutter/engine#55954) 2024-10-18 [email protected] Roll Dart SDK from 993d3069f42e to a51df90298ca (7 revisions) (flutter/engine#55951) 2024-10-18 [email protected] [engine] add back opt out for merged threads. (flutter/engine#55952) 2024-10-18 [email protected] [Impeller] one descriptor pool per frame. (flutter/engine#55939) 2024-10-18 [email protected] [Impeller] add mechanism for sharing bdf inputs. (flutter/engine#55701) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 9F_NaKPd2twh to tNQZ8d5mRYpe If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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
nick9822
pushed a commit
to nick9822/flutter
that referenced
this pull request
Dec 18, 2024
iOS profiling is implemented in ProfileMetricsIOS, which has a default constructor, no member fields, and no superclass. i.e. it's roughly zero cost to create and holds no state. There's therefore no reason to make it an ivar on FlutterEngine. Further, making it an ivar on FlutterEngine means that the profiling sampler lambda needs to capture self which, in the case where all other FlutterEngine references go out of scope during sampling, means that the last FlutterEngine reference exists on a profiling thread, and when the sampling lambda block completes, the engine is dealloc'ed. The engine *must* be deallocated on the platform (main in Apple terminology) thread because FlutterEngine holds a reference to a PlatformViewsController, which owns a WeakPtrFactory. WeakPtrFactory's dtor asserts that it executes on the thread on which it was created, in this case, the platform thread. Further, the engine holds direct and indirect references to other UIKit objects such as `UITextInput` in FlutterTextInputPlugin, which must be dealloc'ed on the main thread. Since there's no need for ProfileMetricsIOS to be tied to the engine, we now create it on the fly in the sampler. No test changes because no semantic changes. Uncovered by ARC migration. Issue: flutter#137801 Issue: flutter#156177 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
nick9822
pushed a commit
to nick9822/flutter
that referenced
this pull request
Dec 18, 2024
This reverts commit 0a9ed47. This relands commit 530a9f8. That commit was a reland of a2ac734. Migrates `FlutterEngine` from manual reference counting to ARC. Migrates properties from `retain` to strong and `assign` to `weak` (where referencing an Obj-C object). This has the following change from the previous patch: * startProfiler was updated to eliminate the self-capture in flutter/engine#55957 which prevents the possibility of dealloc on the profiling thread. No semantic changes, therefore no changes to tests. Issue: flutter#137801 Issue: flutter#156177 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
iOS profiling is implemented in ProfileMetricsIOS, which has a default constructor, no member fields, and no superclass. i.e. it's roughly zero cost to create and holds no state. There's therefore no reason to make it an ivar on FlutterEngine.
Further, making it an ivar on FlutterEngine means that the profiling sampler lambda needs to capture self which, in the case where all other FlutterEngine references go out of scope during sampling, means that the last FlutterEngine reference exists on a profiling thread, and when the sampling lambda block completes, the engine is dealloc'ed.
The engine must be deallocated on the platform (main in Apple terminology) thread because FlutterEngine holds a reference to a PlatformViewsController, which owns a WeakPtrFactory. WeakPtrFactory's dtor asserts that it executes on the thread on which it was created, in this case, the platform thread. Further, the engine holds direct and indirect references to other UIKit objects such as
UITextInputin FlutterTextInputPlugin, which must be dealloc'ed on the main thread.Since there's no need for ProfileMetricsIOS to be tied to the engine, we now create it on the fly in the sampler.
No test changes because no semantic changes.
Uncovered by ARC migration.
Issue: flutter/flutter#137801
Issue: flutter/flutter#156177
Issue: flutter/flutter#157175
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.