-
Notifications
You must be signed in to change notification settings - Fork 6k
Limit heap growth on Android #20473
Limit heap growth on Android #20473
Changes from 10 commits
cd9045e
de357b2
834417b
1ed2d8d
5bebd55
eb485ef
0847dcd
f2a9f84
96ad46e
0a345dd
4612bac
a118c90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,23 +62,12 @@ public static void reset() { | |
| instance = null; | ||
| } | ||
|
|
||
| private FlutterInjector(boolean shouldLoadNative, @NonNull FlutterLoader flutterLoader) { | ||
| this.shouldLoadNative = shouldLoadNative; | ||
| private FlutterInjector(@NonNull FlutterLoader flutterLoader) { | ||
| this.flutterLoader = flutterLoader; | ||
| } | ||
|
|
||
| private boolean shouldLoadNative; | ||
| private FlutterLoader flutterLoader; | ||
|
|
||
| /** | ||
| * Returns whether the Flutter Android engine embedding should load the native C++ engine. | ||
| * | ||
| * <p>Useful for testing since JVM tests via Robolectric can't load native libraries. | ||
| */ | ||
| public boolean shouldLoadNative() { | ||
| return shouldLoadNative; | ||
| } | ||
|
|
||
| /** Returns the {@link FlutterLoader} instance to use for the Flutter Android engine embedding. */ | ||
| @NonNull | ||
| public FlutterLoader flutterLoader() { | ||
|
|
@@ -92,20 +81,6 @@ public FlutterLoader flutterLoader() { | |
| * <p>Non-overriden values have reasonable defaults. | ||
| */ | ||
| public static final class Builder { | ||
|
|
||
| private boolean shouldLoadNative = true; | ||
| /** | ||
| * Sets whether the Flutter Android engine embedding should load the native C++ engine. | ||
| * | ||
| * <p>Useful for testing since JVM tests via Robolectric can't load native libraries. | ||
| * | ||
| * <p>Defaults to true. | ||
| */ | ||
| public Builder setShouldLoadNative(boolean shouldLoadNative) { | ||
| this.shouldLoadNative = shouldLoadNative; | ||
| return this; | ||
| } | ||
|
|
||
| private FlutterLoader flutterLoader; | ||
| /** | ||
| * Sets a {@link FlutterLoader} override. | ||
|
|
@@ -130,8 +105,7 @@ private void fillDefaults() { | |
| public FlutterInjector build() { | ||
| fillDefaults(); | ||
|
|
||
| System.out.println("should load native is " + shouldLoadNative); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤦♀️ thanks |
||
| return new FlutterInjector(shouldLoadNative, flutterLoader); | ||
| return new FlutterInjector(flutterLoader); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
|
|
||
| package io.flutter.embedding.engine.loader; | ||
|
|
||
| import android.app.ActivityManager; | ||
| import android.content.Context; | ||
| import android.content.pm.PackageManager; | ||
| import android.content.res.AssetManager; | ||
|
|
@@ -15,7 +16,6 @@ | |
| import androidx.annotation.NonNull; | ||
| import androidx.annotation.Nullable; | ||
| import io.flutter.BuildConfig; | ||
| import io.flutter.FlutterInjector; | ||
| import io.flutter.embedding.engine.FlutterJNI; | ||
| import io.flutter.util.PathUtils; | ||
| import io.flutter.view.VsyncWaiter; | ||
|
|
@@ -60,10 +60,26 @@ public static FlutterLoader getInstance() { | |
| return instance; | ||
| } | ||
|
|
||
| /** Creates a {@code FlutterLoader} that uses a default constructed {@link FlutterJNI}. */ | ||
| public FlutterLoader() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Docs
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| this(new FlutterJNI()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is nicer |
||
| } | ||
|
|
||
| /** | ||
| * Creates a {@code FlutterLoader} with the specified {@link FlutterJNI}. | ||
| * | ||
| * @param flutterJNI The {@link FlutterJNI} instance to use for loading the libflutter.so C++ | ||
| * library, setting up the font manager, and calling into C++ initalization. | ||
| */ | ||
| public FlutterLoader(@NonNull FlutterJNI flutterJNI) { | ||
| this.flutterJNI = flutterJNI; | ||
| } | ||
|
|
||
| private boolean initialized = false; | ||
| @Nullable private Settings settings; | ||
| private long initStartTimestampMillis; | ||
| private FlutterApplicationInfo flutterApplicationInfo; | ||
| private FlutterJNI flutterJNI; | ||
|
|
||
| private static class InitResult { | ||
| final String appStoragePath; | ||
|
|
@@ -125,9 +141,7 @@ public void startInitialization(@NonNull Context applicationContext, @NonNull Se | |
| public InitResult call() { | ||
| ResourceExtractor resourceExtractor = initResources(appContext); | ||
|
|
||
| if (FlutterInjector.instance().shouldLoadNative()) { | ||
| System.loadLibrary("flutter"); | ||
| } | ||
| flutterJNI.loadLibrary(); | ||
|
|
||
| // Prefetch the default font manager as soon as possible on a background thread. | ||
| // It helps to reduce time cost of engine setup that blocks the platform thread. | ||
|
|
@@ -136,7 +150,7 @@ public InitResult call() { | |
| new Runnable() { | ||
| @Override | ||
| public void run() { | ||
| FlutterJNI.nativePrefetchDefaultFontManager(); | ||
| flutterJNI.prefetchDefaultFontManager(); | ||
| } | ||
| }); | ||
|
|
||
|
|
@@ -225,17 +239,20 @@ public void ensureInitializationComplete( | |
| shellArgs.add("--log-tag=" + settings.getLogTag()); | ||
| } | ||
|
|
||
| ActivityManager activityManager = | ||
| (ActivityManager) applicationContext.getSystemService(Context.ACTIVITY_SERVICE); | ||
| int oldGenHeapSizeMegaBytes = activityManager.getLargeMemoryClass(); | ||
|
||
| shellArgs.add("--old-gen-heap-size=" + oldGenHeapSizeMegaBytes); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 512gb -> 512mb |
||
|
|
||
| long initTimeMillis = SystemClock.uptimeMillis() - initStartTimestampMillis; | ||
|
|
||
| if (FlutterInjector.instance().shouldLoadNative()) { | ||
| FlutterJNI.nativeInit( | ||
| applicationContext, | ||
| shellArgs.toArray(new String[0]), | ||
| kernelPath, | ||
| result.appStoragePath, | ||
| result.engineCachesPath, | ||
| initTimeMillis); | ||
| } | ||
| flutterJNI.init( | ||
| applicationContext, | ||
| shellArgs.toArray(new String[0]), | ||
| kernelPath, | ||
| result.appStoragePath, | ||
| result.engineCachesPath, | ||
| initTimeMillis); | ||
|
|
||
| initialized = true; | ||
| } catch (Exception e) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,11 +32,13 @@ public void setUp() { | |
| @Test | ||
| public void pluginsCanAccessFlutterAssetPaths() { | ||
| // Setup test. | ||
| FlutterInjector.setInstance(new FlutterInjector.Builder().setShouldLoadNative(false).build()); | ||
| FlutterJNI mockFlutterJNI = mock(FlutterJNI.class); | ||
| FlutterInjector.setInstance( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, lg. Can you push some changes to flutter/website#4561 directly for the new pattern?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| new FlutterInjector.Builder().setFlutterLoader(new FlutterLoader(mockFlutterJNI)).build()); | ||
| FlutterJNI flutterJNI = mock(FlutterJNI.class); | ||
| when(flutterJNI.isAttached()).thenReturn(true); | ||
|
|
||
| FlutterLoader flutterLoader = new FlutterLoader(); | ||
| FlutterLoader flutterLoader = new FlutterLoader(mockFlutterJNI); | ||
|
|
||
| // Execute behavior under test. | ||
| FlutterEngine flutterEngine = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,24 +4,32 @@ | |
|
|
||
| package io.flutter.embedding.engine.loader; | ||
|
|
||
| import static android.os.Looper.getMainLooper; | ||
| import static junit.framework.TestCase.assertFalse; | ||
| import static junit.framework.TestCase.assertTrue; | ||
| import static org.mockito.Mockito.anyLong; | ||
| import static org.mockito.Mockito.anyString; | ||
| import static org.mockito.Mockito.eq; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.times; | ||
| import static org.mockito.Mockito.verify; | ||
| import static org.robolectric.Shadows.shadowOf; | ||
|
|
||
| import io.flutter.FlutterInjector; | ||
| import org.junit.Before; | ||
| import android.app.ActivityManager; | ||
| import android.content.Context; | ||
| import io.flutter.embedding.engine.FlutterJNI; | ||
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import org.junit.Test; | ||
| import org.junit.runner.RunWith; | ||
| import org.mockito.ArgumentCaptor; | ||
| import org.robolectric.RobolectricTestRunner; | ||
| import org.robolectric.RuntimeEnvironment; | ||
| import org.robolectric.annotation.Config; | ||
|
|
||
| @Config(manifest = Config.NONE) | ||
| @RunWith(RobolectricTestRunner.class) | ||
| public class FlutterLoaderTest { | ||
| @Before | ||
| public void setUp() { | ||
| FlutterInjector.reset(); | ||
| } | ||
|
|
||
| @Test | ||
| public void itReportsUninitializedAfterCreating() { | ||
|
|
@@ -31,13 +39,40 @@ public void itReportsUninitializedAfterCreating() { | |
|
|
||
| @Test | ||
| public void itReportsInitializedAfterInitializing() { | ||
| FlutterInjector.setInstance(new FlutterInjector.Builder().setShouldLoadNative(false).build()); | ||
| FlutterLoader flutterLoader = new FlutterLoader(); | ||
| FlutterJNI mockFlutterJNI = mock(FlutterJNI.class); | ||
| FlutterLoader flutterLoader = new FlutterLoader(mockFlutterJNI); | ||
|
|
||
| assertFalse(flutterLoader.initialized()); | ||
| flutterLoader.startInitialization(RuntimeEnvironment.application); | ||
| flutterLoader.ensureInitializationComplete(RuntimeEnvironment.application, null); | ||
| shadowOf(getMainLooper()).idle(); | ||
| assertTrue(flutterLoader.initialized()); | ||
| FlutterInjector.reset(); | ||
| verify(mockFlutterJNI, times(1)).loadLibrary(); | ||
| } | ||
|
|
||
| @Test | ||
| public void itSetsTheOldGenHeapSizeAppropriately() { | ||
| FlutterJNI mockFlutterJNI = mock(FlutterJNI.class); | ||
| FlutterLoader flutterLoader = new FlutterLoader(mockFlutterJNI); | ||
|
|
||
| assertFalse(flutterLoader.initialized()); | ||
| flutterLoader.startInitialization(RuntimeEnvironment.application); | ||
| flutterLoader.ensureInitializationComplete(RuntimeEnvironment.application, null); | ||
| shadowOf(getMainLooper()).idle(); | ||
|
|
||
| ActivityManager activityManager = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can this be a separate test? Since it seems unrelated to the test above.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| (ActivityManager) RuntimeEnvironment.application.getSystemService(Context.ACTIVITY_SERVICE); | ||
| final String oldGenHeapArg = "--old-gen-heap-size=" + activityManager.getLargeMemoryClass(); | ||
| ArgumentCaptor<String[]> shellArgsCaptor = ArgumentCaptor.forClass(String[].class); | ||
| verify(mockFlutterJNI, times(1)) | ||
| .init( | ||
| eq(RuntimeEnvironment.application), | ||
| shellArgsCaptor.capture(), | ||
| anyString(), | ||
| anyString(), | ||
| anyString(), | ||
| anyLong()); | ||
| List<String> arguments = Arrays.asList(shellArgsCaptor.getValue()); | ||
| assertTrue(arguments.contains(oldGenHeapArg)); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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