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

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

Merged
auto-submit[bot] merged 3 commits into
flutter-team-archive:mainfrom
cbracken:flush-unused-layers
Jul 23, 2024
Merged

[iOS] Flush layer pool after platform view dispose#54056
auto-submit[bot] merged 3 commits into
flutter-team-archive:mainfrom
cbracken:flush-unused-layers

Conversation

@cbracken

@cbracken cbracken commented Jul 23, 2024

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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

@jonahwilliams jonahwilliams left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look reasonable to me, modulo a test.

@jason-simmons

Copy link
Copy Markdown
Contributor

LGTM

Comment thread shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
@cbracken cbracken marked this pull request as ready for review July 23, 2024 23:06
@cbracken

Copy link
Copy Markdown
Contributor Author

Added a test.

@jonahwilliams jonahwilliams left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

auto pool = flutter::FlutterPlatformViewLayerPool{};

// Add layers to the pool.
auto layer1 = pool.GetLayer(gr_context.get(), ios_context, MTLPixelFormatBGRA8Unorm);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

auto-submit Bot pushed a commit that referenced this pull request Jul 23, 2024
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
@cbracken cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 23, 2024
@auto-submit auto-submit Bot merged commit e1952a3 into flutter-team-archive:main Jul 23, 2024
@cbracken cbracken deleted the flush-unused-layers branch July 23, 2024 23:55
auto-submit Bot pushed a commit to flutter/flutter that referenced this pull request Jul 24, 2024
flutter-team-archive/engine@f5ec4ab...1572635

2024-07-24 58529443+srujzs@users.noreply.github.com Change JSArray.length to return int (flutter-team-archive/engine#54051)
2024-07-24 skia-flutter-autoroll@skia.org Roll Dart SDK from 17131486a2f7 to e53beb039093 (1 revision) (flutter-team-archive/engine#54066)
2024-07-24 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from ZcBsXDojTYbriHD7_... to qA7S-DZ5FyMtcM7_J... (flutter-team-archive/engine#54064)
2024-07-24 flar@google.com [DisplayList] Fix assertions on DisplayList verbose comparison tests (flutter-team-archive/engine#54065)
2024-07-24 skia-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from 5bzzKaW7fCp_No_w_... to clqtZA8cx4GEXwcOe... (flutter-team-archive/engine#54063)
2024-07-23 chris@bracken.jp [iOS] Flush layer pool after platform view dispose (flutter-team-archive/engine#54056)
2024-07-23 chris@bracken.jp [iOS] Mark EmbeddedViewCount const (flutter-team-archive/engine#54062)
2024-07-23 skia-flutter-autoroll@skia.org Roll Skia from f4355cf73508 to 3f1b4e98f65a (1 revision) (flutter-team-archive/engine#54060)
2024-07-23 skia-flutter-autoroll@skia.org Roll Skia from 2d518b6a793a to f4355cf73508 (9 revisions) (flutter-team-archive/engine#54058)
2024-07-23 skia-flutter-autoroll@skia.org Roll Dart SDK from eeb2e4e409bf to 17131486a2f7 (1 revision) (flutter-team-archive/engine#54055)
2024-07-23 skia-flutter-autoroll@skia.org Roll Skia from a9019fddac28 to 2d518b6a793a (4 revisions) (flutter-team-archive/engine#54052)
2024-07-23 jonahwilliams@google.com [Impeller] add emulated advanced blend support for exp canvas. (flutter-team-archive/engine#54020)
2024-07-23 skia-flutter-autoroll@skia.org Roll Skia from 1cda2a7b0ee4 to a9019fddac28 (3 revisions) (flutter-team-archive/engine#54050)
2024-07-23 flar@google.com [DisplayList] track unbounded state on save layers and DisplayLists (flutter-team-archive/engine#54032)
2024-07-23 robert.ancell@canonical.com Temporarily disable use of glBlitFramebuffer on NVIDIA (flutter-team-archive/engine#54040)
2024-07-23 robert.ancell@canonical.com Set the view ID for FlView (flutter-team-archive/engine#54043)
2024-07-23 chinmaygarde@google.com [Impeller] Implement OpenGL to Vulkan texture trampolining. (flutter-team-archive/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 jonahwilliams@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
@cbracken

Copy link
Copy Markdown
Contributor Author

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.

TytaniumDev pushed a commit to TytaniumDev/flutter that referenced this pull request Aug 7, 2024
…52203)

flutter-team-archive/engine@f5ec4ab...1572635

2024-07-24 58529443+srujzs@users.noreply.github.com Change JSArray.length to return int (flutter-team-archive/engine#54051)
2024-07-24 skia-flutter-autoroll@skia.org Roll Dart SDK from 17131486a2f7 to e53beb039093 (1 revision) (flutter-team-archive/engine#54066)
2024-07-24 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from ZcBsXDojTYbriHD7_... to qA7S-DZ5FyMtcM7_J... (flutter-team-archive/engine#54064)
2024-07-24 flar@google.com [DisplayList] Fix assertions on DisplayList verbose comparison tests (flutter-team-archive/engine#54065)
2024-07-24 skia-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from 5bzzKaW7fCp_No_w_... to clqtZA8cx4GEXwcOe... (flutter-team-archive/engine#54063)
2024-07-23 chris@bracken.jp [iOS] Flush layer pool after platform view dispose (flutter-team-archive/engine#54056)
2024-07-23 chris@bracken.jp [iOS] Mark EmbeddedViewCount const (flutter-team-archive/engine#54062)
2024-07-23 skia-flutter-autoroll@skia.org Roll Skia from f4355cf73508 to 3f1b4e98f65a (1 revision) (flutter-team-archive/engine#54060)
2024-07-23 skia-flutter-autoroll@skia.org Roll Skia from 2d518b6a793a to f4355cf73508 (9 revisions) (flutter-team-archive/engine#54058)
2024-07-23 skia-flutter-autoroll@skia.org Roll Dart SDK from eeb2e4e409bf to 17131486a2f7 (1 revision) (flutter-team-archive/engine#54055)
2024-07-23 skia-flutter-autoroll@skia.org Roll Skia from a9019fddac28 to 2d518b6a793a (4 revisions) (flutter-team-archive/engine#54052)
2024-07-23 jonahwilliams@google.com [Impeller] add emulated advanced blend support for exp canvas. (flutter-team-archive/engine#54020)
2024-07-23 skia-flutter-autoroll@skia.org Roll Skia from 1cda2a7b0ee4 to a9019fddac28 (3 revisions) (flutter-team-archive/engine#54050)
2024-07-23 flar@google.com [DisplayList] track unbounded state on save layers and DisplayLists (flutter-team-archive/engine#54032)
2024-07-23 robert.ancell@canonical.com Temporarily disable use of glBlitFramebuffer on NVIDIA (flutter-team-archive/engine#54040)
2024-07-23 robert.ancell@canonical.com Set the view ID for FlView (flutter-team-archive/engine#54043)
2024-07-23 chinmaygarde@google.com [Impeller] Implement OpenGL to Vulkan texture trampolining. (flutter-team-archive/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 jonahwilliams@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
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
…52203)

flutter-team-archive/engine@f5ec4ab...1572635

2024-07-24 58529443+srujzs@users.noreply.github.com Change JSArray.length to return int (flutter-team-archive/engine#54051)
2024-07-24 skia-flutter-autoroll@skia.org Roll Dart SDK from 17131486a2f7 to e53beb039093 (1 revision) (flutter-team-archive/engine#54066)
2024-07-24 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from ZcBsXDojTYbriHD7_... to qA7S-DZ5FyMtcM7_J... (flutter-team-archive/engine#54064)
2024-07-24 flar@google.com [DisplayList] Fix assertions on DisplayList verbose comparison tests (flutter-team-archive/engine#54065)
2024-07-24 skia-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from 5bzzKaW7fCp_No_w_... to clqtZA8cx4GEXwcOe... (flutter-team-archive/engine#54063)
2024-07-23 chris@bracken.jp [iOS] Flush layer pool after platform view dispose (flutter-team-archive/engine#54056)
2024-07-23 chris@bracken.jp [iOS] Mark EmbeddedViewCount const (flutter-team-archive/engine#54062)
2024-07-23 skia-flutter-autoroll@skia.org Roll Skia from f4355cf73508 to 3f1b4e98f65a (1 revision) (flutter-team-archive/engine#54060)
2024-07-23 skia-flutter-autoroll@skia.org Roll Skia from 2d518b6a793a to f4355cf73508 (9 revisions) (flutter-team-archive/engine#54058)
2024-07-23 skia-flutter-autoroll@skia.org Roll Dart SDK from eeb2e4e409bf to 17131486a2f7 (1 revision) (flutter-team-archive/engine#54055)
2024-07-23 skia-flutter-autoroll@skia.org Roll Skia from a9019fddac28 to 2d518b6a793a (4 revisions) (flutter-team-archive/engine#54052)
2024-07-23 jonahwilliams@google.com [Impeller] add emulated advanced blend support for exp canvas. (flutter-team-archive/engine#54020)
2024-07-23 skia-flutter-autoroll@skia.org Roll Skia from 1cda2a7b0ee4 to a9019fddac28 (3 revisions) (flutter-team-archive/engine#54050)
2024-07-23 flar@google.com [DisplayList] track unbounded state on save layers and DisplayLists (flutter-team-archive/engine#54032)
2024-07-23 robert.ancell@canonical.com Temporarily disable use of glBlitFramebuffer on NVIDIA (flutter-team-archive/engine#54040)
2024-07-23 robert.ancell@canonical.com Set the view ID for FlView (flutter-team-archive/engine#54043)
2024-07-23 chinmaygarde@google.com [Impeller] Implement OpenGL to Vulkan texture trampolining. (flutter-team-archive/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 jonahwilliams@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 Merge PR when tree becomes green via auto submit App platform-ios

Development

Successfully merging this pull request may close these issues.

[Memory leak][iOS] PlatformView with UIImageView & Scrolling get junk on showModalBottomSheet

5 participants