Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Split AOT Android Embedder and shell #22179

Merged
merged 29 commits into from
Nov 20, 2020
Merged

Split AOT Android Embedder and shell #22179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a9776d6
Initial code
GaryQian Sep 11, 2020
ea56d3f
Java formatting
GaryQian Oct 29, 2020
5b2cc97
Build on linux and fuchsia
GaryQian Oct 29, 2020
2190ed4
Initial tests
GaryQian Nov 12, 2020
5d64733
Formatting
GaryQian Nov 12, 2020
76071fb
Fix test
GaryQian Nov 12, 2020
396e40b
Fix tests
GaryQian Nov 12, 2020
44e9750
Working test, more tests
GaryQian Nov 13, 2020
32982a7
Add platformview test, adjust java tests
GaryQian Nov 13, 2020
4df3e33
Add another test
GaryQian Nov 13, 2020
de074f8
CLeanup
GaryQian Nov 13, 2020
feb4c99
Tests avoid splitManager global state bug
GaryQian Nov 16, 2020
49a0532
Docs
GaryQian Nov 17, 2020
20d4e3e
Remove extra imports
GaryQian Nov 17, 2020
b0002e4
Fix ios and fuchsia tests
GaryQian Nov 17, 2020
340eb5e
Remove splitcompat
GaryQian Nov 17, 2020
cfeb109
Licenses fix
GaryQian Nov 17, 2020
ae7b771
Fix fuchsia mock
GaryQian Nov 17, 2020
9ed7094
Address docs comments, add destructor
GaryQian Nov 18, 2020
eb3ccef
Formatting
GaryQian Nov 18, 2020
a1c603c
provide DynamicFeatureManager in FlutterLoader
GaryQian Nov 18, 2020
372adf9
Set JNI in the JNI itself
GaryQian Nov 18, 2020
e8861b5
Formatting
GaryQian Nov 18, 2020
4366c79
Add flutter injector tests
GaryQian Nov 18, 2020
2130d59
Formatting and comment nit
GaryQian Nov 18, 2020
b8cfc01
More documentation changes
GaryQian Nov 18, 2020
c995673
Address comments
GaryQian Nov 19, 2020
e4e9a1e
Formatting
GaryQian Nov 19, 2020
f006e9b
FOrmatting
GaryQian Nov 19, 2020
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
2 changes: 1 addition & 1 deletion DEPS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ deps = {
'packages': [
{
'package': 'flutter/android/embedding_bundle',
'version': 'last_updated:2020-05-20T01:36:16-0700'
Copy link
Member

@xster xster Oct 30, 2020

Choose a reason for hiding this comment

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

I think this is all fine for a local build but you still need to update

engine/shell/platform/android/BUILD.gn

Line 394 in 6166712

script = "//flutter/tools/androidx/generate_pom_file.py"
to actually build it into the distributed engine artifact.

i.e. I don't think this will compile on LUCI. @jason-simmons might know more.

Copy link
Contributor Author

@GaryQian GaryQian Oct 30, 2020

Choose a reason for hiding this comment

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

The LUCI tests were able to all pass on this PR this afternoon, I'll take a look at updating it in the above mentioned spot too.

Copy link
Member

@xster xster Nov 9, 2020

Choose a reason for hiding this comment

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

I think it'll pass on LUCI since the tests will run in the "development path". But I think it won't package up the dependencies in the vendored engine build on cloud storage.

'version': 'last_updated:2020-09-11T17:57:41-0700'
}
],
'condition': 'download_android_deps',
Expand Down
2 changes: 2 additions & 0 deletions ci/licenses_golden/licenses_flutter
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -747,6 +747,8 @@ FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/Flutte
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/dart/DartExecutor.java
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/dart/DartMessenger.java
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/dart/PlatformMessageHandler.java
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/dynamicfeatures/DynamicFeatureManager.java
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/dynamicfeatures/PlayStoreDynamicFeatureManager.java
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/loader/ApplicationInfoLoader.java
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/loader/FlutterApplicationInfo.java
FILE: ../../../flutter/shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java
Expand Down
17 changes: 17 additions & 0 deletions shell/common/engine.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,4 +507,21 @@ const std::string& Engine::GetLastEntrypointLibrary() const {
return last_entry_point_library_;
}

// The Following commented out code connects into part 2 of the split AOT
// feature. Left commented out until it lands:

// // |RuntimeDelegate|
// void Engine::RequestDartDeferredLibrary(intptr_t loading_unit_id) {
// return delegate_.RequestDartDeferredLibrary(loading_unit_id);
// }

void Engine::LoadDartDeferredLibrary(intptr_t loading_unit_id,
const uint8_t* snapshot_data,
const uint8_t* snapshot_instructions) {
if (runtime_controller_->IsRootIsolateRunning()) {
// runtime_controller_->LoadDartDeferredLibrary(loading_unit_id,
// snapshot_data, snapshot_instructions);
}
}

} // namespace flutter
53 changes: 53 additions & 0 deletions shell/common/engine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,21 @@ class Engine final : public RuntimeDelegate,
virtual std::unique_ptr<std::vector<std::string>>
ComputePlatformResolvedLocale(
const std::vector<std::string>& supported_locale_data) = 0;

//--------------------------------------------------------------------------
/// @brief Invoked when the Dart VM requests that a deferred library
/// be loaded. Notifies the engine that the deferred library
/// identified by the specified loading unit id should be
/// downloaded and loaded into the Dart VM via
/// `LoadDartDeferredLibrary`
///
/// @param[in] loading_unit_id The unique id of the deferred library's
Copy link
Member

@xster xster Oct 30, 2020

Choose a reason for hiding this comment

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

We should clarify at this level what are we loading specifically? Is it the Dart specific loading units or is it the platform-specific bundles? We should be exact with the name. i.e. in the asset-only bundle case, that asset's still loaded through the Dart API or is it a different API?

Copy link
Contributor Author

@GaryQian GaryQian Nov 7, 2020

Choose a reason for hiding this comment

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

I will be adding separate dart API for loading by android module name. This is currently strictly for the dart loadLibrary() code path. I'll link to additional info in a separate PR introducing the new API for module name/asset only loading.

/// loading unit. This id is to be passed
/// back into LoadDartDeferredLibrary
/// in order to identify which deferred
/// library to load.
///
virtual void RequestDartDeferredLibrary(intptr_t loading_unit_id) = 0;
};

//----------------------------------------------------------------------------
Expand Down Expand Up @@ -767,6 +782,38 @@ class Engine final : public RuntimeDelegate,
///
const std::string& InitialRoute() const { return initial_route_; }

//--------------------------------------------------------------------------
/// @brief Loads the Dart shared library into the Dart VM. When the
/// Dart library is loaded successfully, the Dart future
/// returned by the originating loadLibrary() call completes.
///
/// The Dart compiler may generate separate shared libraries
/// files called 'loading units' when libraries are imported
/// as deferred. Each of these shared libraries are identified
/// by a unique loading unit id. Callers should dlopen the
/// shared library file and use dlsym to resolve the dart
/// symbols. These symbols can then be passed to this method to
/// be dynamically loaded into the VM.
///
/// This method is paired with a RequestDartDeferredLibrary
/// invocation that provides the embedder with the loading unit id
/// of the deferred library to load.
///
///
/// @param[in] loading_unit_id The unique id of the deferred library's
Copy link
Member

@xster xster Nov 9, 2020

Choose a reason for hiding this comment

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

Explicitly state this loading unit id's relationship with the previous request call? Or are they independent?

Copy link
Member

@xster xster Nov 17, 2020

Choose a reason for hiding this comment

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

You didn't address this one. The 2 functions' docs should explicitly cross-reference each other.

/// loading unit, as passed in by
/// RequestDartDeferredLibrary.
///
/// @param[in] snapshot_data Dart snapshot data of the loading unit's
/// shared library.
///
/// @param[in] snapshot_data Dart snapshot instructions of the loading
/// unit's shared library.
///
void LoadDartDeferredLibrary(intptr_t loading_unit_id,
const uint8_t* snapshot_data,
const uint8_t* snapshot_instructions);

private:
Engine::Delegate& delegate_;
const Settings settings_;
Expand Down Expand Up @@ -815,6 +862,12 @@ class Engine final : public RuntimeDelegate,
std::unique_ptr<std::vector<std::string>> ComputePlatformResolvedLocale(
const std::vector<std::string>& supported_locale_data) override;

// The Following commented out code connects into part 2 of the split AOT
// feature. Left commented out until it lands:

// // |RuntimeDelegate|
// void RequestDartDeferredLibrary(intptr_t loading_unit_id) override;

void SetNeedsReportTimings(bool value) override;

void StopAnimator();
Expand Down
2 changes: 2 additions & 0 deletions shell/common/engine_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class MockDelegate : public Engine::Delegate {
MOCK_METHOD1(ComputePlatformResolvedLocale,
std::unique_ptr<std::vector<std::string>>(
const std::vector<std::string>&));
MOCK_METHOD1(RequestDartDeferredLibrary, void(intptr_t));
};

class MockResponse : public PlatformMessageResponse {
Expand All @@ -55,6 +56,7 @@ class MockRuntimeDelegate : public RuntimeDelegate {
MOCK_METHOD1(ComputePlatformResolvedLocale,
std::unique_ptr<std::vector<std::string>>(
const std::vector<std::string>&));
MOCK_METHOD1(RequestDartDeferredLibrary, void(intptr_t));
};

class MockRuntimeController : public RuntimeController {
Expand Down
10 changes: 10 additions & 0 deletions shell/common/platform_view.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,4 +159,14 @@ PlatformView::ComputePlatformResolvedLocales(
return out;
}

void PlatformView::RequestDartDeferredLibrary(intptr_t loading_unit_id) {}

void PlatformView::LoadDartDeferredLibrary(
intptr_t loading_unit_id,
const uint8_t* snapshot_data,
const uint8_t* snapshot_instructions) {}

void PlatformView::UpdateAssetManager(
std::shared_ptr<AssetManager> asset_manager) {}

} // namespace flutter
93 changes: 93 additions & 0 deletions shell/common/platform_view.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,43 @@ class PlatformView {
///
virtual void OnPlatformViewMarkTextureFrameAvailable(
int64_t texture_id) = 0;

//--------------------------------------------------------------------------
/// @brief Loads the dart shared library into the dart VM. When the
/// dart library is loaded successfully, the dart future
/// returned by the originating loadLibrary() call completes.
///
/// The Dart compiler may generate separate shared library .so
/// files called 'loading units' when libraries are imported
/// as deferred. Each of these shared libraries are identified
/// by a unique loading unit id and can be dynamically loaded
/// into the VM by dlopen-ing and resolving the data and
/// instructions symbols.
///
///
/// @param[in] loading_unit_id The unique id of the deferred library's
/// loading unit.
///
/// @param[in] snapshot_data Dart snapshot data of the loading unit's
/// shared library.
///
/// @param[in] snapshot_data Dart snapshot instructions of the loading
/// unit's shared library.
///
virtual void LoadDartDeferredLibrary(
intptr_t loading_unit_id,
const uint8_t* snapshot_data,
const uint8_t* snapshot_instructions) = 0;

// TODO(garyq): Implement a proper asset_resolver replacement instead of
// overwriting the entire asset manager.
//--------------------------------------------------------------------------
/// @brief Sets the asset manager of the engine to asset_manager
///
/// @param[in] asset_manager The asset manager to use.
///
virtual void UpdateAssetManager(
Copy link
Member

@xster xster Nov 9, 2020

Choose a reason for hiding this comment

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

noob question / I might not really understand how this works: how come it's implemented as replacing the asset manager instead of pushing a new resolver inside the existing manager?

Copy link
Contributor Author

@GaryQian GaryQian Nov 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I mentioned this in a TODO somewhere, This "replacement" is temporary. I plan on implementing a system to push a new asset resolver, but the challenge is to track and dispose of the previous APKAssetResolver to prevent a leak. For now, this is more of a stand-in implementation. I expect the full implementation to require additional research and/or new features in the asset manager to be able to specifically find and replace a particular asset resolver. I'd like to do it in a separate PR.

Copy link
Member

@xster xster Nov 10, 2020

Choose a reason for hiding this comment

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

ah ok, leave a TODO so future maintainers don't pile onto this

Copy link
Member

@xster xster Nov 17, 2020

Choose a reason for hiding this comment

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

You didn't address this one yet.

Copy link
Contributor Author

@GaryQian GaryQian Nov 18, 2020

Choose a reason for hiding this comment

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

There is a TODO in one of the other versions of this, I'll add it here too.

std::shared_ptr<AssetManager> asset_manager) = 0;
};

//----------------------------------------------------------------------------
Expand Down Expand Up @@ -565,6 +602,62 @@ class PlatformView {

virtual std::shared_ptr<ExternalViewEmbedder> CreateExternalViewEmbedder();

//--------------------------------------------------------------------------
/// @brief Invoked when the dart VM requests that a deferred library
/// be loaded. Notifies the engine that the deferred library
/// identified by the specified loading unit id should be
/// downloaded and loaded into the Dart VM via
/// `LoadDartDeferredLibrary`
///
/// @param[in] loading_unit_id The unique id of the deferred library's
/// loading unit. This id is to be passed
/// back into LoadDartDeferredLibrary
/// in order to identify which deferred
/// library to load.
///
virtual void RequestDartDeferredLibrary(intptr_t loading_unit_id);

//--------------------------------------------------------------------------
/// @brief Loads the Dart shared library into the Dart VM. When the
/// Dart library is loaded successfully, the Dart future
/// returned by the originating loadLibrary() call completes.
///
/// The Dart compiler may generate separate shared libraries
/// files called 'loading units' when libraries are imported
/// as deferred. Each of these shared libraries are identified
/// by a unique loading unit id. Callers should dlopen the
/// shared library file and use dlsym to resolve the dart
/// symbols. These symbols can then be passed to this method to
/// be dynamically loaded into the VM.
///
/// This method is paired with a RequestDartDeferredLibrary
/// invocation that provides the embedder with the loading unit id
/// of the deferred library to load.
///
///
/// @param[in] loading_unit_id The unique id of the deferred library's
/// loading unit, as passed in by
/// RequestDartDeferredLibrary.
///
/// @param[in] snapshot_data Dart snapshot data of the loading unit's
/// shared library.
///
/// @param[in] snapshot_data Dart snapshot instructions of the loading
/// unit's shared library.
///
virtual void LoadDartDeferredLibrary(intptr_t loading_unit_id,
const uint8_t* snapshot_data,
const uint8_t* snapshot_instructions);

// TODO(garyq): Implement a proper asset_resolver replacement instead of
// overwriting the entire asset manager.
//--------------------------------------------------------------------------
/// @brief Sets the asset manager of the engine to asset_manager
///
/// @param[in] asset_manager The asset manager to use.
///
virtual void UpdateAssetManager(std::shared_ptr<AssetManager> asset_manager);

protected:
PlatformView::Delegate& delegate_;
const TaskRunners task_runners_;
Expand Down
16 changes: 16 additions & 0 deletions shell/common/shell.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1185,6 +1185,22 @@ std::unique_ptr<std::vector<std::string>> Shell::ComputePlatformResolvedLocale(
return platform_view_->ComputePlatformResolvedLocales(supported_locale_data);
Copy link
Member

@xster xster Oct 30, 2020

Choose a reason for hiding this comment

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

is this related?

Copy link
Contributor Author

@GaryQian GaryQian Nov 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Oh this is not related. I realized that one of the calls was extraneous and decided to fix it. I'll move this to a separate PR.

Copy link
Contributor Author

@GaryQian GaryQian Nov 6, 2020

Choose a reason for hiding this comment

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

Separate PR here: #22362

}

void Shell::LoadDartDeferredLibrary(intptr_t loading_unit_id,
const uint8_t* snapshot_data,
const uint8_t* snapshot_instructions) {
engine_->LoadDartDeferredLibrary(loading_unit_id, snapshot_data,
snapshot_instructions);
}

void Shell::UpdateAssetManager(std::shared_ptr<AssetManager> asset_manager) {
engine_->UpdateAssetManager(std::move(asset_manager));
}

// |Engine::Delegate|
void Shell::RequestDartDeferredLibrary(intptr_t loading_unit_id) {
platform_view_->RequestDartDeferredLibrary(loading_unit_id);
}

void Shell::ReportTimings() {
FML_DCHECK(is_setup_);
FML_DCHECK(task_runners_.GetRasterTaskRunner()->RunsTasksOnCurrentThread());
Expand Down
11 changes: 11 additions & 0 deletions shell/common/shell.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,14 @@ class Shell final : public PlatformView::Delegate,
// |PlatformView::Delegate|
void OnPlatformViewSetNextFrameCallback(const fml::closure& closure) override;

// |PlatformView::Delegate|
void LoadDartDeferredLibrary(intptr_t loading_unit_id,
const uint8_t* snapshot_data,
const uint8_t* snapshot_instructions) override;

// |PlatformView::Delegate|
void UpdateAssetManager(std::shared_ptr<AssetManager> asset_manager) override;

// |Animator::Delegate|
void OnAnimatorBeginFrame(fml::TimePoint frame_target_time) override;

Expand Down Expand Up @@ -548,6 +556,9 @@ class Shell final : public PlatformView::Delegate,
std::unique_ptr<std::vector<std::string>> ComputePlatformResolvedLocale(
const std::vector<std::string>& supported_locale_data) override;

// |Engine::Delegate|
void RequestDartDeferredLibrary(intptr_t loading_unit_id) override;

// |Rasterizer::Delegate|
void OnFrameRasterized(const FrameTiming&) override;

Expand Down
3 changes: 3 additions & 0 deletions shell/platform/android/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ android_java_sources = [
"io/flutter/embedding/engine/dart/DartExecutor.java",
"io/flutter/embedding/engine/dart/DartMessenger.java",
"io/flutter/embedding/engine/dart/PlatformMessageHandler.java",
"io/flutter/embedding/engine/dynamicfeatures/DynamicFeatureManager.java",
"io/flutter/embedding/engine/dynamicfeatures/PlayStoreDynamicFeatureManager.java",
"io/flutter/embedding/engine/loader/ApplicationInfoLoader.java",
"io/flutter/embedding/engine/loader/FlutterApplicationInfo.java",
"io/flutter/embedding/engine/loader/FlutterLoader.java",
Expand Down Expand Up @@ -462,6 +464,7 @@ action("robolectric_tests") {
"test/io/flutter/embedding/engine/RenderingComponentTest.java",
"test/io/flutter/embedding/engine/dart/DartExecutorTest.java",
"test/io/flutter/embedding/engine/dart/DartMessengerTest.java",
"test/io/flutter/embedding/engine/dynamicfeatures/PlayStoreDynamicFeatureManagerTest.java",
"test/io/flutter/embedding/engine/loader/ApplicationInfoLoaderTest.java",
"test/io/flutter/embedding/engine/loader/FlutterLoaderTest.java",
"test/io/flutter/embedding/engine/mutatorsstack/FlutterMutatorViewTest.java",
Expand Down
6 changes: 6 additions & 0 deletions shell/platform/android/embedding_bundle/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ android {
embedding "androidx.lifecycle:lifecycle-common:$lifecycle_version"
embedding "androidx.lifecycle:lifecycle-common-java8:$lifecycle_version"

// This dependency is here to allow linking to Play core in tests, but
// is not used in a default Flutter app. This dependency should be manually
// added to the user's app gradle in order to opt into using split AOT
// dynamic features.
embedding "com.google.android.play:core:1.8.0"
Copy link
Member

@xster xster Oct 30, 2020

Choose a reason for hiding this comment

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

28k seems borderline ok. I'd still like us to break down exactly what those 28k are. Are they all dex? If so, what's the package/class breakdown. Are we using all of them? Are there proguard rules we should tweak after adding this.

Copy link
Contributor Author

@GaryQian GaryQian Nov 16, 2020

Choose a reason for hiding this comment

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

This PR no longer imports the Play core dependent classes, leaving them as floating classes for now, which can be opted into by the developer when deciding to use this feature.

Copy link
Member

@xster xster Nov 17, 2020

Choose a reason for hiding this comment

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

Add a comment here to say that it's just here for test but not really added to the engine's dependencies?


// Testing
// TODO(xster): remove these android-all compile time dependencies.
// Use https://github.com/robolectric/robolectric/blob/master/robolectric/src/main/java/org/robolectric/plugins/LegacyDependencyResolver.java#L24
Expand Down
Loading