Provide dart vm initalize isolate callback so that children isolates belong to parent's isolate group. #9888

aam · 2019-07-17T17:47:20Z

Without this callback each child isolate is created as a separate isolate group, and Dart VM won't be able to provide performance savings for spawning of those.

Introduce is_root_isolate property on isolate's IsolateData. This is needed so that when isolate shutdown request is received it knows whether it needs to free IsolateData(if it's a child isolate), or leave it for IsolateGroup shutdown to clean it up(for root isolate).

cc @mkustermann

…belong to parent's isolate group. Without this callback each child isolate is created as a separate isolate group, and Dart VM won't be able to provide performance savings for spawning of those.

mkustermann · 2019-07-23T12:07:07Z

runtime/dart_isolate.cc

-          )
-      );
+      std::make_unique<
+          std::shared_ptr<DartIsolate>>(std::make_shared<DartIsolate>(


This makes a new DartIsolate object, which will need to be freed at some point. I assume the lifetime of this object is limited to the lifetime of this particular isolate.

If so, we need to add a isolate-specific cleanup callback in Dart_Initialize, namely:

Dart_InitializeParams params = {}; ... params.cleanup_isolate = ...

Though we have to be careful to not free the std::shared_ptr<DartIsolate> object of the root / ui isolate, because this pointer is used as the isolate group data and therefore has to stay alive as long as there are any isolates.

We should ensure isolate creation, deletion cycles work, even in asan (I guess flutter has an asan bot?)

If so, we need to add a isolate-specific cleanup callback in Dart_Initialize

Done

We should ensure isolate creation, deletion cycles work, even in asan (I guess flutter has an asan bot?)

I don't think flutter has asan bots: these are the bots used by flutter engine: https://ci.chromium.org/p/flutter/g/engine/console

mkustermann · 2019-07-23T12:22:07Z

runtime/dart_isolate.cc

    Dart_IsolateFlags* flags,
    std::shared_ptr<DartIsolate>* parent_embedder_isolate,
    char** error) {
+  TRACE_EVENT0("flutter", "DartIsolate::DartIsolateGroupCreateCallback");


(I cannot comment on the line blow due to github review, but this comment belongs to line 664)

The code below should be unreachable or should error if there is no support for Isolate.spawnUri, since every Isolate.spawn will cause us to use the initialize_isolate callback. The only other isolate groups created are for service/kernel isolate.

That being said, I don't know how the "multiple flutter engine shell" feature works, though I assume they will not use this code either.

Therefore I think the call to CreateDartVMAndEmbedderObjectPair(..., is_root_isolate=false) should be replaced with an error (or unreachable).

Not sure I understand. This DartIsolateGroupCreateCallback is called on isolate group creation. That will be called two times(service isolate group, main isolate group) and for main isolate the call CreateDartVMAndEmbedderObjectPair(..., is_root_isolate=false) will be triggered, right?

The DartIsolateGroupCreateCallback is called from the VM, if the VM wants to create new isolate groups. This happens in these cases:

VM wants to create service-isolate

Isolate.spawn (only if isolate groups are disabled, if they are enabled then only initialize_isolate callback is used, not create_group)

Isolate.spawnUri (not supported by flutter I suppose)

VM wants to create kernel-isolate (not used by flutter I suppose)

=> If isolate groups are enabled, the only thing this callback will ever get to do is starting the service isolate.

The actual main UI isolate is created by the embedder itself and doesn't go through any callbacks. I believe this is done in runtime/runtime_controller.cc

That being said, I think you can ignore my original comment: As discussed offline we want to have the flexibility to flip --enable-isolate-groups on/off for some time, so we keep this code.

Thank you for detailed explanation, makes sense.

mkustermann · 2019-07-23T12:33:27Z

runtime/dart_isolate.cc

+  auto* isolate_data = static_cast<std::shared_ptr<DartIsolate>*>(
+      Dart_IsolateGroupData(isolate));
+  if (isolate_data->get() != embedder_isolate->get()) {
+    *error = strdup("Unexpected isolate data during initialization.");


Can this ever happen? Should this be UNREACHABLE()?

I don't think that should normally happen. I'm not sure flutter engine folks use UNREACHABLE - from what I can tell normal pattern is to use FML_DLOG(ERROR) and exit current function.

@chinmaygarde

I wasn't too sure so I probably added a few checks that are too paranoid. If you feel this is unnecessary, please get rid of it (or add a DCHECK in its place to document that we thought of this). The engine doesn't use UNREACHABLE.

Switched to FML_DCHECK

cbracken · 2019-08-05T18:28:47Z

@aam do you plan to continue with this PR?

mkustermann

lgtm with comments - though it wouldn't hurt if someone from the engine team could also take a look

mkustermann · 2019-08-26T19:19:55Z

runtime/dart_isolate.cc

    Dart_IsolateFlags* flags,
    std::shared_ptr<DartIsolate>* parent_embedder_isolate,
    char** error) {
+  TRACE_EVENT0("flutter", "DartIsolate::DartIsolateGroupCreateCallback");


The DartIsolateGroupCreateCallback is called from the VM, if the VM wants to create new isolate groups. This happens in these cases:

VM wants to create service-isolate

Isolate.spawn (only if isolate groups are disabled, if they are enabled then only initialize_isolate callback is used, not create_group)

Isolate.spawnUri (not supported by flutter I suppose)

VM wants to create kernel-isolate (not used by flutter I suppose)

=> If isolate groups are enabled, the only thing this callback will ever get to do is starting the service isolate.

The actual main UI isolate is created by the embedder itself and doesn't go through any callbacks. I believe this is done in runtime/runtime_controller.cc

That being said, I think you can ignore my original comment: As discussed offline we want to have the flexibility to flip --enable-isolate-groups on/off for some time, so we keep this code.

mkustermann · 2019-08-26T19:39:42Z

runtime/dart_isolate.cc

+                                /* ui= */ nullptr,
+                                /* io= */ nullptr);
+
+  std::unique_ptr<std::shared_ptr<DartIsolate>> embedder_isolate =


Consider typing this just like in CreateRootIsolate:

auto embedder_isolate = std::make_unique<...>(...);

mkustermann · 2019-08-26T19:41:36Z

runtime/dart_isolate.cc

+  }
+
+  auto* raw_embedder_isolate = reinterpret_cast<std::shared_ptr<DartIsolate>*>(
+      Dart_CurrentIsolateGroupData());


Can this be a static_cast?

raw_embedder_isolate -> root_embedder_isolate?

mkustermann · 2019-08-26T19:48:50Z

runtime/dart_isolate.h

      char** error);

+  static bool InitializeIsolate(std::shared_ptr<DartIsolate> embedder_isolate,
+                                Dart_Isolate& isolate,


Dart_Isolate is already a pointer itself, there's no need for making this a reference with &.

mkustermann · 2019-08-26T19:50:32Z

runtime/dart_isolate.cc

+    std::shared_ptr<DartIsolate>* isolate_data) {
+  TRACE_EVENT0("flutter", "DartIsolate::DartIsolateCleanupCallback");
+
+  if (!isolate_data->get()->IsRootIsolate()) {


This could be written a bit shorter as (*isolate_data)->IsRootIsolate()

mkustermann · 2019-08-26T20:04:54Z

runtime/dart_isolate.cc

             false,                       // is root isolate
             error                        // error
             )
      .first;


When falling back to --no-enable-isolate-groups line 665 passes /*is_root_isolate=*/false. This can cause a double free (I think):

Isolate.spawn() creates a new isolate, invoking the code on line 658.

That isolate dies at some point

The cleanup_isolate callback is called. Since !isolate_data->get()->IsRootIsolate() we free the DartIsolate object.

The cleanup_group callback is called. We delete the same object again

The reason this happens is that, is_root_isolate is different from is_group_isolate if --no-enable-isolate-groups. (Maybe an extra boolean is_isolate_group_root could be used for the memory management)

mkustermann · 2019-08-26T20:09:57Z

runtime/dart_isolate.cc

+    return false;
+  }
+
+  *child_callback_data = embedder_isolate.release();


Consider adding the same comment as below regarding the release()

chinmaygarde · 2019-09-03T18:23:08Z

runtime/dart_isolate.cc

                         fml::closure isolate_create_callback,
-                         fml::closure isolate_shutdown_callback)
+                         fml::closure isolate_shutdown_callback,
+                         const bool is_root_isolate,


Nit: Why const if this passed by value?

chinmaygarde · 2019-09-03T18:31:12Z

runtime/dart_isolate.cc

+  auto* isolate_data = static_cast<std::shared_ptr<DartIsolate>*>(
+      Dart_IsolateGroupData(isolate));
+  if (isolate_data->get() != embedder_isolate->get()) {
+    *error = strdup("Unexpected isolate data during initialization.");


I wasn't too sure so I probably added a few checks that are too paranoid. If you feel this is unnecessary, please get rid of it (or add a DCHECK in its place to document that we thought of this). The engine doesn't use UNREACHABLE.

aam · 2019-09-04T15:51:25Z

Thank you for the review!

…solates belong to parent's isolate group. (flutter/engine#9888)

git@github.com:flutter/engine.git/compare/e7f9ef6aa0b9...cd92039 git log e7f9ef6..cd92039 --no-merges --oneline 2019-09-04 mouad.debbar@gmail.com Handle new navigation platform messages (flutter/engine#11880) 2019-09-04 shihaohong@google.com Android 10+ View.getSystemGestureExclusionRects (flutter/engine#11451) 2019-09-04 aukwat@gmail.com Fix deleting Thai vowel bug on iOS (skip string range sanitization) (flutter/engine#11807) 2019-09-04 bkonyi@google.com Roll src/third_party/dart 4bd13a74d9..c3db2e3ee0 (4 commits) 2019-09-04 skia-flutter-autoroll@skia.org Roll src/third_party/skia c2dc9c864844..e7366841663b (2 commits) (flutter/engine#11878) 2019-09-04 iska.kaushik@gmail.com [flutter_runner] Add common libs to the test far (flutter/engine#11875) 2019-09-04 bkonyi@google.com Roll src/third_party/dart fd27e795ba..4bd13a74d9 (5 commits) 2019-09-04 aam@google.com Provide dart vm initalize isolate callback so that children isolates belong to parent's isolate group. (flutter/engine#9888) 2019-09-04 stuartmorgan@google.com Add style guide and formatting information (flutter/engine#11669) 2019-09-04 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from BR_-u... to LKWtB... (flutter/engine#11874) 2019-09-04 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from dRCdb... to m-hNV... (flutter/engine#11873) 2019-09-04 skia-flutter-autoroll@skia.org Roll src/third_party/skia 77743492418e..c2dc9c864844 (22 commits) (flutter/engine#11872) 2019-09-04 iska.kaushik@gmail.com Add a sample unit test target to flutter runner (flutter/engine#11847) 2019-09-04 iska.kaushik@gmail.com Support building standalone far packages with autogenerating manigests (flutter/engine#11849) 2019-09-04 bkonyi@google.com Roll src/third_party/dart 622747502e..fd27e795ba (9 commits) 2019-09-04 bkonyi@google.com Roll src/third_party/dart 54fdd559d8..622747502e (1 commits) 2019-09-04 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from KqidK... to BR_-u... (flutter/engine#11845) 2019-09-04 skia-flutter-autoroll@skia.org Roll src/third_party/skia 452ce044f0db..77743492418e (12 commits) (flutter/engine#11837) 2019-09-04 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from -kKi5... to dRCdb... (flutter/engine#11840) 2019-09-04 bkonyi@google.com Roll src/third_party/dart 36985859e4..54fdd559d8 (65 commits) 2019-09-04 chinmaygarde@google.com Minor tweaks to the Doxygen theme. (flutter/engine#11576) 2019-09-04 matthew-carroll@users.noreply.github.com Updated API usage in scenario app by deleting unnecessary method. (flutter/engine#11844) 2019-09-03 garyq@google.com Fix RTL justification alignment with newline (flutter/engine#11842) 2019-09-03 matthew-carroll@users.noreply.github.com Rename first frame method and notify FlutterActivity when full drawn (#38714 #36796). (flutter/engine#11357) 2019-09-03 skia-flutter-autoroll@skia.org Roll src/third_party/skia 8050d94ae227..452ce044f0db (1 commits) (flutter/engine#11834) 2019-09-03 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from a8wlq... to KqidK... (flutter/engine#11833) 2019-09-03 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from pVy4h... to -kKi5... (flutter/engine#11832) 2019-09-03 skia-flutter-autoroll@skia.org Roll src/third_party/skia 033c4014b788..8050d94ae227 (2 commits) (flutter/engine#11831) 2019-09-03 skia-flutter-autoroll@skia.org Roll src/third_party/skia 94c66475560d..033c4014b788 (2 commits) (flutter/engine#11830) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC franciscojma@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

…solates belong to parent's isolate group. (flutter#9888)" This reverts commit e6620dd.

…solates belong to parent's isolate group. (#9888)" (#12327) This reverts commit e6620dd.

…ildren isolates belong to parent's isolate group. (flutter#9888)" (flutter#12327)" This reverts commit 23835f5.

* Revert "Revert "Provide dart vm initalize isolate callback so that children isolates belong to parent's isolate group. (#9888)" (#12327)" * Ensure that when isolate shuts down it calls isolate_data, rather than isolage_group_data callback.

Provide dart vm initalize isolate callback so that children isolates …

de4db7f

…belong to parent's isolate group. Without this callback each child isolate is created as a separate isolate group, and Dart VM won't be able to provide performance savings for spawning of those.

googlebot added the cla: yes label Jul 17, 2019

Fix formatting, add traces

5a545f8

mkustermann reviewed Jul 23, 2019

View reviewed changes

aam added 5 commits August 9, 2019 11:56

Merge branch 'master' into provide-initialize-isolate-callback

0819251

Cleanup isolate data

5a8ea6d

Igore isolate shutdown for service isolate

4905403

Reformat

8553cbf

window() check is not reliable, introduce is_root_isolate property

e40a253

mkustermann approved these changes Aug 26, 2019

View reviewed changes

aam added 2 commits August 26, 2019 21:24

Merge branch 'master' into provide-initialize-isolate-callback

48150b3

Introduce is_group_root_isolate, fix DartIsolateCleanupCallback type

553407e

aam requested a review from chinmaygarde August 28, 2019 23:58

aam added 2 commits August 28, 2019 22:03

Merge branch 'master' into provide-initialize-isolate-callback

d7fb798

Run ci/format.sh

6bfaa05

chinmaygarde approved these changes Sep 3, 2019

View reviewed changes

aam added 2 commits September 4, 2019 08:41

Merge branch 'master' into provide-initialize-isolate-callback

8dbd38c

Get rid of consts, introduce FML_DCHECK

0dfecf8

aam merged commit e6620dd into flutter:master Sep 4, 2019

aam deleted the provide-initialize-isolate-callback branch September 4, 2019 17:10

engine-flutter-autoroll mentioned this pull request Sep 4, 2019

Roll engine e7f9ef6aa0b9..cd920398e40e (29 commits) flutter/flutter#39854

Merged

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

e6620dd Provide dart vm initalize isolate callback so that children i…

3ecfa49

…solates belong to parent's isolate group. (flutter/engine#9888)

iskakaushik pushed a commit to iskakaushik/engine that referenced this pull request Sep 17, 2019

Revert "Provide dart vm initalize isolate callback so that children i…

c7bbbdf

…solates belong to parent's isolate group. (flutter#9888)" This reverts commit e6620dd.

iskakaushik mentioned this pull request Sep 17, 2019

Revert "Provide dart vm initalize isolate callback so that children i… #12327

Merged

iskakaushik added a commit that referenced this pull request Sep 17, 2019

Revert "Provide dart vm initalize isolate callback so that children i…

23835f5

…solates belong to parent's isolate group. (#9888)" (#12327) This reverts commit e6620dd.

aam added a commit to aam/engine that referenced this pull request Oct 31, 2019

Revert "Revert "Provide dart vm initalize isolate callback so that ch…

e3fb7c2

…ildren isolates belong to parent's isolate group. (flutter#9888)" (flutter#12327)" This reverts commit 23835f5.

aam mentioned this pull request Nov 8, 2019

Reland children isolates sharing isolate group change. #13758

Merged

Provide dart vm initalize isolate callback so that children isolates belong to parent's isolate group. #9888

Provide dart vm initalize isolate callback so that children isolates belong to parent's isolate group. #9888

Uh oh!

Conversation

aam commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

cbracken commented Aug 5, 2019

Uh oh!

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

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 Sep 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aam commented Jul 17, 2019 •

edited

Loading