Started using Dart_CreateInGroup when using spawn on a release build #23782

gaaclarke · 2021-01-20T01:09:00Z

fixes flutter/flutter#72025

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.
The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

gaaclarke · 2021-01-20T01:11:35Z

runtime/dart_isolate.cc

-      });
+
+  IsolateMaker isolate_maker;
+#if FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_PROFILE || \


@mkustermann Are AOT builds still required? It seems like it from dart-lang/sdk#36097 but I thought I would check.

Yes, we're still working on JIT.

gaaclarke · 2021-01-20T01:26:02Z

@chinmaygarde @xster I still have to figure out how to test this. I just wanted to get a read on if the guarding mechanisms for this route are to your liking and get your thoughts on testing.

xster

LGTM so far

xster · 2021-01-20T12:18:29Z

runtime/dart_isolate.cc

+            Dart_IsolateFlags* flags, char** error) {
+      return Dart_CreateIsolateInGroup(
+          /*group_member=*/spawning_isolate->isolate(),
+          /*name=*/"spawn",


What's the impact of this name? Add some comments?

I'm not sure, I moved over the same name selection from the other section.

xster · 2021-01-20T12:19:00Z

runtime/dart_isolate.cc

+    FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_RELEASE
+  if (spawning_isolate) {
+    isolate_maker =  [spawning_isolate](
+            std::shared_ptr<DartIsolateGroupData>* isolate_group_data,


do you need to use this somehow? Add some comments?

chinmaygarde

Other than the nit about error handling (and double checking the error message buffer). Looks good so far.

chinmaygarde · 2021-01-20T21:40:18Z

runtime/dart_isolate.cc

+          (*isolate_group_data)->GetAdvisoryScriptEntrypoint().c_str(),
+          (*isolate_group_data)->GetIsolateSnapshot()->GetDataMapping(),
+          (*isolate_group_data)->GetIsolateSnapshot()->GetInstructionsMapping(),
+          flags, isolate_group_data, isolate_data, error);


In case of error, the error message must be printed to the log (like elsewhere in this TU). Also, can you make sure we don't have to free the error message buffer? The Dart APIs are a bit inconsistent about what to do with the error buffer. Something you have to free it otherwise its a small leak.

The tonic class DartErrorString is taking care of that here, it was a bit harder to follow before. It should be more clear now.

gaaclarke · 2021-01-22T00:52:22Z

runtime/dart_vm.cc

-    "--precompilation",
-};
+static const char* kDartPrecompilationArgs[] = {"--precompilation",
+                                                "--enable-isolate-groups"};


@mkustermann Is there a risk to turning this on for users that don't ever call Dart_CreateIsolateInGroup? I worry about affecting the stability of users who don't use these experimental APIs.

gaaclarke · 2021-01-22T01:52:05Z

@xster Do you know where we can change the LUCI script so we can get runtime_unittests built for release before we run the tests? I can't seem to find it, it isn't with the recipes.

xster · 2021-01-22T13:53:14Z

Looks like we're already building for profile/release and testing for them

https://cs.opensource.google/flutter/recipes/+/master:recipes/engine.py;l=684

xster · 2021-01-22T16:36:52Z

testing/run_tests.py

+  # TODO(tbd): Remove this explicit testing of release builds after lightweight
+  # isolates are supported in JIT mode.
+  release_runtime_out_dir = EnsureReleaseRuntimeTestsAreBuilt()
+  RunEngineExecutable(release_runtime_out_dir, 'runtime_unittests', filter, shuffle_flags)


according to https://cs.opensource.google/flutter/recipes/+/master:recipes/engine.py;l=684?q=file:recipes%2Fengine.py, we might not have to do this

We need run_tests.py able to run locally too. I would have expected the build to be a noop, but looks like the goma check might be happening before it releases it doesn't need to build something.

in that case, wouldn't you do run_tests.py --type=engine --variant=host_release_unopt?

I tried host_release_unopt I removed that after reading through the recipe, I think it's using host_release https://cs.opensource.google/flutter/recipes/+/master:recipes/engine.py;l=683?q=file:recipes%2Fengine.py

I'm going to put a try block around the build so that it can continue to execution on the server, but hopefully still build locally.

xster

If the new unit tests pass on release mode, LGTM.

…build

xster · 2021-01-22T21:21:00Z

Latest LGTM

…e build (flutter/engine#23782)

* 004d8ad Roll Skia from cc6961b9ac5e to 6a272434c2b2 (3 revisions) (flutter/engine#23864) * 7c2da3b reland of flutter/engine#23634 (flutter/engine#23865) * a4f02b7 Share Android surface GrDirectContext (flutter/engine#23798) * 31ef1dd Roll Skia from 6a272434c2b2 to bfc9be0f773f (2 revisions) (flutter/engine#23867) * 6dec57f Remove workarounds now that type promotion accounts for local boolean variables. (flutter/engine#23862) * a787229 Roll Skia from bfc9be0f773f to 3193ff271628 (5 revisions) (flutter/engine#23872) * a242dd8 [web] Fix shadows for arbitrary paths on PhysicalShape (flutter/engine#23830) * 05b4bec Started using Dart_CreateInGroup when using spawn on a release build (flutter/engine#23782)

…lutter#23782)

googlebot added the cla: yes label Jan 20, 2021

gaaclarke commented Jan 20, 2021

View reviewed changes

gaaclarke requested review from chinmaygarde and xster January 20, 2021 01:22

gaaclarke force-pushed the lfe-create-in-group branch from c9f399f to 6019d79 Compare January 20, 2021 01:23

xster reviewed Jan 20, 2021

View reviewed changes

chinmaygarde reviewed Jan 20, 2021

View reviewed changes

gaaclarke force-pushed the lfe-create-in-group branch 2 times, most recently from ec622eb to d9d572c Compare January 22, 2021 00:41

gaaclarke marked this pull request as ready for review January 22, 2021 00:50

gaaclarke commented Jan 22, 2021

View reviewed changes

gaaclarke requested review from chinmaygarde and xster January 22, 2021 00:52

Started using Dart_CreateInGroup when using spawn on a release build.

e3f851b

gaaclarke force-pushed the lfe-create-in-group branch from d9d572c to e3f851b Compare January 22, 2021 01:37

xster reviewed Jan 22, 2021

View reviewed changes

xster approved these changes Jan 22, 2021

View reviewed changes

gaaclarke added 2 commits January 22, 2021 11:45

put try block around building the runtime_unittests

ca7b718

reverted change to run_tests, tests are already run with the release …

4b6376d

…build

gaaclarke mentioned this pull request Jan 22, 2021

remove AOT specific gates for isolate groups flutter/flutter#74520

Closed

updated todos

f3e60f1

gaaclarke 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 Jan 22, 2021

chinmaygarde approved these changes Jan 22, 2021

View reviewed changes

fluttergithubbot merged commit 05b4bec into flutter:master Jan 22, 2021

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

Roll Engine from fdddf8708039 to 05b4bec8b3c1 (8 revisions) flutter/flutter#74540

Merged

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

05b4bec Started using Dart_CreateInGroup when using spawn on a releas…

c3d2eaf

…e build (flutter/engine#23782)

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

Started using Dart_CreateInGroup when using spawn on a release build (f…

db0ffff

…lutter#23782)

Started using Dart_CreateInGroup when using spawn on a release build #23782

Started using Dart_CreateInGroup when using spawn on a release build #23782

Uh oh!

Conversation

gaaclarke commented Jan 20, 2021

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Jan 20, 2021

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!

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!

gaaclarke commented Jan 22, 2021

Uh oh!

xster commented Jan 22, 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!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

xster commented Jan 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants