[ios_platform_view] only recycle maskView when the view is applying mutators #41573

cyanglaz · 2023-04-28T05:13:04Z

A mistake was introduced in #39498 where the maskViews are already recycles each frame.

Sometimes a PlatformView does not need to be re-composite: (https://github.com/flutter/engine/blob/main/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm#L398-L401), so the mask view for such PlatformView should not be recycled.

This PR changed the recycleMaskViews API to allow individual maskviews to be recycled. ApplyMutator then only recycle the maskView for that particular PlatformView.

The MaskViewPool is also reworked to be simpler.

The pool now contains a single set of mask views, there is no index counter needed.
When a maskView is needed, try to get it from the pool.
- If pool is empty, create a new view.
- If pool has an available maskview, remove it from the pool.
When a PlatformView starts to applyMutator, it removes current the maskView, insert the maskView to the pool.
When the above PlatformView needs to a maskView, it grabs one from the pool.

fixes: flutter/flutter#125620

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.

auto-submit · 2023-04-28T18:54:55Z

auto label is removed for flutter/engine, pr: 41573, due to - The status or check suite Linux mac_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2023-05-01T18:14:01Z

auto label is removed for flutter/engine, pr: 41573, due to - The status or check suite Linux mac_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

jmagman · 2023-05-02T19:57:32Z

Still some arm failures:
https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8782280206955702993/+/u/test:_Scenario_App_Integration_Tests/stdout

cyanglaz · 2023-05-02T20:06:15Z

Still some arm failures: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8782280206955702993/+/u/test:_Scenario_App_Integration_Tests/stdout

Yes, I discovered some other bugs in the maskViewPool, which was caused because I had an assumption that all maskView can be recycled every frame. This isn't true because sometimes, a PlatformView doesn't need to be re-composited, thus the ApplyMutator function is not called for those views.

I'm working on a fix and should have it ready by EOD today.

cyanglaz · 2023-05-02T21:44:30Z

I've seen some failures locally but shouldn't be related to this PR. (They didn't fell in this PR in previous ci runs).
Filed: flutter/flutter#125913

jmagman · 2023-05-02T21:55:07Z

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

-    [maskView reset];
-  }
-  self.availableIndex++;
+  maskView = [self.pool anyObject];


How do you know if this view is available for reuse?

When the view is in use, it is removed from the pool:
See line 493 below.

cyanglaz · 2023-05-03T16:27:10Z

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

-  self.availableIndex = 0;
+- (void)insertViewToPool:(FlutterClippingMaskView*)maskView {
+  FML_DCHECK(![self.pool containsObject:maskView]);
+  [self.pool addObject:maskView];


The user of the maskView decides to insert the view back to the pool when they are done with the maskView

Note: need to add capacity check here.

cyanglaz · 2023-05-03T18:41:39Z

Updated PR description to reflect the new approach.

cyanglaz · 2023-05-09T22:48:31Z

Note to self:
Add a comment in CompositeWithParams and ApplyMutator to explain these methods are not called for all the views every frame.
Add a comment to explain the views in the pool are the ones that are not in use.

chinmaygarde · 2023-05-11T20:10:11Z

This seems WIP till @cyanglaz is back.

remove format revert format change fix typo draft rework recycle pool fix comments and documents

hellohuanlin · 2023-05-15T23:08:03Z

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

  //
  // The `bounding_rect` is the final bounding rect of the PlatformView
  // (EmbeddedViewParams::finalBoundingRect). If a clip mutator's rect contains the final bounding
  // rect of the PlatformView, the clip mutator is not applied for performance optimization.
+  //
+  // This method is only called when thew `embedded_view` needs to be re-composited at the current


hellohuanlin · 2023-05-15T23:10:50Z

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


 @end

 @implementation FlutterClippingMaskViewPool : NSObject

 - (instancetype)initWithCapacity:(NSInteger)capacity {
  if (self = [super init]) {
-    _pool = [[NSMutableArray alloc] initWithCapacity:capacity];
+    _pool = [[NSMutableSet alloc] init];


nit: initWithCapacity to save some hash extending operations?

I have considered it and decided go against it.
For most of the pages, there are only 1 static PlatformViews. And even for scrolling situation, it is rear that many PlatformViews need to be clipped at the same time. So the set would rarely hit the capacity.

Or maybe change it to initWithCapacity:1during initialization? I think in most cases, the PlatformView would have a clip.

hellohuanlin · 2023-05-15T23:13:13Z

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

  return maskView;
 }

- (void)recycleMaskViews {
-  self.availableIndex = 0;
+- (void)insertViewToPool:(FlutterClippingMaskView*)maskView {


nit: insertViewToPoolIfNeeded?

hellohuanlin · 2023-05-15T23:13:39Z

testing/scenario_app/lib/src/platform_view.dart

@@ -1473,7 +1651,8 @@ void addPlatformView(
  const int valueString = 7;
  const int valueUint8List = 8;
  const int valueMap = 13;
-
+  print(_createdPlatformViews);
+  print(platformViewKey);


hellohuanlin · 2023-05-15T23:20:28Z

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

+  //
+  // Note that `views_to_recomposite_` does not represent all the views in the view hierarchy,
+  // if a PlatformView does not change its composition parameter from last frame, it is not
+  // included in the `views_to_recomposite_`.


maybe we should put this in implementation file instead, since we rarely look at the header file. A large part of them are more helpful to implementors and not callers.

Good point.

…applying mutators (flutter/engine#41573)

…126934) flutter/engine@fe24767...5cf141f 2023-05-16 godofredoc@google.com Add linux_clang_tidy builder. (flutter/engine#41990) 2023-05-16 ychris@google.com [ios_platform_view] only recycle maskView when the view is applying mutators (flutter/engine#41573) 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 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…plying mutators (#41573)" This reverts commit 17d5100.

…plying mutators" (#42080) Reverts #41573 Crashing on Framework CI

…applying mutators (flutter/engine#41573)

…sions) (#126961) Manual roll requested by zra@google.com flutter/engine@fe24767...1c775e3 2023-05-16 zanderso@users.noreply.github.com Revert "[ios_platform_view] only recycle maskView when the view is applying mutators" (flutter/engine#42080) 2023-05-16 gspencergoog@users.noreply.github.com [macOS] Wait for binding to be ready before requesting exits from framework (flutter/engine#41753) 2023-05-16 gspencergoog@users.noreply.github.com [linux] Wait for binding to be ready before requesting exits from framework (flutter/engine#41782) 2023-05-16 jacksongardner@google.com Initial support for images in Skwasm (flutter/engine#42019) 2023-05-16 jacksongardner@google.com Use new `unresolvedCodePoints` API from skia. (flutter/engine#41991) 2023-05-16 jason-simmons@users.noreply.github.com Convert public API NativeFieldWrapper classes to abstract interfaces (flutter/engine#41945) 2023-05-16 737941+loic-sharma@users.noreply.github.com [Windows] Add force redraw to the C++ client wrapper (flutter/engine#42061) 2023-05-16 godofredoc@google.com Fix drone_dimension host_engine_builder. (flutter/engine#42068) 2023-05-16 godofredoc@google.com Add linux_clang_tidy builder. (flutter/engine#41990) 2023-05-16 ychris@google.com [ios_platform_view] only recycle maskView when the view is applying mutators (flutter/engine#41573) 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 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…plying mutators #41573" (#42115) The original PR (#41573) was reverted due to flutter/flutter#126951 The issue will be fixed in #42079 This needs to be landed after #42079 fixes: flutter/flutter#125620 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…ew is applying mutators #41573" (#42115)" This reverts commit 266524c.

…plying mutators #41573" (#42231) Reverts #42115 Failing on the framework tree as in https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20platform_views_scroll_perf_ios__timeline_summary/11103/overview

…lutter#126934) flutter/engine@fe24767...5cf141f 2023-05-16 godofredoc@google.com Add linux_clang_tidy builder. (flutter/engine#41990) 2023-05-16 ychris@google.com [ios_platform_view] only recycle maskView when the view is applying mutators (flutter/engine#41573) 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 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…sions) (flutter#126961) Manual roll requested by zra@google.com flutter/engine@fe24767...1c775e3 2023-05-16 zanderso@users.noreply.github.com Revert "[ios_platform_view] only recycle maskView when the view is applying mutators" (flutter/engine#42080) 2023-05-16 gspencergoog@users.noreply.github.com [macOS] Wait for binding to be ready before requesting exits from framework (flutter/engine#41753) 2023-05-16 gspencergoog@users.noreply.github.com [linux] Wait for binding to be ready before requesting exits from framework (flutter/engine#41782) 2023-05-16 jacksongardner@google.com Initial support for images in Skwasm (flutter/engine#42019) 2023-05-16 jacksongardner@google.com Use new `unresolvedCodePoints` API from skia. (flutter/engine#41991) 2023-05-16 jason-simmons@users.noreply.github.com Convert public API NativeFieldWrapper classes to abstract interfaces (flutter/engine#41945) 2023-05-16 737941+loic-sharma@users.noreply.github.com [Windows] Add force redraw to the C++ client wrapper (flutter/engine#42061) 2023-05-16 godofredoc@google.com Fix drone_dimension host_engine_builder. (flutter/engine#42068) 2023-05-16 godofredoc@google.com Add linux_clang_tidy builder. (flutter/engine#41990) 2023-05-16 ychris@google.com [ios_platform_view] only recycle maskView when the view is applying mutators (flutter/engine#41573) 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 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…ew is applying mutators flutter#41573" (flutter#42231)" This reverts commit 2586cbe.

…plying mutators #42115" (#42823) Relands #42115, which was reverted in #42231 due to a crash in the framework test. The crash is due to a memory management issue that I fixed it in this PR, I also added a scenario test to catch the crash. fixes: flutter/flutter#125620 See also: orignal PR #41573 for more details. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…plying mutators flutter#42115" (flutter#42823) Relands flutter#42115, which was reverted in flutter#42231 due to a crash in the framework test. The crash is due to a memory management issue that I fixed it in this PR, I also added a scenario test to catch the crash. fixes: flutter/flutter#125620 See also: orignal PR flutter#41573 for more details. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

flutter-dashboard bot added the platform-ios label Apr 28, 2023

cyanglaz requested a review from jmagman April 28, 2023 05:13

cyanglaz marked this pull request as ready for review April 28, 2023 05:13

cyanglaz force-pushed the maskviewpool_regression branch from bac5f99 to f384936 Compare April 28, 2023 05:19

jmagman approved these changes Apr 28, 2023

View reviewed changes

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

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 28, 2023

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

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 1, 2023

cyanglaz force-pushed the maskviewpool_regression branch from a6ad852 to 9d5d996 Compare May 2, 2023 21:11

cyanglaz requested a review from jmagman May 2, 2023 21:44

jmagman reviewed May 2, 2023

View reviewed changes

jmagman requested a review from hellohuanlin May 2, 2023 21:55

cyanglaz commented May 3, 2023

View reviewed changes

cyanglaz changed the title ~~[ios_platform_view] recycle maskView at the beginning of the frame~~ [ios_platform_view] only recycle maskView when the view is applying mutators May 3, 2023

cyanglaz requested a review from jmagman May 3, 2023 18:41

chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label May 11, 2023

cyanglaz marked this pull request as draft May 11, 2023 21:07

recycle maskView at the begining of the frame

89f258f

remove format revert format change fix typo draft rework recycle pool fix comments and documents

cyanglaz marked this pull request as ready for review May 15, 2023 22:29

fix

27dbd8c

cyanglaz force-pushed the maskviewpool_regression branch from a1c98a6 to 27dbd8c Compare May 15, 2023 22:30

hellohuanlin approved these changes May 15, 2023

View reviewed changes

revuew

1218b75

cyanglaz added autosubmit Merge PR when tree becomes green via auto submit App and removed Work in progress (WIP) Not ready (yet) for review! labels May 16, 2023

auto-submit bot merged commit 17d5100 into flutter:main May 16, 2023

engine-flutter-autoroll mentioned this pull request May 16, 2023

Roll Flutter Engine from fe2476743b59 to 17d5100b0a39 (1 revision) flutter/flutter#126932

Closed

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

17d5100b0 [ios_platform_view] only recycle maskView when the view is …

05e687c

…applying mutators (flutter/engine#41573)

engine-flutter-autoroll mentioned this pull request May 16, 2023

Roll Flutter Engine from fe2476743b59 to 5cf141f7c03c (2 revisions) flutter/flutter#126934

Merged

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

17d5100b0 [ios_platform_view] only recycle maskView when the view is …

765ecc3

…applying mutators (flutter/engine#41573)

dkwingsmt mentioned this pull request May 16, 2023

FlutterPlatformViews might trigger false failure with its new pooling algorithm flutter/flutter#126951

Closed

zanderso added a commit that referenced this pull request May 16, 2023

Revert "[ios_platform_view] only recycle maskView when the view is ap…

0aedecb

…plying mutators (#41573)" This reverts commit 17d5100.

zanderso mentioned this pull request May 16, 2023

Revert "[ios_platform_view] only recycle maskView when the view is applying mutators" #42080

Merged

zanderso added a commit that referenced this pull request May 16, 2023

Revert "[ios_platform_view] only recycle maskView when the view is ap…

1c775e3

…plying mutators" (#42080) Reverts #41573 Crashing on Framework CI

engine-flutter-autoroll mentioned this pull request May 16, 2023

Manual roll Flutter Engine from fe2476743b59 to 1c775e34e2d5 (10 revisions) flutter/flutter#126961

Merged

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

17d5100b0 [ios_platform_view] only recycle maskView when the view is …

bcb15a7

…applying mutators (flutter/engine#41573)

cyanglaz deleted the maskviewpool_regression branch May 17, 2023 18:41

cyanglaz mentioned this pull request May 18, 2023

Reland "[ios_platform_view] only recycle maskView when the view is applying mutators #41573" #42115

Merged

8 tasks

zanderso added a commit that referenced this pull request May 23, 2023

Revert "Reland "[ios_platform_view] only recycle maskView when the vi…

58d02e1

…ew is applying mutators #41573" (#42115)" This reverts commit 266524c.

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Jun 13, 2023

Revert "Revert "[ios_platform_view] only recycle maskView when the vi…

b4736ea

…ew is applying mutators flutter#41573" (flutter#42231)" This reverts commit 2586cbe.

cyanglaz mentioned this pull request Jun 13, 2023

Reland "[ios_platform_view] only recycle maskView when the view is applying mutators #42115" #42823

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios_platform_view] only recycle maskView when the view is applying mutators #41573

[ios_platform_view] only recycle maskView when the view is applying mutators #41573

cyanglaz commented Apr 28, 2023 •

edited

Loading

auto-submit bot commented Apr 28, 2023

auto-submit bot commented May 1, 2023

jmagman commented May 2, 2023

cyanglaz commented May 2, 2023

cyanglaz commented May 2, 2023

jmagman May 2, 2023

cyanglaz May 3, 2023

cyanglaz May 3, 2023 •

edited

Loading

cyanglaz May 9, 2023

cyanglaz commented May 3, 2023

cyanglaz commented May 9, 2023

chinmaygarde commented May 11, 2023

hellohuanlin May 15, 2023

hellohuanlin May 15, 2023

cyanglaz May 16, 2023

hellohuanlin May 15, 2023

hellohuanlin May 15, 2023

hellohuanlin May 15, 2023

cyanglaz May 16, 2023

[ios_platform_view] only recycle maskView when the view is applying mutators #41573

[ios_platform_view] only recycle maskView when the view is applying mutators #41573

Conversation

cyanglaz commented Apr 28, 2023 • edited Loading

Pre-launch Checklist

auto-submit bot commented Apr 28, 2023

auto-submit bot commented May 1, 2023

jmagman commented May 2, 2023

cyanglaz commented May 2, 2023

cyanglaz commented May 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz May 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz commented May 3, 2023

cyanglaz commented May 9, 2023

chinmaygarde commented May 11, 2023

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

cyanglaz commented Apr 28, 2023 •

edited

Loading

cyanglaz May 3, 2023 •

edited

Loading