Plumbing refactor to allow the usage of Dart_CreateIsolateInGroup #23549

gaaclarke · 2021-01-08T23:09:15Z

Description

This should be a noop refactor that plumbs spawning all the way down to the dart isolate so that it can use Dart_CreateIsolateInGroup if necessary.

Related Issues

flutter/flutter#72025

Tests

I added the following tests:

This is just a refactor, no new or changed functionality, existing tests should cover it.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

flutter-dashboard · 2021-01-09T00:19:26Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

chinmaygarde

This breaks the existing behavior that terminating the root isolate also terminates any isolates spawned by it. It should be possible to create distinct isolate groups with shared resources. Also, what resources are being duplicated during isolate creation?

xster · 2021-01-11T17:09:26Z

runtime/dart_isolate.h

+  ///            possible with the caller DartIsolate.  This allows them to
+  ///            occupy less memory together.
+  /// @attention Only certain setups can take advantage of the most savings
+  ///            currently, release builds currently.


@mkustermann where do you estimate you guys will land by the next Flutter stable?

xster

LGTM. Please wait for @chinmaygarde to review.

xster · 2021-01-11T17:11:53Z

runtime/dart_isolate.h

+  //----------------------------------------------------------------------------
+  /// @brief     Creates a running DartIsolate who shares as many resources as
+  ///            possible with the caller DartIsolate.  This allows them to
+  ///            occupy less memory together.


Describe their and their components' lifecycles. Describe restrictions (i.e. they must be from the same aot).

xster · 2021-01-11T17:14:46Z

shell/common/engine.h

+  //----------------------------------------------------------------------------
+  /// @brief      Create a Engine that shares as many resources as
+  ///             possible with the calling Engine such that together
+  ///             they occupy less memory.


Same comment for doc. Can use more details.

Done, mostly linked to other documentation.

xster · 2021-01-11T17:25:17Z

shell/common/shell.h

 public:
  template <class T>
  using CreateCallback = std::function<std::unique_ptr<T>(Shell&)>;
+  typedef std::function<std::unique_ptr<Engine>(


Kinda does the same thing as the above. Can we make the names sound similar like EngineCreateCallback or something?

xster · 2021-01-11T17:25:57Z

shell/common/shell.h

+      const CreateCallback<PlatformView>& on_create_platform_view,
+      const CreateCallback<Rasterizer>& on_create_rasterizer,
+      DartVMRef vm,
+      const EngineMaker& engine_maker);


Same here. Could this be something like on_create_engine for consistency?

good idea, done

gaaclarke · 2021-01-11T17:31:17Z

what resources are being duplicated during isolate creation?

The isolate group, so the heap and GC and the loaded executable.

This PR isn't making any functional changes. The top level APIs behaves the same. A spawned flutter engine and its isolate should live beyond its spawning engine, this matches the semantics of isolate groups and matches how engine groups will work.

I've added a comment to where the functional change will be made, conditionally we will be calling Dart_CreateIsolateInGroup instead of Dart_CreateIsolateGroup. I avoided turning it fully on yet until I can finish all the phase 1 work. I brought this over because this refactor makes other phase 1 work possible. You can see it in action in the prototype https://github.com/gaaclarke/engine/tree/spawn-engine

edit:

This breaks the existing behavior that terminating the root isolate also terminates any isolates spawned by it

No existing APIs have have had their semantics changed by this PR or any of the others. This is a whole different API so it's not a breaking change.

chinmaygarde · 2021-01-11T19:30:22Z

No existing APIs have have had their semantics changed by this PR or any of the others.

That is not what I meant. Existing Flutter apps expect that isolates spawned by their root isolate will be terminated with the root isolate is collected. This will not be the case for isolates spawned using this new call. This deviation from expected behavior is the breaking change I meant.

The isolate group, so the heap and GC and the loaded executable.

The heap should be per isolate and not the group. The GC and other VM components are owned by the Dart VM instance and not the isolate group. The loaded executable code should be handled by libdl and reference counted already. So, in bytes, what are the resources you are saving by using this vs the old APIs.

gaaclarke · 2021-01-11T21:41:02Z

@chinmaygarde This is a whole new API so there isn't any expectations. The behavior you are describing is definitive to the work we are doing.

The heap isn't per isolate, it is per group although there are safeguards that disallow them from directly accessing memory allocated to a different isolate (there is plans to change that). With respect to the loaded program it isn't a problem with occupied memory but the time to create a new isolate. Loading the executable takes a sizable chunk of the time to create a new isolate, in this case it is just wasted effort.

All this work is outlined in this issue: dart-lang/sdk#36097 I think there is a design doc floating around, too.

…Group when applicable.

xster · 2021-01-12T19:40:44Z

runtime/dart_isolate.h

      std::unique_ptr<IsolateConfiguration> isolate_configration,
-      std::shared_ptr<VolatilePathTracker> volatile_path_tracker);
+      std::shared_ptr<VolatilePathTracker> volatile_path_tracker,
+      const DartIsolate* spawning_isolate = nullptr);


if you change the signature, update the method doc

xster · 2021-01-12T19:41:26Z

runtime/dart_isolate.h

+  /// @brief     Creates a running DartIsolate who shares as many resources as
+  ///            possible with the caller DartIsolate.  This allows them to
+  ///            occupy less memory together and to be created faster.
+  /// @details   Shared components will be destroyed when the last DartIsolate


when the last DartIsolate among the group. Might be misunderstood as if you make 1, 2, 3, 4, shared components will be destroyed if 4 is destroyed.

xster · 2021-01-12T19:42:56Z

runtime/dart_isolate.h

+      const DartIsolate* spawning_isolate = nullptr);
+
+  //----------------------------------------------------------------------------
+  /// @brief     Creates a running DartIsolate who shares as many resources as


Creates a new running DartIsolate from the current DartIsolate

I don't think that is much of an improvement. Hopefully the other edits cover the information you are trying to convey with that.

xster · 2021-01-12T19:43:39Z

runtime/dart_isolate.h

+  /// @details   Shared components will be destroyed when the last DartIsolate
+  ///            is destroyed.  SpawnIsolate can only be used to create
+  ///            DartIsolates whose executable code is shared with the calling
+  ///            DartIsolate.


Does the current DartIsolate need to be in any particular states before calling this?

I'm not sure of the hard requirements of Dart_CreateIsolateInGroup. It isn't spelled out in its documentation.

xster · 2021-01-12T19:46:45Z

runtime/dart_isolate.h

+  ///            DartIsolate.
+  /// @attention Only certain setups can take advantage of the most savings
+  ///            currently, AOT specifically.
+  /// @return    A running DartIsolate.


A new, running DartIsolate.

xster · 2021-01-12T19:48:14Z

runtime/dart_isolate.h

+  ///            DartIsolates whose executable code is shared with the calling
+  ///            DartIsolate.
+  /// @attention Only certain setups can take advantage of the most savings
+  ///            currently, AOT specifically.


Add the same disclosure for why it's a weak pointer, who owns it, performance considerations etc.

xster · 2021-01-12T19:52:03Z

shell/common/engine.h

+  ///             possible with the calling Engine such that together
+  ///             they occupy less memory and be created faster.
+  /// @details    This method ultimately calls DartIsolate::SpawnIsolate to make
+  ///             sure resources are shared.


Same doc comments. Does the current engine need to be in any state before calling this? Mention that it's a new Engine.

xster · 2021-01-12T20:18:31Z

shell/common/shell.h

+  /// @details    This version of Create can take in a running DartVMRef and a
+  ///             function that defines how the Shell's Engine should be
+  ///             created.
+  /// @param[in]  vm             A running DartVMRef where this Shell's Dart


Do you want to briefly describe how one typically gets a DartVMRef?

I don't know if we should point to DartVMRef::Create, it's pretty straightforward

chinmaygarde

Mostly looks good. Please add tests for Engine::Spawn to shell_unittests and for DartIsolate::SpawnIsolate to runtime_unittests.

gaaclarke · 2021-01-12T22:35:05Z

Mostly looks good. Please add tests for Engine::Spawn to shell_unittests and for DartIsolate::SpawnIsolate to runtime_unittests.

A dependent PR has a unit test for Engine::Spawn #23603 SpawnIsolate has test coverage via the golden image test for spawning engines and the FlutterEngineTests of objective-c. I'll look into seeing if I can get a unit test directly on SpawnIsolate.

chinmaygarde · 2021-01-12T22:38:18Z

I'll look into seeing if I can get a unit test directly on SpawnIsolate.

You should be able to base the test on DartIsolateTest::RootIoslateCreationAndShutdown. Take that isolate and spawn another one using that as the base.

SGTM about the test in the dependent PR.

…roup (flutter/engine#23549)

* 51dd6aa [web] Enable the new rich paragraph implementation (flutter/engine#23162) * 0f9fc3d Roll Skia from 7cf3addb1bd8 to 93c2d81f199a (1 revision) (flutter/engine#23614) * a1e7424 [dart-runner] Avoid calling Destroy on nullptr (flutter/engine#23608) * 846d958 Windows textures: Add placeholder flutter_texture_registrar.h (flutter/engine#23623) * 55afc18 Roll Dart SDK from 7fcbd388b620 to ef8bf7f0a667 (5 revisions) (flutter/engine#23628) * d2b8154 [canvaskit] apply invser scale on the left (flutter/engine#23550) * be1a3c0 Roll Fuchsia Linux SDK from UB6RsTbdU... to FfWbbB4r8... (flutter/engine#23633) * f743c89 Roll Skia from 93c2d81f199a to 9fd75e96d712 (29 revisions) (flutter/engine#23635) * cf42dbe Roll wuffs to google/wuffs@c86add2 (flutter/engine#23607) * f1278d0 Link SkShaper/SkParagraph into the engine by default (flutter/engine#23626) * fb56b4b Android deeplink sends "path + query" instead of just path (flutter/engine#23561) * 22bb891 Plumbing refactor to allow the usage of Dart_CreateIsolateInGroup (flutter/engine#23549) * 20991a5 Add accessibility suport to Linux shell. (flutter/engine#19634) * 145922b Roll Dart SDK from ef8bf7f0a667 to 636ff0ec97e0 (1 revision) (flutter/engine#23639) * 176ae6e Roll Fuchsia Mac SDK from oll0Dgp9o... to JSzm8D59u... (flutter/engine#23641) * d2320a8 Roll Dart SDK from 636ff0ec97e0 to d3d7b77e8165 (1 revision) (flutter/engine#23642) * 8bdd099 Roll Skia from 9fd75e96d712 to 38ca513408d1 (1 revision) (flutter/engine#23643) * cc572e1 Roll Skia from 38ca513408d1 to be2a8614c5d6 (2 revisions) (flutter/engine#23644) * e9383a0 Roll Dart SDK from d3d7b77e8165 to 010633edc631 (1 revision) (flutter/engine#23645) * 9d6ed8b Roll Fuchsia Linux SDK from FfWbbB4r8... to BUsKF6z4t... (flutter/engine#23646) * 6d55dd4 Roll Dart SDK from 010633edc631 to 724d9e5e7d71 (1 revision) (flutter/engine#23647) * 299f081 Roll Skia from be2a8614c5d6 to 0d7de6bc9ac3 (1 revision) (flutter/engine#23648) * 0ec99cf Roll Fuchsia Mac SDK from JSzm8D59u... to BsUY1yjWh... (flutter/engine#23650) * 859494f Revert "[web] Enable the new rich paragraph implementation (#23162)" (flutter/engine#23651) * 648dae9 Roll Skia from 0d7de6bc9ac3 to 92969f265686 (7 revisions) (flutter/engine#23652)

…utter#23549) Did the plumbing refactor that allows us to call Dart_CreateIsolateInGroup when applicable.

googlebot added the cla: yes label Jan 8, 2021

gaaclarke force-pushed the lfe-isolate-groups branch 2 times, most recently from d255c92 to 5a280fe Compare January 8, 2021 23:19

gaaclarke changed the title ~~Started creating spawned isolates in isolate groups~~ Did plumbing refactor to allow the usage of Dart_CreateIsolateInGroup Jan 8, 2021

gaaclarke force-pushed the lfe-isolate-groups branch from 5a280fe to 3d179a5 Compare January 8, 2021 23:25

gaaclarke changed the title ~~Did plumbing refactor to allow the usage of Dart_CreateIsolateInGroup~~ Plumbing refactor to allow the usage of Dart_CreateIsolateInGroup Jan 8, 2021

gaaclarke force-pushed the lfe-isolate-groups branch from 3d179a5 to 816caac Compare January 8, 2021 23:48

gaaclarke marked this pull request as ready for review January 9, 2021 00:19

gaaclarke requested review from chinmaygarde and xster January 9, 2021 00:19

gaaclarke mentioned this pull request Jan 9, 2021

Share font collections between multiple engines flutter/flutter#72024

Closed

chinmaygarde reviewed Jan 9, 2021

View reviewed changes

xster reviewed Jan 11, 2021

View reviewed changes

gaaclarke added 2 commits January 11, 2021 15:26

Did the plumbing refactor that allows us to call Dart_CreateIsolateIn…

29d2a61

…Group when applicable.

responded to xiao's feedback

1e15db8

gaaclarke force-pushed the lfe-isolate-groups branch from 82dfe3d to 1e15db8 Compare January 11, 2021 23:26

xster approved these changes Jan 12, 2021

View reviewed changes

updated docstrings

dc585c6

chinmaygarde reviewed Jan 12, 2021

View reviewed changes

added unit test for DartIsolate::SpawnIsolate

f01c330

gaaclarke merged commit 22bb891 into flutter:master Jan 13, 2021

engine-flutter-autoroll mentioned this pull request Jan 13, 2021

Roll Engine from 10cee6abcb35 to 145922bd83e5 (14 revisions) flutter/flutter#73827

Closed

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

22bb891 Plumbing refactor to allow the usage of Dart_CreateIsolateInG…

b7bf9c1

…roup (flutter/engine#23549)

engine-flutter-autoroll mentioned this pull request Jan 13, 2021

Roll Engine from 10cee6abcb35 to d2320a8fb7a0 (16 revisions) flutter/flutter#73834

Closed

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

22bb891 Plumbing refactor to allow the usage of Dart_CreateIsolateInG…

ffc7329

…roup (flutter/engine#23549)

engine-flutter-autoroll mentioned this pull request Jan 13, 2021

Roll Engine from 10cee6abcb35 to cc572e1b8384 (18 revisions) flutter/flutter#73839

Closed

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

22bb891 Plumbing refactor to allow the usage of Dart_CreateIsolateInG…

e53bde6

…roup (flutter/engine#23549)

engine-flutter-autoroll mentioned this pull request Jan 13, 2021

Roll Engine from 10cee6abcb35 to 0ec99cfac245 (23 revisions) flutter/flutter#73880

Closed

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

22bb891 Plumbing refactor to allow the usage of Dart_CreateIsolateInG…

7cdeb11

…roup (flutter/engine#23549)

engine-flutter-autoroll mentioned this pull request Jan 13, 2021

Roll Engine from 10cee6abcb35 to 648dae9d99e7 (25 revisions) flutter/flutter#73886

Merged

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

22bb891 Plumbing refactor to allow the usage of Dart_CreateIsolateInG…

3ad8c46

…roup (flutter/engine#23549)

xster mentioned this pull request Jan 15, 2021

FlutterEngineGroup for Android #23675

Merged

13 tasks

gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Jan 20, 2021

Plumbing refactor to allow the usage of Dart_CreateIsolateInGroup (fl…

b9044c2

…utter#23549) Did the plumbing refactor that allows us to call Dart_CreateIsolateInGroup when applicable.

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

Plumbing refactor to allow the usage of Dart_CreateIsolateInGroup (fl…

24c41aa

…utter#23549) Did the plumbing refactor that allows us to call Dart_CreateIsolateInGroup when applicable.

Plumbing refactor to allow the usage of Dart_CreateIsolateInGroup #23549

Plumbing refactor to allow the usage of Dart_CreateIsolateInGroup #23549

Uh oh!

Conversation

gaaclarke commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

Uh oh!

flutter-dashboard bot commented Jan 9, 2021

Uh oh!

chinmaygarde left a comment

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!

gaaclarke commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde commented Jan 11, 2021

Uh oh!

gaaclarke commented Jan 11, 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!

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 left a comment

Choose a reason for hiding this comment

gaaclarke commented Jan 8, 2021 •

edited

Loading

gaaclarke commented Jan 11, 2021 •

edited

Loading