-
Notifications
You must be signed in to change notification settings - Fork 6k
[WIP] Split AOT Engine changes master #21173
Conversation
8dfbedb to
7b81f6c
Compare
3950729 to
b9255eb
Compare
| embedding "androidx.lifecycle:lifecycle-common:$lifecycle_version" | ||
| embedding "androidx.lifecycle:lifecycle-common-java8:$lifecycle_version" | ||
|
|
||
| embedding "com.google.android.play:core:1.8.0" |
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.
ah of course. This makes sense. Though we should try to clarify this high level question early on. Could you take a hello_world app and build it with and without this in release mode (thus R8'ed)? We should compare the APK sizes of the engine with and without this dependency to see if this is an acceptable increase.
We can also drag and drop the old and new APK into Android Studio, introspect the .dex and look at all the new Java classes that's brought in to see if it makes sense or we need to tweak the default R8 rules more.
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.
From what I can tell form the docs, it looks like this package is pretty lightweight. I don't expect it to add much, though I'll also run the experiment to put a number on it.
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.
We should test. One possible case is that that the package itself has very little code but declares other transitive Gradle dependencies.
| * {@code io.flutter.embedding.android} since they self-initialize on first use. | ||
| */ | ||
| public class FlutterApplication extends Application { | ||
| public class FlutterApplication extends SplitCompatApplication { |
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.
We already started asking people to get rid of the FlutterApplication. If this is needed, we might have to add some docs for people to manually add it back or modify their existing Application superclass.
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.
There are other ways to accomplish this. This was just the simplest, but I can easily move to one of the other two methods that dont reply on this extension. See https://developer.android.com/guide/playcore/dynamic-delivery#enable_splitcompat
| import androidx.lifecycle.AndroidViewModel; | ||
| import androidx.lifecycle.LiveData; | ||
| import androidx.lifecycle.MutableLiveData; | ||
| import com.google.android.play.core.splitinstall.SplitInstallException; |
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.
We don't have to figure this out now, and you're probably already thinking about this since it implements an interface. But since this is all dead code for devs in China, we should figure out some sort of compile time or R8 time mechanism to let them strip all this out of their engine .so.
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.
Preliminary APK size comparisons point to a 30kb gain from play core + split aot code. I expect this number may be able to be reduced further as I realized I did not use a LTO dart for the comparison.
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.
Oops, the above comment probably belongs in the other thread about apk sizes.
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.
can you check in the apk analyzer where it came from? i.e. from the dex or from the sum of the .so being higher than 1 .so?
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.
Just did another measurement, this time 28,935 bytes with a full LTO build. I'll take a look at source.
e7afac9 to
213b864
Compare
b297499 to
f29ae06
Compare
|
I will be splitting this PR into the embedder+shell section and the runtime+dart_isolate section |
Begin impl android embedder work Add success and failure callbacks Success and failure calls Additional impl Begin engine impl Building No crash Additional logging Switch to int loading unit id Splits install now Begin reading libs Track loadingUnitId as well: Pass dart handler call through to embedder One direction works Move dart complete into platform_view_android Move loading into runtime Loading .so data complete Move dart set handler to dart isolate Successfully loads dart libs Filter by ABI Dynamic abi passing, cleanup Compile and cleanup Fix dependencies Separate handler into own class Move static vars to handler Clean imports, comments USe io.flutter.Log Fix host unittests Use temp void* Switch to override splitcompat Working asset manager load Merge with upstream, working again
Build on host
|
#22179 contains the android embedder and shell components of split AOT. |
|
You don't need this PR anymore right? |
|
Ahh, yes. This can be closed! |
WIP