Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions shell/common/switches.cc
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,12 @@ Settings SettingsFromCommandLine(const fml::CommandLine& command_line) {
settings.purge_persistent_cache =
command_line.HasOption(FlagForSwitch(Switch::PurgePersistentCache));

if (command_line.HasOption(FlagForSwitch(Switch::OldGenHeapSize))) {
std::string old_gen_heap_size;
command_line.GetOptionValue(FlagForSwitch(Switch::OldGenHeapSize),
&old_gen_heap_size);
settings.old_gen_heap_size = std::stoi(old_gen_heap_size);
}
return settings;
}

Expand Down
3 changes: 3 additions & 0 deletions shell/common/switches.h
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,9 @@ DEF_SWITCH(
"Uses separate threads for the platform, UI, GPU and IO task runners. "
"By default, a single thread is used for all task runners. Only available "
"in the flutter_tester.")
DEF_SWITCH(OldGenHeapSize,
"old-gen-heap-size",
"The size limit in megabytes for the Dart VM old gen heap space.")

DEF_SWITCHES_END

Expand Down
30 changes: 2 additions & 28 deletions shell/platform/android/io/flutter/FlutterInjector.java
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Member

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

return shouldLoadNative;
}

/** Returns the {@link FlutterLoader} instance to use for the Flutter Android engine embedding. */
@NonNull
public FlutterLoader flutterLoader() {
Expand All @@ -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.
Expand All @@ -130,8 +105,7 @@ private void fillDefaults() {
public FlutterInjector build() {
fillDefaults();

System.out.println("should load native is " + shouldLoadNative);
Copy link
Member

Choose a reason for hiding this comment

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

🤦‍♀️ thanks

return new FlutterInjector(shouldLoadNative, flutterLoader);
return new FlutterInjector(flutterLoader);
}
}
}
54 changes: 48 additions & 6 deletions shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,50 @@
public class FlutterJNI {
private static final String TAG = "FlutterJNI";

// BEGIN Methods related to loading for FlutterLoader.
/**
* Loads the libflutter.so C++ 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.
*/
public void init(
@NonNull Context context,
@NonNull String[] args,
@Nullable String bundlePath,
@NonNull String appStoragePath,
@NonNull String engineCachesPath,
long initTimeMillis) {
FlutterJNI.nativeInit(
context, args, bundlePath, appStoragePath, engineCachesPath, initTimeMillis);
}
// END methods related to FlutterLoader

@Nullable private static AsyncWaitForVsyncDelegate asyncWaitForVsyncDelegate;
// This should also be updated by FlutterView when it is attached to a Display.
// The initial value of 0.0 indicates unknown refresh rate.
Expand All @@ -105,7 +149,8 @@ public class FlutterJNI {
// This is set from native code via JNI.
@Nullable private static String observatoryUri;

// TODO(mattcarroll): add javadocs
/** @deprecated Use {@link #init(Context, String[], String, String, String, long)} instead. */
@Deprecated
public static native void nativeInit(
@NonNull Context context,
@NonNull String[] args,
Expand All @@ -114,11 +159,8 @@ public static native void nativeInit(
@NonNull String engineCachesPath,
long initTimeMillis);

/**
* 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.
*/
/** @deprecated Use {@link #prefetchDefaultFontManager()} instead. */
@Deprecated
public static native void nativePrefetchDefaultFontManager();

private native boolean nativeGetIsSoftwareRenderingEnabled();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,19 @@

package io.flutter.embedding.engine.loader;

import android.app.ActivityManager;
import android.content.Context;
import android.content.pm.ApplicationInfo;
import android.content.pm.PackageManager;
import android.content.res.AssetManager;
import android.os.Bundle;
import android.os.Handler;
import android.os.Looper;
import android.os.SystemClock;
import android.view.WindowManager;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import io.flutter.BuildConfig;
import io.flutter.FlutterInjector;
import io.flutter.Log;
import io.flutter.embedding.engine.FlutterJNI;
import io.flutter.util.PathUtils;
Expand All @@ -29,6 +31,9 @@
public class FlutterLoader {
private static final String TAG = "FlutterLoader";

private static final String OLD_GEN_HEAP_SIZE_META_DATA_KEY =
"io.flutter.embedding.android.OldGenHeapSize";

// Must match values in flutter::switches
static final String AOT_SHARED_LIBRARY_NAME = "aot-shared-library-name";
static final String SNAPSHOT_ASSET_PATH_KEY = "snapshot-asset-path";
Expand Down Expand Up @@ -60,10 +65,26 @@ public static FlutterLoader getInstance() {
return instance;
}

/** Creates a {@code FlutterLoader} that uses a default constructed {@link FlutterJNI}. */
public FlutterLoader() {
Copy link
Member

Choose a reason for hiding this comment

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

Docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

this(new FlutterJNI());
Copy link
Member

Choose a reason for hiding this comment

The 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;
Expand Down Expand Up @@ -125,9 +146,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.
Expand All @@ -136,7 +155,7 @@ public InitResult call() {
new Runnable() {
@Override
public void run() {
FlutterJNI.nativePrefetchDefaultFontManager();
flutterJNI.prefetchDefaultFontManager();
}
});

Expand Down Expand Up @@ -225,17 +244,34 @@ public void ensureInitializationComplete(
shellArgs.add("--log-tag=" + settings.getLogTag());
}

ApplicationInfo applicationInfo =
applicationContext
.getPackageManager()
.getApplicationInfo(
applicationContext.getPackageName(), PackageManager.GET_META_DATA);
Bundle metaData = applicationInfo.metaData;
int oldGenHeapSizeMegaBytes =
metaData != null ? metaData.getInt(OLD_GEN_HEAP_SIZE_META_DATA_KEY) : 0;
if (oldGenHeapSizeMegaBytes == 0) {
// default to half of total memory.
ActivityManager activityManager =
(ActivityManager) applicationContext.getSystemService(Context.ACTIVITY_SERVICE);
ActivityManager.MemoryInfo memInfo = new ActivityManager.MemoryInfo();
activityManager.getMemoryInfo(memInfo);
oldGenHeapSizeMegaBytes = (int) (memInfo.totalMem / 1e6 / 2);
}

shellArgs.add("--old-gen-heap-size=" + oldGenHeapSizeMegaBytes);
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;

import io.flutter.embedding.engine.loader.FlutterLoader;
import org.junit.Before;
Expand Down Expand Up @@ -35,7 +34,6 @@ public void itHasSomeReasonableDefaults() {
// Implicitly builds when first accessed.
FlutterInjector injector = FlutterInjector.instance();
assertNotNull(injector.flutterLoader());
assertTrue(injector.shouldLoadNative());
}

@Test
Expand All @@ -44,7 +42,6 @@ public void canPartiallyOverride() {
new FlutterInjector.Builder().setFlutterLoader(mockFlutterLoader).build());
FlutterInjector injector = FlutterInjector.instance();
assertEquals(injector.flutterLoader(), mockFlutterLoader);
assertTrue(injector.shouldLoadNative());
}

@Test()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 =
Expand Down
Loading