[iOS] Flush layer pool after platform view dispose #54056

cbracken · 2024-07-23T19:40:33Z

FlutterPlatformViewLayerPool is a pool of iOS layers used for rendering platform views on iOS. When layers are no longer needed, we currently mark them available for re-use but we never actually flush them and thus recover the memory associated with these layers once we know that the layers are no longer needed.

We now flush the layer pool on SubmitFrame. If a previously-used layer is no longer used in the current frame, it is removed. In theory, this could cause a performance regression in the case where an additional layer is needed every second or third frame, but we should keep it simple on the first pass and only complicate things later if warranted.

Fixes: flutter/flutter#152053

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.

`FlutterPlatformViewLayerPool` is a pool of iOS layers used for rendering platform views on iOS. When layers are no longer needed, we currently mark them available for re-use but we never actually flush them and thus recover the memory associated with these layers once we know that the platformview is no longer used. We now flush the layer pool to recover unused memory after platform views have been disposed. See: flutter/flutter#152053

cbracken · 2024-07-23T19:59:25Z

Taking a look at how to sneak a test in here.

jonahwilliams

Changes look reasonable to me, modulo a test.

jason-simmons · 2024-07-23T20:20:33Z

LGTM

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

cbracken · 2024-07-23T23:06:39Z

Added a test.

jonahwilliams

LGTM

jmagman · 2024-07-23T23:26:10Z

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

+  auto pool = flutter::FlutterPlatformViewLayerPool{};
+
+  // Add layers to the pool.
+  auto layer1 = pool.GetLayer(gr_context.get(), ios_context, MTLPixelFormatBGRA8Unorm);


Is there a way to check this is actually released? Or is that not a concern? I know the __weak/@autoreleasepool pattern won't work with non-Obj-C objects.

The pool holds these in std::shared_ptr<FlutterPlatformViewLayer> so they'll be cleaned up via RAII. If we templatized FlutterPlatformViewLayerPool we could stick in some other class and manipulate a count in the ctor/dtor that we could then test against, but that probably wouldn't give us much more benefit, since it still wouldn't show that the real FlutterPlatformView layer properly releases its memory -- it uses C++ smart pointers for all but the GrDirectContext so I trust it, but it's still not tested.

An integration test that looks at overall memory is probably the way to do something like that.

Since this method isn't mutating the pool, we should mark it const. Cleanup spotted while working on #54056. No test because no semantic change. The compiler is the test. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…/engine#54056)

…152203) flutter/engine@f5ec4ab...1572635 2024-07-24 [email protected] Change JSArray.length to return int (flutter/engine#54051) 2024-07-24 [email protected] Roll Dart SDK from 17131486a2f7 to e53beb039093 (1 revision) (flutter/engine#54066) 2024-07-24 [email protected] Roll Fuchsia Linux SDK from ZcBsXDojTYbriHD7_... to qA7S-DZ5FyMtcM7_J... (flutter/engine#54064) 2024-07-24 [email protected] [DisplayList] Fix assertions on DisplayList verbose comparison tests (flutter/engine#54065) 2024-07-24 [email protected] Roll Fuchsia Test Scripts from 5bzzKaW7fCp_No_w_... to clqtZA8cx4GEXwcOe... (flutter/engine#54063) 2024-07-23 [email protected] [iOS] Flush layer pool after platform view dispose (flutter/engine#54056) 2024-07-23 [email protected] [iOS] Mark EmbeddedViewCount const (flutter/engine#54062) 2024-07-23 [email protected] Roll Skia from f4355cf73508 to 3f1b4e98f65a (1 revision) (flutter/engine#54060) 2024-07-23 [email protected] Roll Skia from 2d518b6a793a to f4355cf73508 (9 revisions) (flutter/engine#54058) 2024-07-23 [email protected] Roll Dart SDK from eeb2e4e409bf to 17131486a2f7 (1 revision) (flutter/engine#54055) 2024-07-23 [email protected] Roll Skia from a9019fddac28 to 2d518b6a793a (4 revisions) (flutter/engine#54052) 2024-07-23 [email protected] [Impeller] add emulated advanced blend support for exp canvas. (flutter/engine#54020) 2024-07-23 [email protected] Roll Skia from 1cda2a7b0ee4 to a9019fddac28 (3 revisions) (flutter/engine#54050) 2024-07-23 [email protected] [DisplayList] track unbounded state on save layers and DisplayLists (flutter/engine#54032) 2024-07-23 [email protected] Temporarily disable use of glBlitFramebuffer on NVIDIA (flutter/engine#54040) 2024-07-23 [email protected] Set the view ID for FlView (flutter/engine#54043) 2024-07-23 [email protected] [Impeller] Implement OpenGL to Vulkan texture trampolining. (flutter/engine#53966) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from ZcBsXDojTYbr to qA7S-DZ5FyMt 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],[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

jonahwilliams · 2024-07-24T20:22:47Z

This led to a nice memory drop on a varierty of benchmarks:

https://flutter-flutter-perf.skia.org/e/?keys=Xe332d40e74211cccfea289ea6902dbd3&selected=commit%3D41690%26name%3D%252Carch%253Dintel%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253DiPhone_11%252Cdevice_version%253Dnone%252Chost_type%253Dmac%252Csub_result%253Dworst_gpu_memory_mb%252Ctest%253Dplatform_views_scroll_perf_ios__timeline_summary%252C&xbaroffset=41690

jonahwilliams · 2024-07-24T20:24:10Z

With a slight increase in some worst fame times (probably from re-creating the overlays):

https://flutter-flutter-perf.skia.org/e/?begin=1721766214&end=1721852614&keys=X92aaab43f15c18b8d4075492ea7c070d&selected=commit%3D41690%26name%3D%252Carch%253Dintel%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253DiPhone_11%252Cdevice_version%253Dnone%252Chost_type%253Dmac%252Csub_result%253D90th_percentile_frame_rasterizer_time_millis%252Ctest%253Dplatform_views_scroll_perf_ios__timeline_summary%252C&xbaroffset=41690

cbracken · 2024-07-24T20:27:48Z

That seems reasonable. We could potentially tweak the pool clearing code to do some kind of back-off, but I worry that going down that path might lead to optimising a bit too much for a particular scenario.

…lutter#152203) flutter/engine@f5ec4ab...1572635 2024-07-24 [email protected] Change JSArray.length to return int (flutter/engine#54051) 2024-07-24 [email protected] Roll Dart SDK from 17131486a2f7 to e53beb039093 (1 revision) (flutter/engine#54066) 2024-07-24 [email protected] Roll Fuchsia Linux SDK from ZcBsXDojTYbriHD7_... to qA7S-DZ5FyMtcM7_J... (flutter/engine#54064) 2024-07-24 [email protected] [DisplayList] Fix assertions on DisplayList verbose comparison tests (flutter/engine#54065) 2024-07-24 [email protected] Roll Fuchsia Test Scripts from 5bzzKaW7fCp_No_w_... to clqtZA8cx4GEXwcOe... (flutter/engine#54063) 2024-07-23 [email protected] [iOS] Flush layer pool after platform view dispose (flutter/engine#54056) 2024-07-23 [email protected] [iOS] Mark EmbeddedViewCount const (flutter/engine#54062) 2024-07-23 [email protected] Roll Skia from f4355cf73508 to 3f1b4e98f65a (1 revision) (flutter/engine#54060) 2024-07-23 [email protected] Roll Skia from 2d518b6a793a to f4355cf73508 (9 revisions) (flutter/engine#54058) 2024-07-23 [email protected] Roll Dart SDK from eeb2e4e409bf to 17131486a2f7 (1 revision) (flutter/engine#54055) 2024-07-23 [email protected] Roll Skia from a9019fddac28 to 2d518b6a793a (4 revisions) (flutter/engine#54052) 2024-07-23 [email protected] [Impeller] add emulated advanced blend support for exp canvas. (flutter/engine#54020) 2024-07-23 [email protected] Roll Skia from 1cda2a7b0ee4 to a9019fddac28 (3 revisions) (flutter/engine#54050) 2024-07-23 [email protected] [DisplayList] track unbounded state on save layers and DisplayLists (flutter/engine#54032) 2024-07-23 [email protected] Temporarily disable use of glBlitFramebuffer on NVIDIA (flutter/engine#54040) 2024-07-23 [email protected] Set the view ID for FlView (flutter/engine#54043) 2024-07-23 [email protected] [Impeller] Implement OpenGL to Vulkan texture trampolining. (flutter/engine#53966) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from ZcBsXDojTYbr to qA7S-DZ5FyMt 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],[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

github-actions bot added the platform-ios label Jul 23, 2024

cbracken mentioned this pull request Jul 23, 2024

[Memory leak][iOS] PlatformView with UIImageView & Scrolling get junk on showModalBottomSheet flutter/flutter#152053

Closed

cbracken requested review from hellohuanlin, jmagman and jonahwilliams July 23, 2024 19:59

jonahwilliams reviewed Jul 23, 2024

View reviewed changes

jmagman approved these changes Jul 23, 2024

View reviewed changes

hellohuanlin reviewed Jul 23, 2024

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm Show resolved Hide resolved

cbracken marked this pull request as ready for review July 23, 2024 23:06

cbracken added 2 commits July 23, 2024 16:08

Add LayerPoolSize

172d100

Add test

b5ab12e

jonahwilliams approved these changes Jul 23, 2024

View reviewed changes

cbracken mentioned this pull request Jul 23, 2024

[iOS] Mark EmbeddedViewCount const #54062

Merged

8 tasks

jmagman approved these changes Jul 23, 2024

View reviewed changes

cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 23, 2024

auto-submit bot merged commit e1952a3 into flutter:main Jul 23, 2024

cbracken deleted the flush-unused-layers branch July 23, 2024 23:55

engine-flutter-autoroll mentioned this pull request Jul 24, 2024

Roll Flutter Engine from f5ec4ab0bf94 to 1572635432c0 (17 revisions) flutter/flutter#152203

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 24, 2024

e1952a388 [iOS] Flush layer pool after platform view dispose (flutter…

1476b82

…/engine#54056)

jmagman mentioned this pull request Jul 24, 2024

Regression in raster time in platform_views_scroll_perf_ad_banners__timeline_summary flutter/flutter#152266

Closed

cbracken mentioned this pull request Aug 6, 2024

[google_maps_flutter] [iOS] Memory limit exceeded flutter/flutter#150823

Open

jmagman mentioned this pull request Aug 28, 2024

Memory leak with IndexedStack containing a UiKitView flutter/flutter#148639

Open

cbracken mentioned this pull request Nov 20, 2024

Page with UIKitView has memory leak after popup flutter/flutter#157700

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[iOS] Flush layer pool after platform view dispose #54056

[iOS] Flush layer pool after platform view dispose #54056

Uh oh!

cbracken commented Jul 23, 2024 •

edited

Loading

Uh oh!

cbracken commented Jul 23, 2024

Uh oh!

jonahwilliams left a comment

Uh oh!

jason-simmons commented Jul 23, 2024

Uh oh!

Uh oh!

cbracken commented Jul 23, 2024

Uh oh!

jonahwilliams left a comment

Uh oh!

jmagman Jul 23, 2024

Uh oh!

cbracken Jul 23, 2024

Uh oh!

jonahwilliams commented Jul 24, 2024

Uh oh!

jonahwilliams commented Jul 24, 2024

Uh oh!

cbracken commented Jul 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[iOS] Flush layer pool after platform view dispose #54056

[iOS] Flush layer pool after platform view dispose #54056

Uh oh!

Conversation

cbracken commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

cbracken commented Jul 23, 2024

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

jason-simmons commented Jul 23, 2024

Uh oh!

Uh oh!

cbracken commented Jul 23, 2024

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

cbracken Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Jul 24, 2024

Uh oh!

jonahwilliams commented Jul 24, 2024

Uh oh!

cbracken commented Jul 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cbracken commented Jul 23, 2024 •

edited

Loading