Limit heap growth on Android #20473

dnfield · 2020-08-13T08:26:02Z

Need to figure out how to test this, as all the surrounding code is untested 😢

jason-simmons · 2020-09-18T19:25:32Z

shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java

+          result.appStoragePath,
+          result.engineCachesPath,
+          initTimeMillis,
+          oldGenHeapSizeMegaBytes);


Can oldGenHeapSizeMegaBytes be provided as a shell command line flag in shell/common/switches.h?

It could but then we lose some static type checking here. Is there some reason you have in mind to do it that way?

It would be more consistent with how other engine settings are assigned and would avoid a proliferation of nativeInit arguments

xster · 2020-09-18T19:50:47Z

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java

 */
 @Keep
 public class FlutterJNI {
+  public static class FlutterJNILoader {


Making all the static private SG. But rather than making this class 2 tiered which is a bit more confusing, why not just let classes that want to mock FlutterJNI just inject a FlutterJNI mock directly so all methods can be mocked?

It limits the scope of what FlutterLoader gets, making it a little easier to quickly see exactly what it expects to use and how these things are related.

IOW, writing tests for this won't leave me wondering which of the many methods on FlutterJNI I'm supposed to mock.

Jason might have the historical background. I believe we made all the methods in the FlutterJNI class grouped in one class instead of being sliced and diced per consumer based on least privilege principle for a reason (probably to have a single place to look at the interop schema). If so, I strongly believe we should have a single pattern in the codebase to be consistent. Either keep everything in one class (and remove statics which I support) or break it all up and parcel views of the JNI to its consumer based on what the consumer needs.

Otherwise, it becomes arbitrary why I could mock the FlutterLoader's JNI loader but on some JNI calls, it still crashes.

I think the cleanest way for your intended change is still to make all the statics normal instance methods for the public APIs and inject the whole FlutterJNI in tests.

The actual native methods are still on Flutter JNI, which still keeps it easier for us to write the JNI bindings.

I'm not really clear on why we would want to keep all of the Java facing methods on the same class. Flutter JNI will stick around as this sort of God-object, and everyone you give it to will be able to call all sorts of native methods that may not make sense.

I don't work on this code very much, so I'm not going to put up a huge fight about this. If, seeing what I'm saying, you still believe these methods should all be exposed on the Java side via the same one object, I can easily refactor this PR to do that - although if I were working on this more I would more strongly object :)

Talked offline. I'll move out this part of the change here, we can track what to do with FlutterJNI overall in flutter/flutter#66131

I've also made the static methods public again, but marked them as deprecated with pointers to the instance methods.

Ping me when it's ready again

It should be ready now :) The test failure is an infra flake.

xster · 2020-09-18T22:55:06Z

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java

 */
 @Keep
 public class FlutterJNI {
+  public static class FlutterJNILoader {


Jason might have the historical background. I believe we made all the methods in the FlutterJNI class grouped in one class instead of being sliced and diced per consumer based on least privilege principle for a reason (probably to have a single place to look at the interop schema). If so, I strongly believe we should have a single pattern in the codebase to be consistent. Either keep everything in one class (and remove statics which I support) or break it all up and parcel views of the JNI to its consumer based on what the consumer needs.

Otherwise, it becomes arbitrary why I could mock the FlutterLoader's JNI loader but on some JNI calls, it still crashes.

I think the cleanest way for your intended change is still to make all the statics normal instance methods for the public APIs and inject the whole FlutterJNI in tests.

xster · 2020-09-18T22:55:51Z

shell/platform/android/io/flutter/FlutterInjector.java

-   *
-   * <p>Useful for testing since JVM tests via Robolectric can't load native libraries.
-   */
-  public boolean shouldLoadNative() {


FWIW, this is a breaking change. I still haven't merged the website PR for the last time this broke so I'll hold off until we resolve this PR. flutter/website#4561

xster

Thanks for making the changes.

xster · 2020-09-22T04:14:04Z

shell/platform/android/io/flutter/FlutterInjector.java

    public FlutterInjector build() {
      fillDefaults();

-      System.out.println("should load native is " + shouldLoadNative);


🤦‍♀️ thanks

xster · 2020-09-22T04:15:34Z

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java


+  // BEGIN Methods related to loading for FlutterLoader.
+  /**
+   * Loads the libflutter.so library.


great. Thanks for adding docs

xster · 2020-09-22T04:16:17Z

shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java


+  // BEGIN Methods related to loading for FlutterLoader.
+  /**
+   * Loads the libflutter.so library.


ultra-nit: Loads the libflutter.so Flutter engine native library.?

libflutter.so C++ library good?

xster · 2020-09-22T04:17:53Z

shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java

    return instance;
  }

+  public FlutterLoader() {


xster · 2020-09-22T04:18:51Z

shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java

+    this(null);
+  }
+
+  public FlutterLoader(@Nullable FlutterJNI flutterJNILoader) {


replace all flutterJNILoader with just flutterJNI. Since there's nothing else called jni loader here.

Done, also made this @nonnullable and made the default ctor new one up.

xster · 2020-09-22T04:22:12Z

shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java

+      ActivityManager activityManager =
+          (ActivityManager) applicationContext.getSystemService(Context.ACTIVITY_SERVICE);
+      int oldGenHeapSizeMegaBytes = activityManager.getLargeMemoryClass();
+      shellArgs.add("--old-gen-heap-size=" + oldGenHeapSizeMegaBytes);


Is this equality right? I assume your total memory size needs to accommodate everything right? Native memory, old gen, new gen, whatever else the VM mmaps. The old gen can't take up everything I imagine.

This is what we have control over.

I'm probably garbling this up a little bit, but basically new space is moved to old space once it survives collection, and things are put directly into old space if they're over a certain size threshhold. So basically, the majority of heap space is reserved for old space. New space is kept smaller so that new shortlived garbage can be collected more aggressively.

This will cause Dart to run collections much more aggressively as we approach this limit. @rmacnak-google - will this be a hard limit for the VM, or will it be possible to go over it?

We might want to bump this up a bit if it's a hard limit now that I think about it. Android doesn't want us to use more than this if we're good citizens, but this value might be a little lower than what's really "safe" to use in practice.

I don't know what this does behind the scene. I guess the desired behavior is probably like if we start trying to collect when it's like 60% of this value, then it's probably safe. If it collects at this value, we're probably going to have trouble.

This acts like a limit the Dart VM tries to keep us from approaching asymptotically.

But this value is much lower than the limit we use today, which is smething like 32GB (and probably more ram than any Android device out there has). The highest I've seen this value is 512gb, on phones that have ~4gb of ram.

512gb -> 512mb

xster · 2020-09-22T04:23:20Z

shell/platform/android/test/io/flutter/embedding/engine/PluginComponentTest.java

    // Setup test.
-    FlutterInjector.setInstance(new FlutterInjector.Builder().setShouldLoadNative(false).build());
+    FlutterJNI mockFlutterJNILoader = mock(FlutterJNI.class);
+    FlutterInjector.setInstance(


Cool, lg. Can you push some changes to flutter/website#4561 directly for the new pattern?

flutter/website@301b89b

xster · 2020-09-22T04:23:40Z

shell/platform/android/test/io/flutter/embedding/engine/loader/FlutterLoaderTest.java

+import android.app.ActivityManager;
+import android.content.Context;
+import io.flutter.embedding.engine.FlutterJNI;
+import java.util.*;


import specific classes if possible.

shell/platform/android/test/io/flutter/embedding/engine/loader/FlutterLoaderTest.java

xster · 2020-09-22T04:24:58Z

shell/platform/android/test/io/flutter/embedding/engine/loader/FlutterLoaderTest.java

-    FlutterInjector.reset();
+    verify(mockFlutterJNILoader, times(1)).loadLibrary();
+
+    ActivityManager activityManager =


can this be a separate test? Since it seems unrelated to the test above.

xster

LG, thanks

xster · 2020-09-22T04:45:53Z

shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java

+  /** Creates a {@code FlutterLoader} that uses a default constructed {@link FlutterJNI}. */
  public FlutterLoader() {
-    this(null);
+    this(new FlutterJNI());


this is nicer

dnfield · 2020-09-28T21:21:05Z

The more I think about this, the more I think we should make this an opt-in parameter. I'm going to change up this PR to do that. We can then look at having some clients set it to whatever level they think is appropriate.

@xster - SGTY?

xster · 2020-09-29T05:18:24Z

Yes I agree

dnfield · 2020-11-12T19:50:02Z

@xster @jason-simmons - I've refactored this to use half of total memory, or a value specified from the AndroidManifest metadata if available.

This will be more in line with what we do on iOS, and should be less surprising to customers - for example, on many devices today, half of total memory will be 2-6 times as much as the large memory class.

chinmaygarde · 2020-11-12T22:34:44Z

I've reported the flake and re-run the presubmit. This looks good to me and already has approvals. Can we land this?

dnfield · 2020-11-12T22:53:33Z

I think that's fine - if @xster or @jason-simmons have any concerns with the new changes we can revisit.

xster · 2020-11-13T06:04:56Z

This seems like a big change. You're sure we don't want to start with just an opt-in?

dnfield · 2020-11-13T06:16:50Z

I think it's more confusing to have this behavior on ios but not on android.

xster · 2020-11-13T06:22:11Z

SG

Limit heap growth to half of totalMem by default, with the option to set it higher or lower in the AndroidManifest

Start

cd9045e

flutter-dashboard bot added the platform-android label Aug 13, 2020

This comment has been minimized.

Sign in to view

googlebot added the cla: yes label Aug 13, 2020

auto-assign bot requested a review from iskakaushik August 13, 2020 08:26

dnfield changed the title ~~Start~~ Limit heap growth on Android Aug 13, 2020

dnfield marked this pull request as draft August 13, 2020 08:26

dnfield removed the request for review from iskakaushik August 13, 2020 08:28

dnfield added 4 commits August 13, 2020 01:30

bytes

de357b2

..

834417b

merge

1ed2d8d

testss

5bebd55

dnfield requested review from chinmaygarde, jason-simmons and xster September 18, 2020 18:54

dnfield marked this pull request as ready for review September 18, 2020 18:54

auto-assign bot requested a review from liyuqian September 18, 2020 18:54

jason-simmons reviewed Sep 18, 2020

View reviewed changes

xster reviewed Sep 18, 2020

View reviewed changes

dnfield added 2 commits September 18, 2020 14:13

Merge remote-tracking branch 'upstream/master' into android_mem_limit

eb485ef

shell args

0847dcd

xster suggested changes Sep 18, 2020

View reviewed changes

make it simpler

f2a9f84

xster reviewed Sep 22, 2020

View reviewed changes

dnfield added 2 commits September 21, 2020 21:39

Merge remote-tracking branch 'upstream/master' into android_mem_limit

96ad46e

xster review

0a345dd

xster approved these changes Sep 22, 2020

View reviewed changes

dnfield mentioned this pull request Sep 22, 2020

add breaking change for FlutterMain.setIsRunningInRobolectricTest removal flutter/website#4561

Merged

dnfield marked this pull request as draft September 29, 2020 17:08

dnfield mentioned this pull request Oct 5, 2020

Flutter needs a way to handle memory constraints around image decoding. flutter/flutter#50024

Closed

dnfield added 2 commits November 12, 2020 10:07

merge

4612bac

Use totalMem and allow it to be overridden in manifest

a118c90

dnfield marked this pull request as ready for review November 12, 2020 19:48

dnfield merged commit d073681 into flutter:master Nov 12, 2020

dnfield deleted the android_mem_limit branch November 12, 2020 22:54

engine-flutter-autoroll mentioned this pull request Nov 13, 2020

Roll Engine from c5c7e83b9e53 to d07368169e3b (35 revisions) flutter/flutter#70417

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 13, 2020

d073681 Limit heap growth on Android (flutter/engine#20473)

84d46a8

engine-flutter-autoroll mentioned this pull request Nov 13, 2020

Roll Engine from c5c7e83b9e53 to 1ff237fdca45 (38 revisions) flutter/flutter#70425

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 13, 2020

d073681 Limit heap growth on Android (flutter/engine#20473)

7a7f0b4

engine-flutter-autoroll mentioned this pull request Nov 13, 2020

Roll Engine from c5c7e83b9e53 to dddb532b5c7b (40 revisions) flutter/flutter#70429

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 13, 2020

d073681 Limit heap growth on Android (flutter/engine#20473)

3976854

gaaclarke mentioned this pull request Nov 13, 2020

Dart version check tripping up on copyright notices with local engine flutter/flutter#70498

Closed

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

Limit heap growth on Android (flutter#20473)

4f9acd1

Limit heap growth to half of totalMem by default, with the option to set it higher or lower in the AndroidManifest

dnfield mentioned this pull request Dec 17, 2020

Lower the default Dart VM old space size on Android flutter/flutter#72546

Open

eggfly mentioned this pull request Aug 11, 2021

All android-arm(32bit) applications crash on android phone with 18G of RAM flutter/flutter#86855

Closed

Limit heap growth on Android #20473

Limit heap growth on Android #20473

Uh oh!

Conversation

dnfield commented Aug 13, 2020

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

xster commented Nov 13, 2020 •

edited

Loading