Split AOT Engine Runtime #22624

GaryQian · 2020-11-20T01:49:39Z

Description

Adds a Dart_DeferredLibraryHandler to plumb loadLibrary() calls from dart through the dart isolate, platform configuration, runtime controller into the shell entrypoint of Engine::RequestDartDeferredLibrary.

Also connects the shell's Engine::LoadDartDeferredLibrary to the dart isolate.

Related Issues

flutter/flutter#62229
flutter/flutter#57617

chinmaygarde · 2020-11-24T21:12:23Z

runtime/dart_isolate.cc

+  tonic::DartState::Scope scope(this);
+  Dart_Handle result = Dart_DeferredLoadComplete(loading_unit_id, snapshot_data,
+                                                 snapshot_instructions);
+  if (Dart_IsApiError(result)) {


Here and elsewhere, tonic::LogIfError unless there is a reason this can only be an API error.

chinmaygarde · 2020-11-24T21:13:16Z

runtime/dart_isolate.cc

@@ -320,6 +320,8 @@ bool DartIsolate::Initialize(Dart_Isolate dart_isolate) {
    return false;
  }

+  Dart_SetDeferredLoadHandler(OnDartLoadLibrary);


Unchecked error handle.

chinmaygarde · 2020-11-24T21:16:38Z

shell/common/engine.cc

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


Can this return an error (or boolean to indicate one) if the request failed. I am not comfortable swallowing a potential error here.

The idea is that errors/failures should invoke LoadDartDeferredLibraryFailure. Capturing/returning any errors here may inadvertently indicate to the dev that the error handling is complete. This method is meant to inform the embedder/engine that the dart lib is requested, but makes no promises that it will or even should succeed.

I can make sure I document this.

The full LoadDartDeferredLibraryFailure implementation is coming soon in a future PR.

Also, this request is not blocking, as the majority of the work happens async in the downloading/extracting/opening/io of the lib. Thus, even a return value of true would not indicate success since most of the work has yet to be done.

chinmaygarde · 2020-11-24T21:17:50Z

runtime/runtime_controller.h

+  ///             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


Please make a note here about the lifecycle of the shared library handle. Is it tied to the isolate or the VM for example. Also, make a note of who owns it in the engine right now.

chinmaygarde · 2020-11-24T21:19:26Z

runtime/runtime_controller.h

+  /// @param[in]  snapshot_data    Dart snapshot instructions of the loading
+  ///                              unit's shared library.
+  ///
+  void LoadDartDeferredLibrary(intptr_t loading_unit_id,


I think, at this layer, we can use fml::Mappings instead. We'll still have to use raw pointers to talk to Dart. Using mappings (specifically SymbolMappings) will make the code self documenting and readers will be better able to follow ownership.

chinmaygarde · 2020-11-24T21:20:03Z

runtime/runtime_controller.h

+  /// @param[in]  loading_unit_id  The unique id of the deferred library's
+  ///                              loading unit.
+  ///
+  /// @return     A Dart_Handle that is Dart_Null on success, and a dart error


The docs say you return something by you actually return void. I think you missed propagating the error.

chinmaygarde · 2020-11-24T21:26:42Z

Is this still WIP? Its causing it to skip review queues.

GaryQian · 2020-11-24T22:30:06Z

I had it marked Draft when I last pushed, it should go through precheck whenever I push again.

xster

Please wait for Chinmay's LGTM too. But LGTM

xster · 2020-11-25T08:09:50Z

shell/common/engine.h

-  ///             symbols. These symbols can then be passed to this method to
-  ///             be dynamically loaded into the VM.
+  ///             by a unique loading unit id. Callers should open and resolve
+  ///             a SymbolMapping from the shared library. The symbols can


If you're being explicit about taking a SymbolMapping, should the signature be specified to take SymbolMapping too?

SymbolMapping is used when we own the the native library handle too. In some cases (usually where the platform or embedder owns this handle), we can use an UnownedMapping. In any case, both are subclasses of fml::Mapping. I don't think this layer should care how the mapping is resolved. Just that there is a mapping whose lifecycle has been sorted out (and readers can follow).

chinmaygarde

Please don't land this till the symbol lifecycle issue is fixed. Also, this has no tests but I am not sure if this is WIP. When you do, please add one to runtime_unittests.

chinmaygarde · 2020-11-25T22:56:52Z

runtime/dart_isolate.cc

@@ -320,6 +320,8 @@ bool DartIsolate::Initialize(Dart_Isolate dart_isolate) {
    return false;
  }

+  tonic::LogIfError(Dart_SetDeferredLoadHandler(OnDartLoadLibrary));


I know its a really weird API but LogIfError returns a bool if there an error. So, you'd terminate isolate initialization in this case.

chinmaygarde · 2020-11-25T23:01:19Z

shell/common/engine.h

-  ///             symbols. These symbols can then be passed to this method to
-  ///             be dynamically loaded into the VM.
+  ///             by a unique loading unit id. Callers should open and resolve
+  ///             a SymbolMapping from the shared library. The symbols can


SymbolMapping is used when we own the the native library handle too. In some cases (usually where the platform or embedder owns this handle), we can use an UnownedMapping. In any case, both are subclasses of fml::Mapping. I don't think this layer should care how the mapping is resolved. Just that there is a mapping whose lifecycle has been sorted out (and readers can follow).

chinmaygarde · 2020-11-25T23:18:34Z

runtime/dart_isolate.cc

+bool DartIsolate::LoadLoadingUnit(
+    intptr_t loading_unit_id,
+    std::unique_ptr<const fml::SymbolMapping> snapshot_data,
+    std::unique_ptr<const fml::SymbolMapping> snapshot_instructions) {


A subtle error has been introduced here when you went from the C++ API to the Dart C API. The symbol mapping owns the library handle you created later in this patch. However, both go out of scope here. The library is now un-reffed and may close the library handle. This can cause issues depending on the dynamic linker implementation. From the docs of dlcose:

"The use of dlclose() reflects a statement of intent on the part of the process, but does not create any requirement upon the implementation, such as removal of the code or symbols referenced by handle. Once an object has been closed using dlclose() an application should assume that its symbols are no longer available to dlsym()."

Depending on the implementation, subject to timing issues within the implementation, and, other references to this library by the engine, the OS is also free to unmap the memory you just gave the Dart code via the C API which would then lead to segfaults.

We do this in other places by owning a reference to a flutter::DartSnapshot which is nothing but two mappings. I suggest you do the same. Maybe toss it inside a set in DartIsolate.

I am glad we are running into these design issues now. Using the C++ wrappers forces you to think about lifecycle issues that raw pointers don't. This would be a very hard to debug segfault otherwise.

Ahh, I see what you meant by lifecycle now. Thanks for catching this and the explanation.

Right. When you vend a raw pointer, you need to know upfront till when the user expects to use the pointer. That way, you can keep what is being pointed to alive for at least that long.

From my last comment on lifecycle, it is still not clear to me if the VM can use this post isolate termination. Presumably not, but just good to check.

GaryQian · 2020-12-02T01:48:26Z

Some test behavior is waiting on https://dart-review.googlesource.com/c/sdk/+/174563 to land and roll to better handle failure to load cases.

chinmaygarde

Except the null return in case of error in DartIsolate::OnDartLoadLibrary, everything else is nits and minor observations. Otherwise LGTM. Nicely done.

chinmaygarde · 2020-12-02T18:18:17Z

testing/elf_loader.h

@@ -15,6 +15,8 @@ namespace flutter {
 namespace testing {

 inline constexpr const char* kAOTAppELFFileName = "app_elf_snapshot.so";
+inline constexpr const char* kAOTAppELFSplitFileName =


Not sure how this name was generated. I see the fixture generator specifying the app_elf_snapshot base name but unclear about the so-2.part bit. Can you add a comment here for readers on how the tooling generates this name? Or, just be explicit about the name if the capability to do so is already present.

Yeah, sure. This naming is just what gen_snapshot defaults to. The 2 indicates the id for the loading unit is 2 (base module is 1).

chinmaygarde · 2020-12-02T18:19:08Z

testing/fixture_test.h

@@ -8,6 +8,7 @@
 #include <memory>

 #include "flutter/common/settings.h"
+#include "flutter/fml/mapping.h"


Nit: Unsure why this include was necessary. ELFAOTSymbols is not in fml/mapping.

chinmaygarde · 2020-12-02T18:28:49Z

runtime/runtime_controller.h

+  ///                              in order to identify which deferred
+  ///                              library to load.
+  ///
+  void RequestDartDeferredLibrary(intptr_t loading_unit_id) override;


Nit: The doc generation works better when the docs are added to the virtual methods in the base class instead of the overriden ones in the subclass. So, I'd move these doc comments to platform_configuration.h. Keep the | PlatformConfigurationClient| comment though.

chinmaygarde · 2020-12-02T18:30:38Z

runtime/runtime_controller.h

+  ///
+  void LoadDartDeferredLibrary(
+      intptr_t loading_unit_id,
+      std::unique_ptr<const fml::Mapping> snapshot_data,


I thought you were moving these over to use flutter::DartSnapshot. Either way is fine though as the DartSnapshot is nothing but a pointer to two mappings. I see you are wrapping this up in the implementation. you could also just make the caller wrap it for you. Just a thought.

chinmaygarde · 2020-12-02T18:32:50Z

runtime/runtime_controller.cc

@@ -4,6 +4,8 @@

 #include "flutter/runtime/runtime_controller.h"

+#include <sstream>


I believe this include is spurious.

chinmaygarde · 2020-12-02T18:36:15Z

runtime/dart_isolate.cc

+    Current()->platform_configuration()->client()->RequestDartDeferredLibrary(
+        loading_unit_id);
+  } else {
+    FML_LOG(ERROR) << "Platform Configuration was null. Deferred library load "


Shouldn't we be returning an error handle here instead of Dart_Null?

Revert accidental comment changes

Pass as mapping cleanup Docs More docs Cleanup

SymbolMapping

Use unordered_set

Begin testing work Loading split aot tests part 1 Passing tests, but flaky due to race

Docs for testing changes Licenses

Additional cleanup

aam · 2020-12-03T17:38:23Z

I believe the test DartIsolateTest_ValidLoadingUnitSucceeds_Test introduced in this CL is flaky: flutter/flutter#71661

* d6beaed Roll Fuchsia Linux SDK from gkfmiRsIl... to un3JixwuO... (flutter/engine#22744) * 8832b48 Roll Skia from 888c5d3e57eb to 51b74afb84d4 (12 revisions) (flutter/engine#22746) * e890901 Don't register CanvasKit with `define` (flutter/engine#22745) * 3c51679 Roll Skia from 51b74afb84d4 to 452369182f6e (1 revision) (flutter/engine#22749) * 5bf6533 Introduce a delegate class for gpu metal rendering (flutter/engine#22611) * 5131aa4 Roll Skia from 452369182f6e to f2efb80bc316 (4 revisions) (flutter/engine#22750) * 7b5f79f fuchsia: Ensure full-screen input interceptor (flutter/engine#22687) * cec8a6e Manual roll of Dart SDK from ce76503f5b46 to dcd5a8f005a (flutter/engine#22766) * 001a511 Roll Fuchsia Linux SDK from un3JixwuO... to Bnaeivv07... (flutter/engine#22757) * b9615b1 Roll Fuchsia Mac SDK from 36uDTGJQp... to qpkZl0s5J... (flutter/engine#22753) * c4c4763 Roll Skia from f2efb80bc316 to 8d78da910e45 (5 revisions) (flutter/engine#22754) * dbd1abe Roll Dart SDK from dcd5a8f005a2 to 960620d2e811 (794 revisions) (flutter/engine#22768) * 1c2a6bd Fix the unchecked conversion warning for searchPaths in PlayStoreDynamicFeatureManager (flutter/engine#22654) * 81af789 add file package to deps in prep for glob update (flutter/engine#22770) * a35e3fe Let FlutterFragment not pop the whole activity by default when more fragments are in the activity (flutter/engine#22692) * adb3312 Revert "Introduce a delegate class for gpu metal rendering (#22611)" (flutter/engine#22775) * bcc8832 Cleanup dart_runner examples & tests. (flutter/engine#22769) * 609307d Roll Skia from 8d78da910e45 to fd41d878b13d (20 revisions) (flutter/engine#22772) * 587c023 [web] Add new line break type (prohibited) (flutter/engine#22771) * 6b2ed2b Roll Skia from fd41d878b13d to 70fe17e12f38 (6 revisions) (flutter/engine#22776) * 7910a17 Roll Dart SDK from 960620d2e811 to 7a2a3968ef53 (12 revisions) (flutter/engine#22778) * f4ada80 Roll Skia from 70fe17e12f38 to 4c6f57a23e63 (1 revision) (flutter/engine#22781) * 3101dff [web] Optimize Matrix4.identity (flutter/engine#22622) * a4ce848 Add FlutterPlayStoreSplitApplication for simpler opt in to Split AOT (flutter/engine#22752) * 747b791 Add file.dart to DEPS (flutter/engine#22794) * 40fa345 Fix race condition in key event handling on Android (flutter/engine#22658) * d2ad441 Fix PlatformDispatcher.locale to return something meaningful when there are no locales. (flutter/engine#22608) * b9a0b5e Roll Skia from 4c6f57a23e63 to a927771c9cce (10 revisions) (flutter/engine#22802) * 96d63e5 Roll Dart SDK from 7a2a3968ef53 to e9a03fd98faa (5 revisions) (flutter/engine#22801) * cdf72da Roll Skia from a927771c9cce to 7b776b514933 (3 revisions) (flutter/engine#22803) * a0c8b67 Roll buildroot and benchmark (flutter/engine#22804) * c3c3ec6 Roll Fuchsia Mac SDK from qpkZl0s5J... to 7O11wjLVX... (flutter/engine#22805) * 6625308 Revert "Roll buildroot and benchmark (#22804)" (flutter/engine#22816) * 64d9add Add a golden scenario test for fallback font rendering on iOS take 3 (flutter/engine#22736) * 7d7a260 Add static text trait to plain semantics object with label in iOS (flutter/engine#22811) * 22e1143 Roll Skia from 7b776b514933 to c504ecda03b8 (6 revisions) (flutter/engine#22808) * 65254eb Roll Dart SDK from e9a03fd98faa to 5acaa5f14b03 (1 revision) (flutter/engine#22810) * 3926b21 Roll Fuchsia Linux SDK from Bnaeivv07... to W14Qninrb... (flutter/engine#22817) * 5eb505f Roll Fuchsia Mac SDK from 7O11wjLVX... to Z_-ciOYM9... (flutter/engine#22820) * d85cb10 add trace kernel flag to allowlist (flutter/engine#22812) * 14cb066 [embedder] Compositor can specify that no backing stores be cached (flutter/engine#22780) * eb6eabc Reland "Introduce a delegate class for gpu metal rendering (#22611)" (flutter/engine#22777) * 644dd65 Temporarily reduce e2e test matrix to stop flaky web engine builds (flutter/engine#22824) * 105004d Stop using the List constructor. (flutter/engine#22793) * 34f49a1 Roll Dart SDK from 5acaa5f14b03 to cfaa7606cbf5 (2 revisions) (flutter/engine#22827) * 6c8342f Revert "Fix race condition in key event handling on Android (#22658)" (flutter/engine#22823) * 1c2a8f9 Roll Skia from c504ecda03b8 to 9443d58af292 (16 revisions) (flutter/engine#22828) * b63e911 Better handle image codec instantiation failure (flutter/engine#22809) * 1358fda Generate Maven metadata files for engine artifacts (flutter/engine#22685) * 079c669 Generate gen_snapshot_armv7 and gen_snapshot_arm64 (flutter/engine#22818) * fcbfa9f Split AOT Engine Runtime (flutter/engine#22624) * 24d289e Roll Fuchsia Linux SDK from W14Qninrb... to M_8svVndh... (flutter/engine#22842) * 78b567f Reland: "Fix race condition in key event handling on Android (#22658)" (flutter/engine#22834) * 7d32cea (MacOS) Add FlutterGLCompositor with support for rendering multiple layers (flutter/engine#22782) * a713174 Roll Skia from 9443d58af292 to c7112edbe0f4 (10 revisions) (flutter/engine#22839) * bee352c Roll Dart SDK from cfaa7606cbf5 to 97cfd05b3cb3 (2 revisions) (flutter/engine#22840) * e5f510f [web] Fix event transform between mousedown/up due to mouse move event (flutter/engine#22813) * 04b98dc Roll Fuchsia Mac SDK from Z_-ciOYM9... to DRN4P3zbe... (flutter/engine#22841) * 0e3b2cf Roll Skia from c7112edbe0f4 to d39aec0e40ec (17 revisions) (flutter/engine#22844) * e71c6f4 leaving only html tests (flutter/engine#22846) * 3773835 Make CkPicture resurrectable (flutter/engine#22807) * bd394a1 Roll Skia from d39aec0e40ec to 38921cafe1bb (7 revisions) (flutter/engine#22847) * 66f44c6 Roll Dart SDK from 97cfd05b3cb3 to a37a4d42e53d (4 revisions) (flutter/engine#22849) * bdadaad Add delayed event delivery for Linux. (flutter/engine#22577) * 48befc5 More rename from GPU thread to raster thread (flutter/engine#22819) * 9b1b7f6 Roll Skia from 38921cafe1bb to abcc1ecdfd0c (8 revisions) (flutter/engine#22851) * 14a6fd9 Fix NPE when platform plugin delegate is null (flutter/engine#22852)

GaryQian added affects: engine Work in progress (WIP) Not ready (yet) for review! labels Nov 20, 2020

googlebot added the cla: yes label Nov 20, 2020

GaryQian marked this pull request as ready for review November 24, 2020 05:26

GaryQian requested a review from chinmaygarde November 24, 2020 05:26

GaryQian changed the title ~~[WIP] Split AOT Engine Runtime~~ Split AOT Engine Runtime Nov 24, 2020

GaryQian requested a review from xster November 24, 2020 19:48

chinmaygarde reviewed Nov 24, 2020

View reviewed changes

chinmaygarde added Work in progress (WIP) Not ready (yet) for review! and removed Work in progress (WIP) Not ready (yet) for review! labels Nov 24, 2020

GaryQian force-pushed the splitaotruntime branch from ee15bb6 to 9a97d9a Compare November 25, 2020 02:50

GaryQian removed the Work in progress (WIP) Not ready (yet) for review! label Nov 25, 2020

GaryQian force-pushed the splitaotruntime branch from 7fa36ce to 047d991 Compare November 25, 2020 06:22

GaryQian requested a review from chinmaygarde November 25, 2020 07:18

xster approved these changes Nov 25, 2020

View reviewed changes

GaryQian mentioned this pull request Nov 25, 2020

Add FlutterPlayStoreSplitApplication for simpler opt in to Split AOT #22752

Merged

chinmaygarde suggested changes Nov 25, 2020

View reviewed changes

GaryQian requested a review from chinmaygarde December 2, 2020 06:48

GaryQian mentioned this pull request Dec 2, 2020

☂️ Deferred Components: Build and load split AOT Android App Bundles flutter/flutter#57617

Closed

18 tasks

GaryQian added 6 commits December 1, 2020 23:52

INitial code

d6d02a9

Update runtime controller API to new engine API

59ff685

Compiles

3a15e42

Remove deferred load handler from init

a7026f9

Refactor handler into platformconfiguration

ebaa564

Remove dartdeferredloadhandler

913a39e

GaryQian force-pushed the splitaotruntime branch from 9ce88d9 to 8972ff2 Compare December 2, 2020 07:53

chinmaygarde approved these changes Dec 2, 2020

View reviewed changes

GaryQian added 8 commits December 2, 2020 11:00

Remove dlfcn, revert accidentqal comment changes

81c927a

Revert accidental comment changes

Address comments, fix playstoredynamicfeaturemanager:

2e90525

Use native_library

1442b6e

Pass as mapping cleanup Docs More docs Cleanup

Formatting

a43817f

SymbolMapping

Store DartSnapshots

f027452

Use unordered_set

Add failure test

e3655e2

Begin testing work Loading split aot tests part 1 Passing tests, but flaky due to race

Docs, fix test flakiness, use general Mapping

1f62be4

Docs for testing changes Licenses

Clean imports

c9ca40b

Additional cleanup

GaryQian force-pushed the splitaotruntime branch from 56cd833 to c9ca40b Compare December 2, 2020 19:00

Address nits

df145b9

GaryQian 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 Dec 2, 2020

fluttergithubbot merged commit fcbfa9f into flutter:master Dec 2, 2020

GaryQian mentioned this pull request Dec 2, 2020

Support engine loading dart loading units - AOT split flutter/flutter#62229

Closed

aam mentioned this pull request Dec 3, 2020

flutter/engine DartIsolateTest_ValidLoadingUnitSucceeds_Test is flaky flutter/flutter#71661

Closed

engine-flutter-autoroll mentioned this pull request Dec 4, 2020

Roll Engine from 20caf5496951 to 14a6fd97ca12 (67 revisions) flutter/flutter#71699

Merged

engine-flutter-autoroll mentioned this pull request Dec 4, 2020

Roll Engine from 20caf5496951 to 6a5971dac761 (70 revisions) flutter/flutter#71701

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 4, 2020

fcbfa9f Split AOT Engine Runtime (flutter/engine#22624)

d253ce7

cbracken pushed a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 4, 2020

fcbfa9f Split AOT Engine Runtime (flutter/engine#22624)

1c91aed

		@@ -4,6 +4,8 @@

		#include "flutter/runtime/runtime_controller.h"

		#include <sstream>

Split AOT Engine Runtime #22624

Split AOT Engine Runtime #22624

Uh oh!

Conversation

GaryQian commented Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

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!

chinmaygarde commented Nov 24, 2020

Uh oh!

GaryQian commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

chinmaygarde 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!

GaryQian commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde 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!

aam commented Dec 3, 2020

Uh oh!

Uh oh!

GaryQian commented Nov 20, 2020 •

edited

Loading

GaryQian commented Nov 24, 2020 •

edited

Loading

GaryQian commented Dec 2, 2020 •

edited

Loading