-
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 5 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 |
|---|---|---|
|
|
@@ -94,6 +94,58 @@ | |
| */ | ||
| @Keep | ||
| public class FlutterJNI { | ||
| public static class FlutterJNILoader { | ||
|
||
| /** | ||
| * Loads the libflutter.so library. | ||
| * | ||
| * <p>This must be called before any other native methods, and can be overridden by tests to | ||
| * avoid loading native libraries. | ||
| */ | ||
| public void loadLibrary() { | ||
| System.loadLibrary("flutter"); | ||
| } | ||
|
|
||
| /** | ||
| * Prefetch the default font manager provided by SkFontMgr::RefDefault() which is a process-wide | ||
| * singleton owned by Skia. Note that, the first call to SkFontMgr::RefDefault() will take | ||
| * noticeable time, but later calls will return a reference to the preexisting font manager. | ||
| */ | ||
| public void prefetchDefaultFontManager() { | ||
| FlutterJNI.nativePrefetchDefaultFontManager(); | ||
| } | ||
|
|
||
| /** | ||
| * Perform one time initialization of the Dart VM and Flutter engine. | ||
| * | ||
| * <p>This method must be called only once. | ||
| * | ||
| * @param context The application context. | ||
| * @param args Arguments to the Dart VM/Flutter engine. | ||
| * @param bundlePath For JIT runtimes, the path to the Dart kernel file for the application. | ||
| * @param appStoragePath The path to the application data directory. | ||
| * @param engineCachesPath The path to the application cache directory. | ||
| * @param initTimeMillis The time, in milliseconds, taken for initialization. | ||
| * @param oldGenHeapSizeMegaBytes The maximum size for the old gen heap size. | ||
| */ | ||
| public void nativeInit( | ||
| @NonNull Context context, | ||
| @NonNull String[] args, | ||
| @Nullable String bundlePath, | ||
| @NonNull String appStoragePath, | ||
| @NonNull String engineCachesPath, | ||
| long initTimeMillis, | ||
| int oldGenHeapSizeMegaBytes) { | ||
| FlutterJNI.nativeInit( | ||
| context, | ||
| args, | ||
| bundlePath, | ||
| appStoragePath, | ||
| engineCachesPath, | ||
| initTimeMillis, | ||
| oldGenHeapSizeMegaBytes); | ||
| } | ||
| } | ||
|
|
||
| private static final String TAG = "FlutterJNI"; | ||
|
|
||
| @Nullable private static AsyncWaitForVsyncDelegate asyncWaitForVsyncDelegate; | ||
|
|
@@ -104,21 +156,16 @@ public class FlutterJNI { | |
| // This is set from native code via JNI. | ||
| @Nullable private static String observatoryUri; | ||
|
|
||
| // TODO(mattcarroll): add javadocs | ||
| public static native void nativeInit( | ||
| private static native void nativeInit( | ||
| @NonNull Context context, | ||
| @NonNull String[] args, | ||
| @Nullable String bundlePath, | ||
| @NonNull String appStoragePath, | ||
| @NonNull String engineCachesPath, | ||
| long initTimeMillis); | ||
| long initTimeMillis, | ||
| int oldGenHeapSizeMegaBytes); | ||
|
|
||
| /** | ||
| * Prefetch the default font manager provided by SkFontMgr::RefDefault() which is a process-wide | ||
| * singleton owned by Skia. Note that, the first call to SkFontMgr::RefDefault() will take | ||
| * noticeable time, but later calls will return a reference to the preexisting font manager. | ||
| */ | ||
| public static native void nativePrefetchDefaultFontManager(); | ||
| private static native void nativePrefetchDefaultFontManager(); | ||
|
|
||
| private native boolean nativeGetIsSoftwareRenderingEnabled(); | ||
|
|
||
|
|
||
| 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,22 @@ public static FlutterLoader getInstance() { | |
| return instance; | ||
| } | ||
|
|
||
| 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(null); | ||
| } | ||
|
|
||
| public FlutterLoader(@Nullable FlutterJNI.FlutterJNILoader flutterJNILoader) { | ||
| if (flutterJNILoader == null) { | ||
| flutterJNILoader = new FlutterJNI.FlutterJNILoader(); | ||
| } | ||
| this.flutterJNILoader = flutterJNILoader; | ||
| } | ||
|
|
||
| private boolean initialized = false; | ||
| @Nullable private Settings settings; | ||
| private long initStartTimestampMillis; | ||
| private FlutterApplicationInfo flutterApplicationInfo; | ||
| private FlutterJNI.FlutterJNILoader flutterJNILoader; | ||
|
|
||
| private static class InitResult { | ||
| final String appStoragePath; | ||
|
|
@@ -125,9 +137,7 @@ public void startInitialization(@NonNull Context applicationContext, @NonNull Se | |
| public InitResult call() { | ||
| ResourceExtractor resourceExtractor = initResources(appContext); | ||
|
|
||
| if (FlutterInjector.instance().shouldLoadNative()) { | ||
| System.loadLibrary("flutter"); | ||
| } | ||
| flutterJNILoader.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 +146,7 @@ public InitResult call() { | |
| new Runnable() { | ||
| @Override | ||
| public void run() { | ||
| FlutterJNI.nativePrefetchDefaultFontManager(); | ||
| flutterJNILoader.prefetchDefaultFontManager(); | ||
| } | ||
| }); | ||
|
|
||
|
|
@@ -225,17 +235,20 @@ public void ensureInitializationComplete( | |
| shellArgs.add("--log-tag=" + settings.getLogTag()); | ||
| } | ||
|
|
||
| ActivityManager activityManager = | ||
| (ActivityManager) applicationContext.getSystemService(Context.ACTIVITY_SERVICE); | ||
| int oldGenHeapSizeMegaBytes = activityManager.getLargeMemoryClass(); | ||
|
||
|
|
||
| long initTimeMillis = SystemClock.uptimeMillis() - initStartTimestampMillis; | ||
|
|
||
| if (FlutterInjector.instance().shouldLoadNative()) { | ||
| FlutterJNI.nativeInit( | ||
| applicationContext, | ||
| shellArgs.toArray(new String[0]), | ||
| kernelPath, | ||
| result.appStoragePath, | ||
| result.engineCachesPath, | ||
| initTimeMillis); | ||
| } | ||
| flutterJNILoader.nativeInit( | ||
| applicationContext, | ||
| shellArgs.toArray(new String[0]), | ||
| kernelPath, | ||
| result.appStoragePath, | ||
| result.engineCachesPath, | ||
| initTimeMillis, | ||
| oldGenHeapSizeMegaBytes); | ||
|
||
|
|
||
| initialized = true; | ||
| } catch (Exception e) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,11 +32,15 @@ public void setUp() { | |
| @Test | ||
| public void pluginsCanAccessFlutterAssetPaths() { | ||
| // Setup test. | ||
| FlutterInjector.setInstance(new FlutterInjector.Builder().setShouldLoadNative(false).build()); | ||
| FlutterJNI.FlutterJNILoader mockFlutterJNILoader = mock(FlutterJNI.FlutterJNILoader.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(mockFlutterJNILoader)) | ||
| .build()); | ||
| FlutterJNI flutterJNI = mock(FlutterJNI.class); | ||
| when(flutterJNI.isAttached()).thenReturn(true); | ||
|
|
||
| FlutterLoader flutterLoader = new FlutterLoader(); | ||
| FlutterLoader flutterLoader = new FlutterLoader(mockFlutterJNILoader); | ||
|
|
||
| // Execute behavior under test. | ||
| FlutterEngine flutterEngine = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,11 +4,21 @@ | |
|
|
||
| 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.any; | ||
| 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 org.junit.Test; | ||
| import org.junit.runner.RunWith; | ||
| import org.robolectric.RobolectricTestRunner; | ||
|
|
@@ -18,10 +28,6 @@ | |
| @Config(manifest = Config.NONE) | ||
| @RunWith(RobolectricTestRunner.class) | ||
| public class FlutterLoaderTest { | ||
| @Before | ||
| public void setUp() { | ||
| FlutterInjector.reset(); | ||
| } | ||
|
|
||
| @Test | ||
| public void itReportsUninitializedAfterCreating() { | ||
|
|
@@ -31,13 +37,26 @@ public void itReportsUninitializedAfterCreating() { | |
|
|
||
| @Test | ||
| public void itReportsInitializedAfterInitializing() { | ||
| FlutterInjector.setInstance(new FlutterInjector.Builder().setShouldLoadNative(false).build()); | ||
| FlutterLoader flutterLoader = new FlutterLoader(); | ||
| FlutterJNI.FlutterJNILoader mockFlutterJNILoader = mock(FlutterJNI.FlutterJNILoader.class); | ||
| FlutterLoader flutterLoader = new FlutterLoader(mockFlutterJNILoader); | ||
|
|
||
| assertFalse(flutterLoader.initialized()); | ||
| flutterLoader.startInitialization(RuntimeEnvironment.application); | ||
| flutterLoader.ensureInitializationComplete(RuntimeEnvironment.application, null); | ||
| shadowOf(getMainLooper()).idle(); | ||
| assertTrue(flutterLoader.initialized()); | ||
| FlutterInjector.reset(); | ||
| verify(mockFlutterJNILoader, times(1)).loadLibrary(); | ||
|
|
||
| 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); | ||
| verify(mockFlutterJNILoader, times(1)) | ||
| .nativeInit( | ||
| eq(RuntimeEnvironment.application), | ||
| any(), | ||
| anyString(), | ||
| anyString(), | ||
| anyString(), | ||
| anyLong(), | ||
| eq(activityManager.getLargeMemoryClass())); | ||
| } | ||
| } | ||
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