[ios_platform_view] MaskView pool to reuse maskViews. #38989

cyanglaz · 2023-01-18T23:00:42Z

The maskViews are constructed and destructed every frame for every PlatformView on the screen. It slows down the performance quite a lot.
This PR adds a pool that caches maximum of 5 maskViews. These maskViews are reused every frame.
If there are more than 5 maskViews required in a frame, extra maskViews are constructed and destructed each frame. (This scenario is very unlikely in real life application)

Part of flutter/flutter#116640

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 Hixie said 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.

reuse mask view format format draft draft fixes test

jmagman · 2023-01-18T23:55:05Z

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

+@interface FlutterClippingMaskViewPool ()
+
+@property(assign, nonatomic) NSUInteger capacity;
+@property(retain, nonatomic) NSMutableArray<FlutterClippingMaskView*>* pool;


Would this retain the FlutterClippingMaskView even after the ChildClippingView is dealloc'd?
Would NSPointerArray with NSPointerFunctionsWeakMemory (I think zeroes it out with MRC but worth testing) work better here? If you could reliably know the mask views in the pool are used (because they are retained by a ChildClippingView), then you could get rid of recycleMaskViews and during insert compact, then insert at count if there was capacity.

Well never mind, retaining it after the ChildClippingView is dealloc'd is the whole point of this PR...
Can you confirm that deallocating FlutterPlatformViewsController releases the pool and the mask views?

Will add a test for it!

jmagman · 2023-01-19T18:42:27Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h

@@ -328,6 +353,7 @@ class FlutterPlatformViewsController {
  fml::scoped_nsobject<FlutterMethodChannel> channel_;
  fml::scoped_nsobject<UIView> flutter_view_;
  fml::scoped_nsobject<UIViewController> flutter_view_controller_;
+  fml::scoped_nsobject<FlutterClippingMaskViewPool> mask_view_pool_;


Is there a reason to make this a scoped_nsobject?

No, I should change it to properties. I was blindly following the current pattern.

Oh, actually, it is in a c++ class. So I think scoped_nsobject is better that the lifecycle is automatically managed.

Which class is c++? Your new FlutterClippingMaskViewPool is a Obj-C class.

FlutterPlatformViewsController is the c++ class.

jmagman · 2023-01-19T18:44:17Z

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

+@interface FlutterClippingMaskViewPool ()
+
+@property(assign, nonatomic) NSUInteger capacity;
+@property(retain, nonatomic) NSMutableArray<FlutterClippingMaskView*>* pool;


Well never mind, retaining it after the ChildClippingView is dealloc'd is the whole point of this PR...
Can you confirm that deallocating FlutterPlatformViewsController releases the pool and the mask views?

hellohuanlin

Just a question on the pool capacity.

hellohuanlin · 2023-01-19T19:07:23Z

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

+- (FlutterClippingMaskView*)getMaskViewWithFrame:(CGRect)frame {
+  FML_DCHECK(self.availableIndex <= self.capacity);
+  FlutterClippingMaskView* maskView;
+  if (self.availableIndex == self.capacity) {


it looks like the capacity is not updated? should it be increased when you add a new one?

we can probably just compare with pool.count, and do not keep this capacity ivar?

The capacity is a static limit set for the pool. My intention is to limit the memory used by the pool. For example, (it is probably not possible in real life), I don't want the pool to cache 20 views for the whole application session.

gotcha. I mistakenly thought that this capacity increases like an NSMutableArray.

Ah, yeah it could be confusing, I added a comment.

hellohuanlin · 2023-01-19T19:11:02Z

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

@@ -451,6 +453,17 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
  return clipCount;
 }

+void FlutterPlatformViewsController::ClipViewAddMaskView(UIView* clipView) {


supernit: ClipViewSetMaskView?

…ter/engine#38989)

…118938) * 2ee82578c [ios_platform_view] MaskView pool to reuse maskViews. (flutter/engine#38989) * c137b3b33 Roll Fuchsia Mac SDK from GvtVLigysBcywNN9T... to ZTKDeVL1HDAwsZdhl... (flutter/engine#39044) * 3a444b366 Roll Dart SDK from 807077cc5d1b to 8c2eb20b5376 (2 revisions) (flutter/engine#39047)

* reuse maskView reuse mask view format format draft draft fixes test * comments * fix comment * test for releasing maskView * updates * fix

This reverts commit 2ee8257.

…" (#39490) This reverts commit 2ee8257.

…#38989)" (#39490)" This reverts commit 6a2de77.

…#38989)" (#39490)" (#39498) This reverts commit 6a2de77.

…skViews. (#38989)" (#39490)" (#39498)" This reverts commit 4270d74.

…skViews. (flutter#38989)" (flutter#39490)" (flutter#39498)" This reverts commit 4270d74.

* Revert "Revert "Revert "[ios_platform_view] MaskView pool to reuse maskViews. (#38989)" (#39490)" (#39498)" This reverts commit 4270d74. * fix build error

…er#39608) * Revert "Revert "Revert "[ios_platform_view] MaskView pool to reuse maskViews. (flutter#38989)" (flutter#39490)" (flutter#39498)" This reverts commit 4270d74. * fix build error

cp #42823. The PR fixes flutter/flutter#125620, which was a regression caused by #38989. We cannot simply revert #38989 because #38989 addressed a critical performance issue [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

flutter-dashboard bot added the platform-ios label Jan 18, 2023

cyanglaz force-pushed the platform_view_reuse_mask_view branch from d2e8e43 to d9aa181 Compare January 18, 2023 23:01

reuse maskView

ef09d5b

reuse mask view format format draft draft fixes test

cyanglaz force-pushed the platform_view_reuse_mask_view branch from d9aa181 to ef09d5b Compare January 18, 2023 23:03

comments

afa4f42

cyanglaz requested review from hellohuanlin and jmagman January 18, 2023 23:17

jmagman reviewed Jan 18, 2023

View reviewed changes

fix comment

72b8fa9

jmagman reviewed Jan 19, 2023

View reviewed changes

hellohuanlin reviewed Jan 19, 2023

View reviewed changes

test for releasing maskView

f9b8ad7

hellohuanlin approved these changes Jan 20, 2023

View reviewed changes

Chris Yang added 2 commits January 19, 2023 17:45

updates

473bb50

fix

843f952

jmagman approved these changes Jan 20, 2023

View reviewed changes

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 21, 2023

auto-submit bot merged commit 2ee8257 into flutter:main Jan 21, 2023

engine-flutter-autoroll mentioned this pull request Jan 21, 2023

Roll Flutter Engine from 7b68d71b8d03 to 2ee82578c085 (1 revision) flutter/flutter#118934

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 21, 2023

2ee82578c [ios_platform_view] MaskView pool to reuse maskViews. (flut…

4cfc580

…ter/engine#38989)

engine-flutter-autoroll mentioned this pull request Jan 21, 2023

Roll Flutter Engine from 7b68d71b8d03 to c137b3b339e6 (2 revisions) flutter/flutter#118935

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 21, 2023

2ee82578c [ios_platform_view] MaskView pool to reuse maskViews. (flut…

3caefe9

…ter/engine#38989)

engine-flutter-autoroll mentioned this pull request Jan 21, 2023

Roll Flutter Engine from 7b68d71b8d03 to 3a444b36657c (3 revisions) flutter/flutter#118938

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 21, 2023

2ee82578c [ios_platform_view] MaskView pool to reuse maskViews. (flut…

dd6b0e4

…ter/engine#38989)

cyanglaz deleted the platform_view_reuse_mask_view branch January 24, 2023 19:08

zanderso mentioned this pull request Feb 7, 2023

Regression in average_gpu_usage in simple_animation_perf_ios flutter/flutter#120207

Closed

cyanglaz pushed a commit that referenced this pull request Feb 8, 2023

Revert "[ios_platform_view] MaskView pool to reuse maskViews. (#38989)"

8cc4492

This reverts commit 2ee8257.

cyanglaz mentioned this pull request Feb 8, 2023

Revert "[ios_platform_view] MaskView pool to reuse maskViews." #39490

Merged

auto-submit bot pushed a commit that referenced this pull request Feb 8, 2023

Revert "[ios_platform_view] MaskView pool to reuse maskViews. (#38989)…

6a2de77

…" (#39490) This reverts commit 2ee8257.

cyanglaz pushed a commit that referenced this pull request Feb 8, 2023

Revert "Revert "[ios_platform_view] MaskView pool to reuse maskViews. (…

c091e38

…#38989)" (#39490)" This reverts commit 6a2de77.

cyanglaz mentioned this pull request Feb 8, 2023

Reland "[ios_platform_view] MaskView pool to reuse maskViews." #39498

Merged

auto-submit bot pushed a commit that referenced this pull request Feb 8, 2023

Revert "Revert "[ios_platform_view] MaskView pool to reuse maskViews. (…

4270d74

…#38989)" (#39490)" (#39498) This reverts commit 6a2de77.

cyanglaz pushed a commit that referenced this pull request Feb 13, 2023

Revert "Revert "Revert "[ios_platform_view] MaskView pool to reuse ma…

7ece9bf

…skViews. (#38989)" (#39490)" (#39498)" This reverts commit 4270d74.

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Feb 14, 2023

Revert "Revert "Revert "[ios_platform_view] MaskView pool to reuse ma…

aba36dd

…skViews. (flutter#38989)" (flutter#39490)" (flutter#39498)" This reverts commit 4270d74.

jmagman mentioned this pull request Mar 20, 2023

[ios] reland "[ios_platform_view] MaskView pool to reuse maskViews. #38989" #39630

Merged

8 tasks

cyanglaz mentioned this pull request Mar 20, 2023

Reland "[ios_platform_view] MaskView pool to reuse maskViews" (#39608)" #40456

Merged

8 tasks

jmagman mentioned this pull request Apr 27, 2023

PlatformViews are drawn outside of parent widget (beta channel) flutter/flutter#125620

Closed

2 tasks

cyanglaz mentioned this pull request Jun 20, 2023

[cp] only recycle maskView when the view is applying mutators #43007

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios_platform_view] MaskView pool to reuse maskViews. #38989

[ios_platform_view] MaskView pool to reuse maskViews. #38989

cyanglaz commented Jan 18, 2023

jmagman Jan 18, 2023 •

edited

Loading

jmagman Jan 19, 2023

cyanglaz Jan 19, 2023

jmagman Jan 19, 2023

cyanglaz Jan 19, 2023

cyanglaz Jan 19, 2023

jmagman Jan 19, 2023

cyanglaz Jan 19, 2023

jmagman Jan 19, 2023

hellohuanlin left a comment

hellohuanlin Jan 19, 2023

hellohuanlin Jan 19, 2023

cyanglaz Jan 19, 2023 •

edited

Loading

hellohuanlin Jan 20, 2023

cyanglaz Jan 20, 2023

hellohuanlin Jan 19, 2023

cyanglaz Jan 20, 2023

[ios_platform_view] MaskView pool to reuse maskViews. #38989

[ios_platform_view] MaskView pool to reuse maskViews. #38989

Conversation

cyanglaz commented Jan 18, 2023

Pre-launch Checklist

jmagman Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellohuanlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman Jan 18, 2023 •

edited

Loading

cyanglaz Jan 19, 2023 •

edited

Loading