FlutterEngineGroup for Android #23675

xster · 2021-01-14T11:46:15Z

Description

The PR creates a second FlutterEngine creation path where resources are shared from the first FlutterEngine to reduce startup and memory cost.

It's similar to Implemented FlutterEngineGroup and Spawn API. #22975 on iOS except with the additions of Android specific items between the FlutterEngine and the C++ Shell, such as the Java FlutterJNI, the C++ platform_view_android_jni_impl, the C++ AndroidShellHolder
Changed Implemented FlutterEngineGroup and Spawn API. #22975 Shell::Spawn to take a new RunConfiguration instead of a new Settings. The second spawned Shell can't materially change much of the Settings from the first Shell except the RunConfiguration, so it's a more scoped parameter. We also can't count on re-inflating the RunConfiguration from the Settings in a platform agnostic way since Android needs to construct the RunConfiguration a specific way, so holding onto the Settings isn't useful.
Shares ThreadHost between FlutterEngine/Shells.
Shares the GPU context between Shells.
Moves the RunConfiguration creation logic from the jni_impl to the android_shell_holder. It was strange for a jni util class to have so much functionality. AndroidShellHolder being intentfully stateful can better remake new RunConfigurations with parts of the previous state.
Add a FlutterEngine destruction listener so FlutterEngineGroup can keep track.
FlutterJNI should ideally be 2 tiered. FlutterJNI itself should be entirely stateless and there should be a stateful wrapper to represent C++ Shell instances, and to supply callbacks to C++. For the time being, just created multiple (stateful) FlutterJNI instances. Some of the stateless function are unfortunately redundant. Added some warnings if called more than once.
FlutterEngineGroup spawns engines from existing ones if there's at least one engine already running.
Fixed an omission where the entrypoint function name is customizable but the entrypoint file path is not on Android.

Related Issues

Fixes flutter/flutter#72018
Fixes flutter/flutter#72528
Fixes flutter/flutter#72630
Fixes flutter/flutter#73599

Tests

I added the following tests:

FlutterEngineTest
FlutterEngineGroupComponentTest
Scenario test

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

xster

Added some read-along narrative. Hopefully helps with review a bit.

xster · 2021-01-15T18:25:25Z

shell/common/shell.cc


 std::unique_ptr<Shell> Shell::Spawn(
-    Settings settings,
+    RunConfiguration run_configuration,


This was changed from Settings to RunConfiguration since the spawning mechanism can't consume most of the Settings data anyway (since we're spawing off of a previous Shell which was already built using existing Settings data).

xster · 2021-01-15T18:25:59Z

shell/common/shell_unittests.cc

  ASSERT_TRUE(configuration.IsValid());
  configuration.SetEntrypoint("fixturesAreFunctionalMain");

+  auto second_configuration = RunConfiguration::InferFromSettings(settings);


Tweaked the test slightly to make sure it can get 2 different entrypoints.

xster · 2021-01-15T18:26:37Z

shell/platform/android/android_shell_holder.cc

+  static size_t thread_host_count = 1;
+  auto thread_label = std::to_string(thread_host_count++);

+  thread_host_ = std::make_shared<ThreadHost>();


Also shared Threads in this PR

xster · 2021-01-15T18:27:19Z

shell/platform/android/android_shell_holder.cc

+            shell,                   // delegate
+            shell.GetTaskRunners(),  // task runners
+            jni_facade,              // JNI interop
+            android_context          // Android context


Also shared the GPU context in this PR

xster · 2021-01-15T18:28:00Z

shell/platform/android/android_shell_holder.cc

  shell_->NotifyLowMemoryWarning();
 }
+
+std::optional<RunConfiguration> AndroidShellHolder::BuildRunConfiguration(


I moved this from the jni_impl to here since it's a lot of business logic and it would be nice to keep the jni as a simple pipe

xster · 2021-01-15T18:30:18Z

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


  // ----- End Engine FlutterTextUtils Methods ----

-  @Nullable private Long nativePlatformViewId;


This thing was a misnomer. The receiving side file is named platform view but is really a Shell (which owns the platform view). Renamed to reduce confusion.

xster · 2021-01-15T18:39:01Z

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

+      @Nullable String entrypointFunctionName, @Nullable String pathToEntrypointFunction) {
+    ensureRunningOnMainThread();
+    ensureAttachedToNative();
+    FlutterJNI spawnedJNI = new FlutterJNI();


This builds everything backwards. Instead of creating a partially built wrapper and filling in a corresponding native instance in there, this builds the native instance first, then builds everything in Java around it.

xster · 2021-01-15T18:39:57Z

shell/platform/android/io/flutter/embedding/engine/dart/DartExecutor.java

    Log.v(TAG, "Executing Dart entrypoint: " + dartEntrypoint);

    flutterJNI.runBundleAndSnapshotFromLibrary(
-        dartEntrypoint.pathToBundle, dartEntrypoint.dartEntrypointFunctionName, null, assetManager);


The null here was just a feature mysteriously and annoyingly missing on Android for some reason flutter/flutter#72630.

xster · 2021-01-15T18:40:51Z

shell/platform/android/platform_view_android_jni_impl.cc

      fml::jni::JavaStringToString(env, jBundlePath))  // apk asset dir
  );

-  std::unique_ptr<IsolateConfiguration> isolate_configuration;


this has to happen in 2 places now, so I moved it into the android shell holder as explained above.

xster · 2021-01-15T18:41:45Z

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

          threadHostType};
 }

-static void SetEntryPoint(flutter::Settings* settings, NSString* entrypoint, NSString* libraryURI) {


I removed this new indirection since we're no longer filling in the Settings in the Spawn to partially consume it anymore as explained above.

gaaclarke

Wow monster review, awesome progress. I glossed over the golden image test because my brain was starting to sizzle.

shell/platform/android/android_shell_holder.cc

gaaclarke · 2021-01-15T17:59:50Z

shell/platform/android/android_shell_holder.cc

+
+  // Pull out the new PlatformViewAndroid from the new Shell to feed to it to
+  // the new AndroidShellHolder.
+  fml::WeakPtr<PlatformViewAndroid> weak_platform_view;


What's keeping this alive that makes a weak pointer the right choice?

The shell (which we have a unique_ptr to a few lines down) owns a unique_ptr to a PlatformView which we then read out into this line to feed into the AndroidShellHolder which owns a unique_ptr to the Shell which owns this PlatformView.

Adding some more comments.

gaaclarke · 2021-01-15T18:02:25Z

shell/platform/android/android_shell_holder.cc

+  asset_manager_ = asset_manager;
+  auto config = BuildRunConfiguration(asset_manager, entrypoint, libraryUrl);
+  if (!config) {
+    return;


FML_LOG instead of failing silently?

It does on line 280 which is the only way to end up here.

shell/platform/android/android_shell_holder.h

gaaclarke · 2021-01-15T18:36:03Z

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

+    // Shell ID into the new FlutterJNI instance.
+    spawnedJNI.nativeShellId =
+        nativeSpawn(spawnedJNI, nativeShellId, entrypointFunctionName, pathToEntrypointFunction);
+    Preconditions.checkState(


This name is kind of silly, in the case it's a post-condition =)

Ya, the name is a bit silly. That's just what Java/Guava calls these things. cs/Preconditions.checkState

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

gaaclarke · 2021-01-15T18:40:24Z

shell/platform/android/io/flutter/util/Preconditions.java

+   * @param expression a boolean expression that must be checked to be true
+   * @throws IllegalStateException if {@code expression} is false
+   */
+  public static void checkState(boolean expression) {


s/checkState/checkIsTrue

ditto, cs/Preconditions.checkState

gaaclarke · 2021-01-15T18:42:33Z

shell/platform/android/platform_view_android_jni_impl.cc

+  if (spawned_shell_holder->IsValid()) {
+    return reinterpret_cast<jlong>(spawned_shell_holder.release());
+  } else {
+    return 0;


Would it be more appropriate to throw a java exception?

I think so, but it's it should be consistent with the mainline path nativeAttach. It's a pretty big breaking change to make it conform there. Instead, the current pattern is defensive coding on the Java side. Doing that as well here.

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

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

jason-simmons · 2021-01-15T18:20:56Z

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

+  public FlutterEngine createAndRunEngine(@Nullable DartEntrypoint dartEntrypoint) {
+    FlutterEngine engine = null;
+    if (activeEngines.size() == 0) {
+      engine = createEngine(context);


Any reason for doing this here instead of merging it into the block below that calls executeDartEntrypoint?

oh ya, a good place to add a comment. The engine needs to be created first to call the FlutterLoader if needed. This lets the default DartEntrypoint creator know how to find the assets.

jason-simmons · 2021-01-15T18:28:43Z

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

      @Nullable String[] dartVmArgs,
      boolean automaticallyRegisterPlugins,
      boolean waitForRestorationData) {
+    this.context = context;


Is it necessary to hold a reference to the context instead of having callers pass the context to spawn()?

Holding the context here creates a risk that a FlutterEngine may keep the context (such as an Activity) alive longer than intended, resulting in resource leaks.

Ya that's a really good point. On the one hand, if we keep it the same:

Theoretically, the users should pass the application context to make this point moot, but it's basically unenforceable.

It's very hard to audit all the places this Context this ends up at. And if we create 2 engines that share a whole bunch of things but not the same Context, it's not clear yet if there might be side effects where components built with 2 different Context might interact in unpredictable ways.

On the other hand:

Risking leaking an Activity after detaching an engine is severe.

Let me look at this again. I'm leaning towards doing it your way.

I ended removing the Context ivar from the engine and the engine group. Thanks for pointing out!

jason-simmons · 2021-01-15T18:40:41Z

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

+   * <p>This method should only be called once across all FlutterJNI instances.
   */
  public void loadLibrary() {
+    if (loadLibraryCalled) {


The flag is not necessary here - System.loadLibrary is safe to call twice for the same library.

(These methods are effectively static - IIRC they were declared as instance methods just so they could be mocked in tests)

Ya, though herein lies the cognitive ambiguity since it's not clear what methods the user is expected to call again on a second JNI and which ones are pointless no-ops or redundant. This doesn't really do anything except add docs and messages to point out that certain "static" functions don't need to be called again on a new JNI.

jason-simmons · 2021-01-15T18:44:09Z

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

      @NonNull String engineCachesPath,
      long initTimeMillis) {
+    if (initCalled) {
+      Log.w(TAG, "FlutterJNI.init called more than once");


In this case, nativeInit is not safe to call more than once. This should throw an exception if called again.

Changed log to exception

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

jason-simmons · 2021-01-15T18:59:57Z

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

+  // Below represents the stateful part of the FlutterJNI instances that aren't static per program.
+  // Conceptually, it represents a native shell instance.
+
+  @Nullable private Long nativeShellId;


I'd vote for renaming this to nativeShellHolderId.

This is a pointer to an AndroidShellHolder, which in turn holds references to the Shell and the PlatformView.

Sure. The Shell actually holds the real unique_ptr to the PlatformView and the AndroidShellHolder holds the unique_ptr to the Shell, so it's 1 layer of indirection. But I'm all for being as technically exact as possible.

jason-simmons · 2021-01-15T19:05:17Z

shell/platform/android/platform_view_android_jni_impl.cc

+// AOT.
+static jlong SpawnJNI(JNIEnv* env,
+                      jobject jcaller,
+                      jobject flutterJNI,


Rename this to spawnedJNI. The flutterJNI name is a bit confusing because jcaller is also an instance of FlutterJNI.

Alternatively this method could use JNI APIs to construct the spawned FlutterJNI instance.

I'm gonna call it newFlutterJNI. It's not technically "spawned" since nothing spawned it from an existing "pool". The Java side just constructed a new one and gave it to C++ for a callback. Adding some code comments too.

Alternatively this method could use JNI APIs to construct the spawned FlutterJNI instance.

I didn't understand this comment

Currently nativeSpawn takes an argument containing an empty FlutterJNI representing the newly spawned engine. nativeSpawn constructs the peer AndroidShellHolder and returns a pointer to it, which the Java side then stores in nativeShellHolderId.

The nativeSpawn C++ implementation could instead construct the new FlutterJNI instance, fill in its nativeShellHolderId, and return it. That might make for a cleaner interface. But it also means more complex JNI code. So given that this is a private interface, it's probably more trouble than it's worth.

You mean do something like https://stackoverflow.com/questions/10130819/use-jni-to-create-populate-and-return-a-java-class-instance right? Takes a bit of gymnastics but it is semantically clearer. Lower prio but it's interesting, I'll try it. The FlutterJNI construction signature is unlikely to change at this point too so we'll probably not expose Java refactors into entangled C++ changes.

man that was really verbose, since the Java Long needs to be instantiated separately. But done now.

shell/platform/android/android_shell_holder.h

xster · 2021-01-15T22:06:41Z

Thanks for your quick thorough review!

xster · 2021-01-15T22:41:36Z

I'll try to run all the devicelab tests locally before submitting

xster · 2021-01-17T10:33:22Z

Ran all the Android devicelab tests locally. We should be ok.

…y linux can match. Try to pull it from CI

gaaclarke

Okay, I think this is good, LGTM. It's pretty big. I just had a few nits and comments, nothing blocking.

gaaclarke · 2021-01-19T17:33:16Z

testing/scenario_app/lib/src/bogus_font_text.dart

-
-  @override
-  void onDrawFrame() {
-    // Just draw once since the content never changes.
-  }


Was this removal intentional? Since it doesn't call the super implementation this empty method might be on purpose.

It is intentional (since the super syncs a second frame then signals to take a screenshot). The no-op here was somewhat intentional too (just to write some comments on it). I don't think it'll do anything on iOS. It'll send a 'take_screenshot' channel message that'll be dropped on the floor.

gaaclarke · 2021-01-19T17:38:28Z

shell/platform/android/platform_view_android_jni_impl.cc

+  }
+
+  jfieldID shellHolderId =
+      env->GetFieldID(jniClass, "nativeShellHolderId", "Ljava/lang/Long;");


Not a huge deal, but this would be faster if we cached the FieldIds and MethodIds. Maybe it can be a follow up PR.

aha, good point. Done

gaaclarke · 2021-01-19T17:39:31Z

shell/platform/android/platform_view_android.h

+  ///
+  PlatformViewAndroid(PlatformView::Delegate& delegate,
+                      flutter::TaskRunners task_runners,
+                      std::shared_ptr<PlatformViewAndroidJNI> jni_facade,


nit: Should pass these by const reference. The copy constructor for std::shared_ptr is pretty heavy, it includes mutex locking and stuff.

done here and in android_shell_holder

gaaclarke · 2021-01-19T17:47:03Z

shell/platform/android/platform_view_android.cc


 PlatformViewAndroid::~PlatformViewAndroid() = default;

+void PlatformViewAndroid::InitSurface() {


nit: This is kind of a weak abstraction. It doesn't just init the surface and obfuscates its side effects =) This would be slightly better:

surface_factory_ = MakeSurfaceFactory(android_context_); android_surface_ = MakeSurface(surface_factory_);

The benefit being that the side effects are visible at the call site; the dependencies are explicit as input parameters, which also reduces the powers of the abstraction; the abstraction names more closely match what they actually do.

Just another point, i've often seen people use void(void) methods for initialization and they often end up creating temporal bugs (where one method assumes an instance variable has already been initialized) which can't happen with pure functions.

dnfield · 2021-07-09T04:41:31Z

testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/TestActivity.java

+    MethodChannel channel =
+        new MethodChannel(getFlutterEngine().getDartExecutor(), "driver", JSONMethodCodec.INSTANCE);
+    Map<String, Object> test = new HashMap<>(2);
+    test.put("name", launchIntent.getStringExtra("scenario"));


This comes through as an integer, rather than a string, and causes the test to fail on firebase unfortunately - although we're not picking up the failure :\

Hm - I can see this was just moved from other code - I guess FTL changed or this code just never worked.

flutter-dashboard bot added platform-android platform-ios labels Jan 14, 2021

googlebot added the cla: yes label Jan 14, 2021

xster force-pushed the flutter-engine-group branch 4 times, most recently from 956f086 to a207e91 Compare January 15, 2021 08:54

xster marked this pull request as ready for review January 15, 2021 09:10

xster requested review from gaaclarke and jason-simmons January 15, 2021 09:10

xster commented Jan 15, 2021

View reviewed changes

gaaclarke reviewed Jan 15, 2021

View reviewed changes

jason-simmons reviewed Jan 15, 2021

View reviewed changes

xster force-pushed the flutter-engine-group branch from 416326e to 0c734d7 Compare January 15, 2021 22:02

xster requested review from gaaclarke and jason-simmons January 15, 2021 22:04

This comment has been minimized.

Sign in to view

xster added 10 commits January 19, 2021 00:39

half of the plumbing

64a1778

piped through, start manual testing

37acd45

autoformat

78dd4d4

patched Android together

3241679

Simplified shell spawn down to just take run configuration

3a0d864

a round of comments and docs

e2b91ff

add tests

9767a6a

autoformat

08654a5

fix

bb201cf

reviews

25520ad

xster added 6 commits January 19, 2021 00:39

fix licenses + forgot to document some methods on FlutterEngineGroup

1b4b7c6

forgot to refactor scenario test

4ec9cdd

revert the nativeInit crash for now since our tests rely on it

02fbbc6

having trouble getting the golden images to look right, I suspect onl…

4dc1f9b

…y linux can match. Try to pull it from CI

fix golden size

94fe64e

refactor jni

f402440

xster force-pushed the flutter-engine-group branch from 4326bcc to f402440 Compare January 19, 2021 09:34

local and ci autoformats fighting each other

01d1057

gaaclarke approved these changes Jan 19, 2021

View reviewed changes

xster added 2 commits January 19, 2021 11:14

review

448bdd6

autoformat fighting each other again

a591f5f

xster 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 19, 2021

fluttergithubbot merged commit 915e2ca into flutter:master Jan 19, 2021

xster deleted the flutter-engine-group branch January 19, 2021 20:32

engine-flutter-autoroll mentioned this pull request Jan 19, 2021

Roll Engine from 609036f2bf7c to 915e2ca178e5 (13 revisions) flutter/flutter#74266

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 19, 2021

915e2ca FlutterEngineGroup for Android (flutter/engine#23675)

b417cf9

gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Jan 20, 2021

FlutterEngineGroup for Android (flutter#23675)

7dd40b0

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

FlutterEngineGroup for Android (flutter#23675)

2dd78d6

dnfield reviewed Jul 9, 2021

View reviewed changes


		// ----- End Engine FlutterTextUtils Methods ----

		@Nullable private Long nativePlatformViewId;


		PlatformViewAndroid::~PlatformViewAndroid() = default;

		void PlatformViewAndroid::InitSurface() {

FlutterEngineGroup for Android #23675

FlutterEngineGroup for Android #23675

Uh oh!

Conversation

xster commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

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!

gaaclarke left a comment

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

xster commented Jan 14, 2021 •

edited

Loading