Android embedding refactor pr40 add static engine cache #10481

matthew-carroll · 2019-08-02T23:40:47Z

Android embedding refactor pr40 add static engine cache.

This PR includes testing dependency updates. Those updates have already been uploaded to CIPD and tagged, then re-synced locally to verify.

This PR changes the canonical way to use FlutterActivity and FlutterFragment to the following:

Create a default FlutterActivity:
Intent intent = FlutterActivity.createDefaultIntent();

Create a custom FlutterActivity with new FlutterEngine:
Intent intent = FlutterActivity.withNewEngine().dartEntrypoint("myEntry")...build();

Create a custom FlutterActivity with a cached FlutterEngine:
Intent intent = FlutterActivity.withCachedEngine("my_engine_id")...build();

Create a default FlutterFragment:
FlutterFragment fragment = FlutterFragment.createDefault();

Create a custom FlutterFragment with a new FlutterEngine:
FlutterFragment fragment = FlutterFragment.withNewEngine().dartEntrypoint("myEntry")...build();

Create a custom FlutterFragment with a cached FlutterEngine:
FlutterFragment fragment = FlutterFragment.withCachedEngine("my_engine_id")...build();

To cache a FlutterEngine:

FlutterEngine engine = new FlutterEngine(context);
// steps to begin executing Dart are omitted.
FlutterEngineCache.getInstance().put("my_engine_id", engine);

…ngine's. (flutter#32870)

mehmetf · 2019-08-03T16:15:32Z

shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java

+    if (cachedEngineId != null) {
+      flutterEngine = FlutterEngineCache.getInstance().get(cachedEngineId);
+      isFlutterEngineFromHost = true;
+      if (flutterEngine == null) {


This is almost like an assert() to me. I do agree that we should validate that a cached engine exists but only in debug mode if it can be detected here. In release mode, perhaps we create an engine?

I considered that. The thing is, we'd be assuming the desired app bundle path, dart entrypoint, and initial route. Maybe we guess right, maybe not. If we get to this point and no cached engine exists, we have a fairly serious logic error. Now, if it's possible for this to happen due some kind of timing issue then we'd want to provide a better resolution, but if the dev simply forgot to put an engine in the cache then I don't think we can continue execution.

@mehmetf @xster any further thoughts here? I'm open to alternatives if you think we should do something else. I just don't think we can assume a particular engine configuration to auto-create one.

If we have to guess all of that then yes, I agree with you. We should fail.

mehmetf · 2019-08-03T16:16:35Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

 * </ul>
+ * <p>
+ * {@code FlutterActivity} can be used with a cached {@link FlutterEngine} instead of creating a new
+ * one. Use {@link #withCachedEngine(String)} to build a {@code FlutterActivity} {@code Intent} that


Should this be accompanied with some documentation change on when to use a cached engine?

Is there some existing documentation that you think should be updated?

I am not sure where the upstream user documentation for the new embedding is. Internally, it would be as part of the FAQ.

FlutterActivity can be used with a cached FlutterEngine.

but why and when should I use it with a cached Flutter Engine?

@mehmetf @xster the only 2 times that I can think of that you wouldn't want to warm up an engine is:

FlutterActivity is the first Activity that is launched, therefore rendering warm up redundant, or

You have no idea when/if you'll need a Flutter experience and you don't want to waste the resources.

Can either of you think of other cases?

I think that's right. We can probably phrase the doc to say using a cached engine is generally recommended for optimal launch latency except where impossible such as in the cases listed.

Updated docs to reflect this.

xster

awesome PR!!!
LGTM

xster · 2019-08-06T01:07:44Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

-   * Creates an {@link IntentBuilder}, which can be used to configure an {@link Intent} to
-   * launch a {@code FlutterActivity}.
+   * Creates an {@link NewEngineIntentBuilder}, which can be used to configure an {@link Intent} to
+   * launch a {@code FlutterActivity} that internally creates a new {@link FlutterEngine} using


Thanks for revisiting all these docs and clarifying.

xster · 2019-08-06T01:10:43Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

+   * Creates a {@link CachedEngineIntentBuilder}, which can be used to configure an {@link Intent}
+   * to launch a {@code FlutterActivity} that internally uses an existing {@link FlutterEngine} that
+   * is cached in {@link FlutterEngineCache}.
+   */


Can we add a snippet showing how one caches a FlutterEngine in a FlutterEngineCache?

Sure, done.

xster · 2019-08-06T01:11:28Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

+    }
+
+    /**
+     * Returns true if the cached {@link FlutterEngine} should be destroyed and remoed from the


xster · 2019-08-06T03:07:42Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

+  @Override
+  @Nullable
+  public String getCachedEngineId() {
+    return getIntent().getStringExtra(EXTRA_CACHED_ENGINE_ID);


I forget how this works. If you made an activity with an Intent and rotate the screen, it's the same intent instance?

I don't know about the Intent instance, but the Intent should report the same extras.

xster · 2019-08-06T03:08:58Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

+   */
+  @Override
+  public boolean shouldDestroyEngineWithHost() {
+    return getIntent().getBooleanExtra(EXTRA_DESTROY_ENGINE_WITH_ACTIVITY, false);


Describe the default either in this doc or in the builder class or the builder factory.

Done. I also updated FlutterActivity and FlutterFragment to set this value to true whenever the FlutterActivity/Fragment create its own FlutterEngine, or one is explicitly provided by subclassing. Do you think that's OK?

xster · 2019-08-06T03:10:54Z

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

+import java.util.Map;
+
+/**
+ * Static singleton cache that holds {@link FlutterEngine} instances identified by {@code String}s.


Describe somewhere how that String relates to the engine instance. i.e. how do you get that string from an engine etc.

xster · 2019-08-06T03:12:39Z

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

+   * If a {@link FlutterEngine} already exists in this cache for the given {@code engineId}, that
+   * {@link FlutterEngine} is removed from this cache.
+   */
+  public void put(@NonNull String engineId, @Nullable FlutterEngine engine) {


oh it's full on whatever you make it? I guess that works too :)

xster · 2019-08-06T03:25:00Z

...atform/android/test/io/flutter/embedding/android/FlutterActivityAndFragmentDelegateTest.java

-    // Create a spy around the FakeHost so that we can verify method invocations.
-    spyHost = spy(fakeHost);
+    // Create a mocked Host, which is required by the delegate being tested.
+    fakeHost = mock(FlutterActivityAndFragmentDelegate.Host.class);


uber uber nit: you can call it mockHost at this point :D

xster · 2019-08-06T03:27:43Z

shell/platform/android/test/io/flutter/embedding/android/FlutterActivityTest.java

+  }
+
+  @Test
+  public void itCreatesCachedEngineIntentThatDoesNotDestroyTheEngine() {


…ter/engine#10481)

git@github.com:flutter/engine.git/compare/42917704790d...9bfa4f5 git log 4291770..9bfa4f5 --no-merges --oneline 2019-08-07 stuartmorgan@google.com Roll buildroot back to an earlier version (flutter/engine#10681) 2019-08-07 stuartmorgan@google.com Roll buildroot to pick up EGL library name fix (flutter/engine#10679) 2019-08-06 chinmaygarde@google.com Remove semi-redundant try-jobs. (flutter/engine#10485) 2019-08-06 chinmaygarde@google.com Allow embedders to control Dart VM lifecycle on engine shutdown. (flutter/engine#10652) 2019-08-06 matthew-carroll@users.noreply.github.com Android embedding refactor pr40 add static engine cache (flutter/engine#10481) 2019-08-06 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from W6ZtV... to 9sR9P... (flutter/engine#10672) 2019-08-06 bkonyi@google.com Roll src/third_party/dart 574c4a51c6..c262cbd414 (11 commits) 2019-08-06 stuartmorgan@google.com Roll buildroot for ANGLE support (flutter/engine#10667) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (jimgraham@google.com), and stop the roller if necessary.

matthew-carroll added 2 commits July 31, 2019 15:49

WIP

2fbacc6

Android Embedding Refactor PR 40: Introduce static cache for FlutterE…

a1e82ed

…ngine's. (flutter#32870)

googlebot added cla: yes labels Aug 2, 2019

Remove superfluous getLifecycle() method in FlutterFragment.

829ed51

matthew-carroll requested review from mehmetf, mklim and xster August 2, 2019 23:45

License updates.

0e5e3a7

mehmetf reviewed Aug 3, 2019

View reviewed changes

xster reviewed Aug 6, 2019

View reviewed changes

matthew-carroll added 2 commits August 6, 2019 14:57

Formatted BUILD.gn

9f5affc

PR Updates.

8aa17fb

matthew-carroll merged commit 735255f into flutter:master Aug 6, 2019

matthew-carroll mentioned this pull request Aug 6, 2019

Make it convenient to create a FlutterActivity with an existing engine flutter/flutter#32870

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 7, 2019

735255f Android embedding refactor pr40 add static engine cache (flut…

6ce2ed9

…ter/engine#10481)

engine-flutter-autoroll mentioned this pull request Aug 7, 2019

Roll engine 42917704790d..9bfa4f53cdbe (8 commits) flutter/flutter#37755

Merged

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Aug 16, 2019

Android embedding refactor pr40 add static engine cache (flutter#10481)

1b894a8

Android embedding refactor pr40 add static engine cache #10481

Android embedding refactor pr40 add static engine cache #10481

Uh oh!

Conversation

matthew-carroll commented Aug 2, 2019

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants