Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

toImageSync retains display list which can lead to surprising memory retention. #138627

Open
2 tasks done
alnitak opened this issue Nov 17, 2023 · 9 comments
Open
2 tasks done
Labels
a: desktop Running on desktop c: performance Relates to speed or footprint issues (see "perf:" labels) engine flutter/engine repository. See also e: labels. found in release: 3.16 Found to occur in 3.16 found in release: 3.17 Found to occur in 3.17 has reproducible steps The issue has been confirmed reproducible and is ready to work on P2 Important issues not at the top of the work list perf: memory Performance issues related to memory team-engine Owned by Engine team triaged-engine Triaged by Engine team

Comments

@alnitak
Copy link

alnitak commented Nov 17, 2023

Is there an existing issue for this?

Steps to reproduce

  1. run example
  2. wait 40~60 to see memory leak on the OS memory monitor that is not seen in DevTools

Actual results

All the PC memory get filled till system crash

Code sample

I have created a public GitHub project as short as I could.
Some more info in the README.md

Screenshots or Video

After about 40~60 seconds memory leak is starting growing till it reach 100%.
DevTools memory tab doesn't show memory leaks.

Screenshots

Screenshot from 2023-11-17 16-29-57

Flutter Doctor output

Doctor output
[✓] Flutter (Channel stable, 3.16.0, on Manjaro Linux 6.1.62-1-MANJARO, locale en_US.UTF-8)
    • Flutter version 3.16.0 on channel stable at /home/deimos/dev/flutter
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision db7ef5bf9f (2 days ago), 2023-11-15 11:25:44 -0800
    • Engine revision 74d16627b9
    • Dart version 3.2.0
    • DevTools version 2.28.2

[✓] Android toolchain - develop for Android devices (Android SDK version 34.0.0)
    • Android SDK at /home/deimos/Android/Sdk
    • Platform android-34, build-tools 34.0.0
    • ANDROID_HOME = /home/deimos/Android/Sdk
    • ANDROID_SDK_ROOT = /home/deimos/Android/Sdk
    • Java binary at: /home/deimos/dev/android-studio/jbr/bin/java
    • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b802.4-9586694)
    • All Android licenses accepted.

[✓] Chrome - develop for the web
    • CHROME_EXECUTABLE = /bin/chromium

[✓] Linux toolchain - develop for Linux desktop
    • clang version 16.0.6
    • cmake version 3.27.7
    • ninja version 1.11.1
    • pkg-config version 1.8.1

[✓] Android Studio (version 2022.2)
    • Android Studio at /home/deimos/dev/android-studio
    • Flutter plugin version 75.1.1
    • Dart plugin version 222.4582
    • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b802.4-9586694)

[✓] Connected device (2 available)
    • Linux (desktop) • linux  • linux-x64      • Manjaro Linux 6.1.62-1-MANJARO
    • Chrome (web)    • chrome • web-javascript • Chromium 119.0.6045.159 Arch Linux

[✓] Network resources
    • All expected network resources are available.

• No issues found!
@huycozy huycozy added the in triage Presently being triaged by the triage team label Nov 20, 2023
@huycozy
Copy link
Member

huycozy commented Nov 20, 2023

I ran your sample code and can reproduce it on Linux and macOS Flutter app. It doesn't seem to occur on Web platform.

flutter doctor -v (stable & master)
[✓] Flutter (Channel stable, 3.16.0, on Ubuntu 22.04.3 LTS 6.2.0-36-generic, locale en_US.UTF-8)
    • Flutter version 3.16.0 on channel stable at /home/huynq/Documents/Working/flutter
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision db7ef5bf9f (5 days ago), 2023-11-15 11:25:44 -0800
    • Engine revision 74d16627b9
    • Dart version 3.2.0
    • DevTools version 2.28.2

[✓] Android toolchain - develop for Android devices (Android SDK version 34.0.0-rc3)
    • Android SDK at /home/huynq/Android/Sdk/
    • Platform android-33, build-tools 34.0.0-rc3
    • Java binary at: /home/huynq/Documents/android-studio/jre/bin/java
    • Java version OpenJDK Runtime Environment (build 11.0.11+0-b60-7590822)
    • All Android licenses accepted.

[✓] Chrome - develop for the web
    • Chrome at google-chrome

[✓] Linux toolchain - develop for Linux desktop
    • clang version 10.0.0-4ubuntu1
    • cmake version 3.22.1
    • ninja version 1.10.1
    • pkg-config version 0.29.2

[✓] Android Studio (version 2022.3)
    • Android Studio at /snap/android-studio/128/android-studio
    • Flutter plugin version 76.2.2
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b829.9-10027231)

[✓] Android Studio (version 2021.1)
    • Android Studio at /home/huynq/Documents/android-studio/
    • Flutter plugin version 67.0.1
    • Dart plugin version 211.7817
    • android-studio-dir = /home/huynq/Documents/android-studio/
    • Java version OpenJDK Runtime Environment (build 11.0.11+0-b60-7590822)

[✓] VS Code (version 1.83.1)
    • VS Code at /usr/share/code
    • Flutter extension version 3.76.0

[✓] Connected device (2 available)
    • Linux (desktop) • linux  • linux-x64      • Ubuntu 22.04.3 LTS 6.2.0-36-generic
    • Chrome (web)    • chrome • web-javascript • Google Chrome 118.0.5993.70

[✓] Network resources
    • All expected network resources are available.

• No issues found!
[!] Flutter (Channel master, 3.17.0-10.0.pre.61, on Ubuntu 22.04.3 LTS 6.2.0-36-generic, locale en_US.UTF-8)
    • Flutter version 3.17.0-10.0.pre.61 on channel master at /home/huynq/Documents/Working/flutter_master
    ! Warning: `flutter` on your path resolves to /home/huynq/Documents/Working/flutter/bin/flutter, which is not inside your current Flutter SDK checkout at /home/huynq/Documents/Working/flutter_master. Consider adding /home/huynq/Documents/Working/flutter_master/bin to the front of your path.
    ! Warning: `dart` on your path resolves to /home/huynq/Documents/Working/flutter/bin/dart, which is not inside your current Flutter SDK checkout at /home/huynq/Documents/Working/flutter_master. Consider adding /home/huynq/Documents/Working/flutter_master/bin to the front of your path.
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision d2b67d4cea (2 hours ago), 2023-11-20 01:06:28 -0500
    • Engine revision e916d5bfdd
    • Dart version 3.3.0 (build 3.3.0-149.0.dev)
    • DevTools version 2.30.0-dev.4
    • If those were intentional, you can disregard the above warnings; however it is recommended to use "git" directly to perform update checks and upgrades.

[✓] Android toolchain - develop for Android devices (Android SDK version 34.0.0-rc3)
    • Android SDK at /home/huynq/Android/Sdk/
    • Platform android-33, build-tools 34.0.0-rc3
    • Java binary at: /home/huynq/Documents/android-studio/jre/bin/java
    • Java version OpenJDK Runtime Environment (build 11.0.11+0-b60-7590822)
    • All Android licenses accepted.

[✓] Chrome - develop for the web
    • Chrome at google-chrome

[✓] Linux toolchain - develop for Linux desktop
    • clang version 10.0.0-4ubuntu1
    • cmake version 3.22.1
    • ninja version 1.10.1
    • pkg-config version 0.29.2

[✓] Android Studio (version 2022.3)
    • Android Studio at /snap/android-studio/128/android-studio
    • Flutter plugin version 76.2.2
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 17.0.6+0-17.0.6b829.9-10027231)

[✓] Android Studio (version 2021.1)
    • Android Studio at /home/huynq/Documents/android-studio/
    • Flutter plugin version 67.0.1
    • Dart plugin version 211.7817
    • android-studio-dir = /home/huynq/Documents/android-studio/
    • Java version OpenJDK Runtime Environment (build 11.0.11+0-b60-7590822)

[✓] VS Code (version 1.83.1)
    • VS Code at /usr/share/code
    • Flutter extension version 3.76.0

[✓] Connected device (2 available)
    • Linux (desktop) • linux  • linux-x64      • Ubuntu 22.04.3 LTS 6.2.0-36-generic
    • Chrome (web)    • chrome • web-javascript • Google Chrome 118.0.5993.70

[✓] Network resources
    • All expected network resources are available.

! Doctor found issues in 1 category.

@huycozy huycozy added engine flutter/engine repository. See also e: labels. a: desktop Running on desktop perf: memory Performance issues related to memory has reproducible steps The issue has been confirmed reproducible and is ready to work on team-desktop found in release: 3.16 Found to occur in 3.16 found in release: 3.17 Found to occur in 3.17 c: performance Relates to speed or footprint issues (see "perf:" labels) and removed in triage Presently being triaged by the triage team labels Nov 20, 2023
@gspencergoog gspencergoog added team-engine Owned by Engine team and removed team-desktop labels Nov 30, 2023
@jonahwilliams
Copy link
Member

@alnitak It doesn't look like you are ever disposing the image objects. This may result in your application running out of memory, as you are relying on the Dart GC to clear objects that are holding onto substantial native memory.

Its not obvious to me that there is a specific bug with setImageSampler, but this could use more investigation at any rate.

@jonahwilliams jonahwilliams added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Dec 4, 2023
@cmkweber
Copy link

cmkweber commented Dec 4, 2023

It looks like its disposing the previous image each tick:
sampler1?.dispose();

And disposing the current picture after the image has been acquired:
picture.dispose();

Would this still be considered leaving it to GC?

@jonahwilliams
Copy link
Member

Yeah good point, but it also looks like this creates an endless chain of sampler 1 -> sampler 2 -> sampler 1. Since toImageSync stores its display list that is likely keeping all images alive indefinitely

@jonahwilliams
Copy link
Member

FYI @dnfield @jason-simmons I think this is actually an issue with toImageSync

@alnitak
Copy link
Author

alnitak commented Dec 4, 2023

@alnitak It doesn't look like you are ever disposing the image objects. This may result in your application running out of memory, as you are relying on the Dart GC to clear objects that are holding onto substantial native memory.

Its not obvious to me that there is a specific bug with setImageSampler, but this could use more investigation at any rate.

Thanks for your time looking at this.
As noted by @cmkweber images and pictures are disposed. At a first glance I though it was a toImageSync problem too. The thing that got me thinking about setImageSampler is that if I use an image already in memory instead of the previous sampler2, the problem doesn't occurs, ie changing:
..setImageSampler(0, sampler2 ?? blankImage!);
to
..setImageSampler(0, blankImage!);

I would also like to highlight the fact that I don't notice any increasing memory while monitoring memory on DevTools, the Dart GC seems normal, but just looking at the OS memory monitor.

@github-actions github-actions bot removed the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Dec 4, 2023
@jason-simmons
Copy link
Member

The object returned by toImageSync internally holds a reference to the display list of the picture that was used to create the image.

So if the picture created on the current tick uses the sampler2 created on the previous tick, then the toImageSync image will retain a reference to that sampler2. This reference is held within the returned image object and is still present after calling dispose() on the picture or on the old sampler2 object.

If the previous tick's sampler2 was created in the same way, then it will hold a reference to the sampler2 from the tick before that. This means that the current image holds references to a chain of all the images created by every tick.

The asynchronous Picture.toImage API will create a simple bitmap that does not need to hold a reference to the underlying picture.

@jonahwilliams
Copy link
Member

Perhaps this is an example of a case where we should provide some sort of flattening operation, or the ability to create host resident textures (to ensure we don't need to retain a display list to recreate them).

@jonahwilliams jonahwilliams changed the title FragmentShader.setImageSampler seems to leak memory toImageSync retains display list which can lead to surprising memory retention. Dec 11, 2023
@jonahwilliams jonahwilliams added P2 Important issues not at the top of the work list triaged-engine Triaged by Engine team labels Dec 11, 2023
@alnitak
Copy link
Author

alnitak commented Dec 15, 2023

This issue seems related to the closed #131524

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: desktop Running on desktop c: performance Relates to speed or footprint issues (see "perf:" labels) engine flutter/engine repository. See also e: labels. found in release: 3.16 Found to occur in 3.16 found in release: 3.17 Found to occur in 3.17 has reproducible steps The issue has been confirmed reproducible and is ready to work on P2 Important issues not at the top of the work list perf: memory Performance issues related to memory team-engine Owned by Engine team triaged-engine Triaged by Engine team
Projects
None yet
Development

No branches or pull requests

6 participants