Add traces to Android embedding #29230

jiahaog · 2021-10-18T11:48:18Z

flutter/flutter#91280, b/201473640

These traces will show up in systrace / Perfetto, making performance work easier.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

jiahaog · 2021-10-18T13:37:10Z

Is flutter/android/embedding_bundle or something supposed to be updated here to fix the build failures? Would appreciate some help here :)

dnfield · 2021-10-18T17:28:23Z

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

@@ -9,6 +9,7 @@
 import android.content.res.AssetManager;
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
+import androidx.tracing.Trace;


Does this require a new jar from AndroidX?

If so, can we just use android.os.Tracing?

The AndroidX wrapper abstracts the differences in support for the tracing APIs across various versions of Android.

It might be worthwhile to regenerate the android_embedding_dependencies package. Many of the libraries in there can probably be removed now that Robolectric dependencies are being obtained dynamically by the Gradle-based test runner.

PR for the android_embedding_dependencies update: #29233

As Jason mentioned, it's a small abstraction over nuances like SDK versions. We're probably going to need to roll our own implementations anyway if we don't use it, and it seems like androidx APIs are pretty much aligned with the rest of the Android ecosystem. Lmk if there are strong objections to this though.

dnfield · 2021-10-18T17:28:44Z

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

@@ -356,11 +357,18 @@ public FlutterEngine(
  }

  private void attachToJni() {
+    Trace.beginSection("FlutterEngine attachToJni");


I think this one needs to be an async section.

also nit: FlutterEngine#attachToJni

@dnfield can you point me to where this async section should end? (Also for destroy)

Removed, since it isn't useful for us at this point anyway

dnfield · 2021-10-18T17:29:01Z

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

@@ -411,20 +419,26 @@ private boolean isAttachedToJni() {
   * <p>This {@code FlutterEngine} instance should be discarded after invoking this method.
   */
  public void destroy() {
+    Trace.beginSection("FlutterEngine destroy");


Removed, since it isn't useful for us at this point anyway

dnfield · 2021-10-18T17:30:36Z

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

@@ -83,7 +84,8 @@ public DartExecutor(@NonNull FlutterJNI flutterJNI, @NonNull AssetManager assetM
  public void onAttachedToJNI() {
    Log.v(


nit: consider removing these in favor of trace statements.

Did not do this since the above onAttachedToJNI was also removed and we don't really need it

dnfield · 2021-10-18T17:31:19Z

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

@@ -250,7 +263,12 @@ public void setIsolateServiceIdListener(@Nullable IsolateServiceIdListener liste
   */
  public void notifyLowMemoryWarning() {
    if (flutterJNI.isAttached()) {
-      flutterJNI.notifyLowMemoryWarning();
+      Trace.beginSection("DartExecutor notifyLowMemoryWarning");


There's already some tracing for this in the engine and in Dart. I don't think we need it here - we'd effectively just be tracing the JNI overhead, which is small.

Removed, thanks

dnfield · 2021-10-18T17:31:49Z

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

@@ -147,15 +155,20 @@ public void executeDartCallback(@NonNull DartCallback dartCallback) {
      return;
    }

+    Trace.beginSection("DartExecutor executeDartCallback");


I suspect this would want to be an async one, but I also think we have a bunch of tracing around this already in the C++ side. Is this adding much value?

This one might be still useful for diagnostics since different apps can call this at different points (depending on whether they use FlutterActivity / roll their own)

dnfield · 2021-10-18T17:32:47Z

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

@@ -60,15 +61,21 @@ public void send(
      @NonNull String channel,
      @Nullable ByteBuffer message,
      @Nullable BinaryMessenger.BinaryReply callback) {
+    Trace.beginSection("DartMessenger send on " + channel);


I think this might be better to do on the Dart side in the Flutter framework. It also might want to be async to see if we could capture the reply as well.

I'm for the spirit of these 2, but adding this here might get spammy and possibly affect runtime performance. Can we try this locally on GPay and see how much traffic this adds (and possibly run an ab test)?

Yes. async traces will be helpful here as well. This one seems to be useful though - it tells us when the main thread is blocked by a plugin callback. At the same time, having both this one and also traces on the Dart side allows us to observe how long the platform message is queued before it is able to make the thread jump?

That makes sense to me.

dnfield · 2021-10-18T17:33:22Z

shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java

@@ -202,6 +215,8 @@ public void ensureInitializationComplete(
      throw new IllegalStateException(
          "ensureInitializationComplete must be called after startInitialization");
    }
+    Trace.beginSection("FlutterLoader ensureInitializationComplete");


Async section.

Correct me if I'm wrong, but this piece doesn't look like it's async and the method blocks until it's done

dnfield · 2021-10-18T17:33:45Z

tools/cipd/android_embedding_bundle/build.gradle

@@ -42,6 +42,7 @@ android {
  dependencies {
    embedding "androidx.annotation:annotation:1.1.0"
    embedding "androidx.fragment:fragment:1.1.0"
+    embedding "androidx.tracing:tracing:1.0.0"


Let's avoid this if we can - what does it add over android.os.trace?

Discussed above :)

xster · 2021-10-18T21:29:17Z

tools/androidx/files.json

@@ -49,5 +49,13 @@
      "androidx.annotation.UiThread",
      "androidx.annotation.VisibleForTesting"
    ]
+  },
+  {
+    "url": "https://maven.google.com/androidx/tracing/tracing/1.0.0/tracing-1.0.0.aar",


I think it's ok but for posterity, this is adding 4kB to all Flutter users

Commented below a bit more - if you're referring to the .aar file, it is a 4KB build dependency, but much less when built into an APK

Is the change to this file necessary though? I removed it but it seemed to build correctly

xster · 2021-10-18T21:30:44Z

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

@@ -60,15 +61,21 @@ public void send(
      @NonNull String channel,
      @Nullable ByteBuffer message,
      @Nullable BinaryMessenger.BinaryReply callback) {
+    Trace.beginSection("DartMessenger send on " + channel);


I'm for the spirit of these 2, but adding this here might get spammy and possibly affect runtime performance. Can we try this locally on GPay and see how much traffic this adds (and possibly run an ab test)?

blasten · 2021-10-18T21:49:51Z

Since this is adding 4KB, are all users going to use this feature? If not, we may be able to use R8 to fully remove this dependency from builds that don't need it. https://developer.android.com/studio/build/shrink-code#optimization

jiahaog · 2021-10-19T00:10:56Z

If not, we may be able to use R8 to fully remove this dependency from builds that don't need it

The tracing calls have to be built into the app since this is only configurable at runtime, but some methods we don't call should be tree shaken

jiahaog · 2021-10-19T00:16:32Z

https://maven.google.com/androidx/tracing/tracing/1.0.0/tracing-1.0.0.aar will add 4KB to Flutter users at build time, but much less of that 4KB would probably be built into the APK (will do some builds and update). As seen from the source, it is really a thin wrapper around android.os.Trace that I'd expect we'd need to write ourselves if we use android.os.Trace directly.

blasten · 2021-10-19T04:13:30Z

The tracing calls have to be built into the app since this is only configurable at runtime, but some methods we don't call should be tree shaken

R8 is able to tree shake code that isn't reachable too, so if (EVALUATES_TO_FALSE) { callLibrary() } should be able to delete callLibrary if this isn't necessary for all users, but no need to add it if the impact is tiny.

jiahaog · 2021-10-19T08:55:16Z

It's a ~1KB difference in the APK size, including all the changes from this PR. I think this should be small enough and it wouldn't make a difference whether we use androidx.tracing.Trace vs rolling our own wrappers around android.os.Trace to deal with versioning.

$ ls -l
total 20904
-rw-r--r--  1 jiahaog  primarygroup  5348729 Oct 19 16:47 base.apk
-rw-r--r--  1 jiahaog  primarygroup  5349767 Oct 19 16:37 expt.apk

jiahaog · 2021-10-21T12:43:34Z

Ran an A/B test internally and didn't see any significant changes to the benchmarks beyond the usual noise.

This reverts commit 8d7d4e7. Manually resolved conflict in tools/androidx/files.json

flutter-dashboard bot added the platform-android label Oct 18, 2021

google-cla bot added the cla: yes label Oct 18, 2021

jiahaog force-pushed the trace branch 2 times, most recently from 78abb17 to a3b6a41 Compare October 18, 2021 12:08

Add traces to Android embedding

27eec8f

jiahaog force-pushed the trace branch from a3b6a41 to 27eec8f Compare October 18, 2021 12:15

jiahaog requested review from xster, dnfield and blasten October 18, 2021 13:31

dnfield reviewed Oct 18, 2021

View reviewed changes

xster reviewed Oct 18, 2021

View reviewed changes

blasten mentioned this pull request Oct 18, 2021

Remove Robolectric and add androidx.tracing to the android_embedding_dependencies package #29233

Merged

jiahaog added 3 commits October 21, 2021 15:57

Merge branch 'master' into trace

1839087

Revert some metrics we don't need

c1c0a6e

Use #

0f4496a

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

8d7d4e7 Add traces to Android embedding (flutter/engine#29230)

f1cd40c

engine-flutter-autoroll mentioned this pull request Oct 30, 2021

Roll Engine from 3a0a63c2d209 to 47446ca043c2 (24 revisions) flutter/flutter#92765

Closed

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

8d7d4e7 Add traces to Android embedding (flutter/engine#29230)

54a18f3

engine-flutter-autoroll mentioned this pull request Oct 30, 2021

Roll Engine from 3a0a63c2d209 to 47446ca043c2 (24 revisions) flutter/flutter#92771

Closed

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

8d7d4e7 Add traces to Android embedding (flutter/engine#29230)

d0eef36

engine-flutter-autoroll mentioned this pull request Oct 30, 2021

Roll Engine from 3a0a63c2d209 to 5597ef01f5d2 (25 revisions) flutter/flutter#92772

Closed

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

8d7d4e7 Add traces to Android embedding (flutter/engine#29230)

a7a9e2c

engine-flutter-autoroll mentioned this pull request Oct 31, 2021

Roll Engine from 3a0a63c2d209 to 5597ef01f5d2 (25 revisions) flutter/flutter#92775

Closed

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

8d7d4e7 Add traces to Android embedding (flutter/engine#29230)

b901a8b

engine-flutter-autoroll mentioned this pull request Oct 31, 2021

Roll Engine from 3a0a63c2d209 to 5597ef01f5d2 (25 revisions) flutter/flutter#92778

Closed

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

8d7d4e7 Add traces to Android embedding (flutter/engine#29230)

54f7110

engine-flutter-autoroll mentioned this pull request Oct 31, 2021

Roll Engine from 3a0a63c2d209 to 8ae70a59792e (26 revisions) flutter/flutter#92781

Closed

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

8d7d4e7 Add traces to Android embedding (flutter/engine#29230)

f802594

engine-flutter-autoroll mentioned this pull request Oct 31, 2021

Roll Engine from 3a0a63c2d209 to e781ed8b2980 (27 revisions) flutter/flutter#92784

Closed

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

8d7d4e7 Add traces to Android embedding (flutter/engine#29230)

9e2b985

engine-flutter-autoroll mentioned this pull request Oct 31, 2021

Roll Engine from 3a0a63c2d209 to e781ed8b2980 (27 revisions) flutter/flutter#92786

Closed

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

8d7d4e7 Add traces to Android embedding (flutter/engine#29230)

cd57604

engine-flutter-autoroll mentioned this pull request Oct 31, 2021

Roll Engine from 3a0a63c2d209 to e781ed8b2980 (27 revisions) flutter/flutter#92794

Closed

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

8d7d4e7 Add traces to Android embedding (flutter/engine#29230)

1934681

zanderso mentioned this pull request Nov 1, 2021

Engine -> Framework roll blocked on new Android tracing library flutter/flutter#92801

Closed

jiahaog added a commit to jiahaog/flutter-engine that referenced this pull request Nov 1, 2021

Revert "Add traces to Android embedding (flutter#29230)"

df8b562

This reverts commit 8d7d4e7. Manually resolved conflict in tools/androidx/files.json

engine-flutter-autoroll mentioned this pull request Nov 1, 2021

Roll Engine from 3a0a63c2d209 to 9f3ca93d8d59 (28 revisions) flutter/flutter#92813

Closed

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

8d7d4e7 Add traces to Android embedding (flutter/engine#29230)

264f2b1

engine-flutter-autoroll mentioned this pull request Nov 1, 2021

Roll Engine from 3a0a63c2d209 to 1eec8c1e57e4 (29 revisions) flutter/flutter#92819

Closed

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

8d7d4e7 Add traces to Android embedding (flutter/engine#29230)

0acb487

engine-flutter-autoroll mentioned this pull request Nov 1, 2021

Roll Engine from 3a0a63c2d209 to 9c5f5d23347e (30 revisions) flutter/flutter#92823

Closed

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

8d7d4e7 Add traces to Android embedding (flutter/engine#29230)

fec7659

engine-flutter-autoroll mentioned this pull request Nov 1, 2021

Roll Engine from 3a0a63c2d209 to 9c88e86f5d05 (31 revisions) flutter/flutter#92828

Closed

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

8d7d4e7 Add traces to Android embedding (flutter/engine#29230)

8e1c21f

chinmaygarde mentioned this pull request Mar 15, 2022

[Android][Production]: java.lang.IllegalArgumentException sectionName is too long flutter/flutter#99980

Closed

		@@ -83,7 +84,8 @@ public DartExecutor(@NonNull FlutterJNI flutterJNI, @NonNull AssetManager assetM
		public void onAttachedToJNI() {
		Log.v(

Add traces to Android embedding #29230

Add traces to Android embedding #29230

Uh oh!

Conversation

jiahaog commented Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

jiahaog commented Oct 18, 2021

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!

jiahaog Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jiahaog Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jiahaog Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

blasten commented Oct 18, 2021

Uh oh!

jiahaog commented Oct 19, 2021

Uh oh!

jiahaog commented Oct 19, 2021

Uh oh!

jiahaog commented Oct 18, 2021 •

edited

Loading

jiahaog Oct 19, 2021 •

edited

Loading

jiahaog Oct 21, 2021 •

edited

Loading

jiahaog Oct 21, 2021 •

edited

Loading

jiahaog commented Oct 21, 2021 •

edited

Loading