This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Migrate vsync_waiter_ios to ARC#52104

Merged

auto-submit[bot] merged 2 commits intoflutter:mainfrom

jmagman:vsync_waiter_ios-arc

Apr 16, 2024

Member

jmagman commented Apr 13, 2024

Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162.

Migrate vsync_waiter_ios from MRC to ARC.

Part of flutter/flutter#137801.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

jmagman self-assigned this

github-actions bot added the platform-ios label

jmagman commented

View reviewed changes

shell/platform/darwin/ios/framework/Source/VsyncWaiterIosTest.mm

Comment on lines +142 to +143

		// VSyncClient has released the CADisplayLink.
		XCTAssertNil(weakLink);

Member Author

jmagman Apr 13, 2024

This doesn't currently pass on main with the current code.

shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm

               - (void)invalidate {
-                [display_link_.get() invalidate];
+                [_displayLink invalidate];
+                _displayLink = nil;  // Break retain cycle.

Member Author

jmagman Apr 13, 2024

The test I wrote to validate the display link is released when VSyncClient invalidates and deallocs doesn't pass without this line (and also doesn't pass on main with the current code).

shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm

                 recorder->RecordVsync(frame_start_time, frame_target_time);
                 if (_allowPauseAfterVsync) {
-                  display_link_.get().paused = YES;
+                  link.paused = YES;

Member Author

jmagman Apr 13, 2024

Use link passed into onDisplayLink: instead of grabbing it again.

jmagman marked this pull request as ready for review

April 13, 2024 22:48

jmagman requested review from hellohuanlin and stuartmorgan-g

April 13, 2024 22:57

stuartmorgan-g reviewed

View reviewed changes

shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm Outdated

    
                flutter::VsyncWaiter::Callback callback_;

                fml::scoped_nsobject<CADisplayLink> display_link_;

                CADisplayLink* _displayLink;

                double current_refresh_rate_;

Contributor

stuartmorgan-g Apr 13, 2024

Optional, but maybe fix the other two names while you are here so they aren't inconsistent?

Member Author

jmagman Apr 15, 2024

Done.

shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm Outdated

    
                  // Strongly retain the the captured link until it is added to the runloop.

                  CADisplayLink* localDisplayLink = _displayLink;

                  task_runner->PostTask([localDisplayLink]() {

                    [localDisplayLink addToRunLoop:[NSRunLoop currentRunLoop] forMode:NSRunLoopCommonModes];

Contributor

stuartmorgan-g Apr 13, 2024

Optional modernization nit: NSRunLoop.currentRunLoop since it's a property.

Member Author

jmagman Apr 15, 2024

Did a sweep of this file for dot notation and other property cleanup. Moved refreshRate off the header since it's only used in the implementation file and made it a readonly property, and made displayRefreshRate a class property.

shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm Outdated

+                _displayLink = nil;  // Break retain cycle.
               }
               - (void)dealloc {

Contributor

stuartmorgan-g Apr 13, 2024 •

edited

Loading

I would remove this method; it's basically a lie (and was before as well). The display link has a strong reference to this object, so it's impossible† for dealloc to be called unless the display link has already been invalidated.

† Unless there's an overrelease, of course, but then we have other problems.

Member Author

jmagman Apr 15, 2024

I removed it and added a comment to -invalidate in the header.

jmagman force-pushed the vsync_waiter_ios-arc branch from 083937b to 08f4b22 Compare

April 15, 2024 19:28

stuartmorgan-g approved these changes

View reviewed changes

Contributor

stuartmorgan-g left a comment

LGTM

hellohuanlin approved these changes

View reviewed changes

jmagman added 2 commits

April 15, 2024 19:47


          Migrate vsync_waiter_ios to ARC

02ffec5


          Review edits

7fcab77

jmagman force-pushed the vsync_waiter_ios-arc branch from 08f4b22 to 7fcab77 Compare

April 16, 2024 04:04

jmagman added the autosubmit label

auto-submit bot merged commit ae00c28 into flutter:main

jmagman deleted the vsync_waiter_ios-arc branch

April 16, 2024 06:55

engine-flutter-autoroll mentioned this pull request

Roll Flutter Engine from 66ad80217898 to 5487ce15dbf7 (4 revisions) flutter/flutter#146799

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          ae00c28 Migrate vsync_waiter_ios to ARC (flutter/engine#52104)

engine-flutter-autoroll mentioned this pull request

Roll Flutter Engine from 66ad80217898 to 5c7d519d3d9e (5 revisions) flutter/flutter#146807

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          ae00c28 Migrate vsync_waiter_ios to ARC (flutter/engine#52104)

12c8aaf

engine-flutter-autoroll mentioned this pull request

Roll Flutter Engine from 66ad80217898 to cba7678fcfd2 (6 revisions) flutter/flutter#146811

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          ae00c28 Migrate vsync_waiter_ios to ARC (flutter/engine#52104)

a6b5e13

auto-submit bot pushed a commit to flutter/flutter that referenced this pull request


          Roll Flutter Engine from 66ad802 to cba7678 (6 revisions) (#146811)

9aea02b

flutter/engine@66ad802...cba7678

2024-04-16 skia-flutter-autoroll@skia.org Roll Dart SDK from 7d02f18b8a84 to cb0c34389d90 (1 revision) (flutter/engine#52151)
2024-04-16 skia-flutter-autoroll@skia.org Roll Skia from 50ac1117f159 to 8013b7f1dcd3 (5 revisions) (flutter/engine#52149)
2024-04-16 49699333+dependabot[bot]@users.noreply.github.com Bump google/osv-scanner-action from 1.6.2.pre.beta1 to 1.7.1 (flutter/engine#52147)
2024-04-16 magder@google.com Migrate vsync_waiter_ios to ARC (flutter/engine#52104)
2024-04-16 skia-flutter-autoroll@skia.org Roll Skia from b159229f2174 to 50ac1117f159 (1 revision) (flutter/engine#52146)
2024-04-16 skia-flutter-autoroll@skia.org Roll Dart SDK from 5f03197b9992 to 7d02f18b8a84 (2 revisions) (flutter/engine#52145)

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 jsimmons@google.com,rmistry@google.com,zra@google.com 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

gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request


          Roll Flutter Engine from 66ad802 to cba7678 (6 revisions) (flutter#14…

eac2d18

…6811)

flutter/engine@66ad802...cba7678

2024-04-16 skia-flutter-autoroll@skia.org Roll Dart SDK from 7d02f18b8a84 to cb0c34389d90 (1 revision) (flutter/engine#52151)
2024-04-16 skia-flutter-autoroll@skia.org Roll Skia from 50ac1117f159 to 8013b7f1dcd3 (5 revisions) (flutter/engine#52149)
2024-04-16 49699333+dependabot[bot]@users.noreply.github.com Bump google/osv-scanner-action from 1.6.2.pre.beta1 to 1.7.1 (flutter/engine#52147)
2024-04-16 magder@google.com Migrate vsync_waiter_ios to ARC (flutter/engine#52104)
2024-04-16 skia-flutter-autoroll@skia.org Roll Skia from b159229f2174 to 50ac1117f159 (1 revision) (flutter/engine#52146)
2024-04-16 skia-flutter-autoroll@skia.org Roll Dart SDK from 5f03197b9992 to 7d02f18b8a84 (2 revisions) (flutter/engine#52145)

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 jsimmons@google.com,rmistry@google.com,zra@google.com 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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit platform-ios