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

Add a custom switch to clean up the leaked Dart VM #30541

Merged
merged 5 commits into from
Jan 21, 2022
Merged

Add a custom switch to clean up the leaked Dart VM #30541

merged 5 commits into from
Jan 21, 2022

Conversation

eggfly
Copy link
Member

@eggfly eggfly commented Dec 27, 2021 •
edited
Loading

It's both Android and iOS part of:

Considering the leak_vm setting is a process global option, I put it into AndroidManifest.xml's app meta-data, like the OldGenHeapSize option.

Demo app link: https://github.com/eggfly/flutter_leak_vm_test

Code:

https://github.com/eggfly/flutter_leak_vm_test/blob/612b2271c90de68f9bbaf03696fcdb18bcd73745/android/app/src/main/AndroidManifest.xml#L51

 <!-- Force clean up the leak VM -->
        <meta-data
            android:name="io.flutter.embedding.android.LeakVM"
            android:value="false" />

Keep switching between native MainActivity and flutter activity, and observe the memory usage.

LeakVM is true (or not set): back to native Activity then native memory = 34.9MB
image

LeakVM is false: back to native Activity then native memory = 32.2MB
image

Some debug logs:

image

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.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@eggfly eggfly force-pushed the disable_leak_vm branch 2 times, most recently from b87799d to 6c888c9 Compare December 28, 2021 12:41
@eggfly eggfly mentioned this pull request Dec 28, 2021
Closed
2 tasks
@eggfly eggfly changed the title WIP: Add switch to clean up leak vm [android] Add a custom switch to clean up the leak vm Dec 28, 2021
@eggfly eggfly requested review from dnfield, jason-simmons and chinmaygarde and removed request for dnfield December 28, 2021 14:21
chinmaygarde
chinmaygarde reviewed Dec 28, 2021
View reviewed changes
Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

cc @blasten for whether this we should do this. I think, if anything, we should switch the default on the flag to never leak the VM.

Ideally, this flag should not exist and the last instance of the shell should always collect the VM. But this was added because of benchmark regressions on application resumption. IMO, we should migrate to non-leaky behavior instead of exposing another flag.

@eggfly eggfly requested review from chinmaygarde and mklim December 29, 2021 03:40
@blasten
Copy link

blasten commented Jan 5, 2022

@goderbauer @dnfield
do you know if customer:money needs this flag to be on by default?

Some context about this flag:

engine/common/settings.h

Lines 198 to 209 in ea734ba

// All shells in the process share the same VM. The last shell to shutdown
// should typically shut down the VM as well. However, applications depend on
// the behavior of "warming-up" the VM by creating a shell that does not do
// anything. This used to work earlier when the VM could not be shut down (and
// hence never was). Shutting down the VM now breaks such assumptions in
// existing embedders. To keep this behavior consistent and allow existing
// embedders the chance to migrate, this flag defaults to true. Any shell
// launched with this flag set to true will leak the VM in the process. There
// is no way to shut down the VM once such a shell has been started. All
// shells in the platform (via their embedding APIs) should cooperate to make
// sure this flag is never set if they want the VM to shutdown and free all
// associated resources.

@blasten blasten removed the request for review from mklim January 5, 2022 20:57
@eggfly eggfly requested a review from blasten January 6, 2022 02:52
@blasten
Copy link

blasten commented Jan 7, 2022

This change seems reasonable. I also think that the default behavior should be to never leak the VM. Do you know how this interacts with cached engines, e.g. https://api.flutter.dev/javadoc/io/flutter/embedding/engine/FlutterEngineCache.html ?

@dnfield
Copy link
Contributor

dnfield commented Jan 10, 2022

This flag was specifically set up because on Android and iOS applications, going to and from background was causing the VM to restart. We don't want that to happen because it is slow to restart the VM.

@eggfly
Copy link
Member Author

eggfly commented Jan 11, 2022 •
edited
Loading

This flag was specifically set up because on Android and iOS applications, going to and from background was causing the VM to restart. We don't want that to happen because it is slow to restart the VM.

I agree with you. And also add the setLeakVM method for iOS, comment it to YES for default value.

@eggfly eggfly changed the title [android] Add a custom switch to clean up the leak vm Add a custom switch to clean up the leak vm Jan 11, 2022
@eggfly eggfly requested review from stuartmorgan-g and removed request for stuartmorgan-g January 19, 2022 02:37
@blasten
Copy link

blasten commented Jan 19, 2022

Let's land this change with the current default.

Please file an issue to change the default value, and add TODO with the link to the issue.

Another PR can change the default to false, so it can be tested separately.

This was referenced Jan 19, 2022
Closed
Merged
@eggfly
Add more comments and leave a TODO
9cbcdcd 
Also add example code in add-to-app sample project
@eggfly eggfly requested a review from blasten January 19, 2022 08:54
zhongwuzw
zhongwuzw reviewed Jan 19, 2022
View reviewed changes
@eggfly eggfly requested a review from zhongwuzw January 20, 2022 11:31
zhongwuzw
zhongwuzw approved these changes Jan 21, 2022
View reviewed changes
Copy link
Member

@zhongwuzw zhongwuzw left a comment

Choose a reason for hiding this comment

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

LGTM!

@eggfly eggfly added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 21, 2022
@eggfly eggfly changed the title Add a custom switch to clean up the leak vm Add a custom switch to clean up the leak Dart VM Jan 21, 2022
@eggfly eggfly changed the title Add a custom switch to clean up the leak Dart VM Add a custom switch to clean up the leaked Dart VM Jan 21, 2022
@fluttergithubbot fluttergithubbot merged commit 0ba963f into flutter:main Jan 21, 2022
@eggfly
Copy link
Member Author

eggfly commented Jan 21, 2022 •
edited
Loading

Added a sample code in:

@eggfly eggfly deleted the disable_leak_vm branch January 21, 2022 07:24
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 21, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 21, 2022
@engine-flutter-autoroll
0ba963f Add a custom switch to clean up the leaked Dart VM (flutter/e…
d6e15ec 
…ngine#30541)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zhongwuzw zhongwuzw zhongwuzw approved these changes

@jason-simmons jason-simmons Awaiting requested review from jason-simmons

@dnfield dnfield Awaiting requested review from dnfield

@chinmaygarde chinmaygarde Awaiting requested review from chinmaygarde

@blasten blasten Awaiting requested review from blasten

Assignees
No one assigned
Labels
platform-android platform-ios waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
Contributions from Bytedance
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@eggfly @blasten @dnfield @chinmaygarde @zhongwuzw @fluttergithubbot