Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set up secondary isolates with all kernel buffers rather than just one buffer. #6832

Merged
merged 6 commits into from
Nov 14, 2018

Conversation

aam
Copy link
Member

@aam aam commented Nov 13, 2018

This fixes issue with having secondary isolates not being able to use package:flutter classes.

@aam
Copy link
Member Author

aam commented Nov 13, 2018

cc @zanderso

@@ -348,8 +348,15 @@ bool DartIsolate::PrepareForRunningFromKernel(
return false;
}

child_isolate_preparer_ = [mapping](DartIsolate* isolate) {
return isolate->PrepareForRunningFromKernel(mapping);
child_isolate_preparer_ = [this](DartIsolate* isolate) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is thread safe. Instead of referring to this (which could have begun collection on a different thread immediately after spawning a new isolate). Can you can copy the vector and move it into the lambda? The copy is cheap since you are just copying a vector of pointers rather than the mappings themselves.

Copy link
Member

Choose a reason for hiding this comment

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

So just [kernel_buffers = kernel_buffers_](DartIsolate *isolate) and only when last_piece is true.

Copy link
Member

Choose a reason for hiding this comment

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

last_piece is already only true by this point. So just copy the kernel buffers vector and you should be good to go :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done.

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

lgtm

@aam
Copy link
Member Author

aam commented Nov 14, 2018

PTAL - had to add a check to make sure that child_isolate_preparer_ is initialized only once.

child_isolate_preparer_ = [mapping](DartIsolate* isolate) {
return isolate->PrepareForRunningFromKernel(mapping);
};
if (child_isolate_preparer_ == nullptr) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment here explaining how we might get here more than once, and why we don't want to reinitialize child_isolate_preparer_ in that case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@aam aam merged commit 14ee957 into flutter:master Nov 14, 2018
@aam aam deleted the setup-secondary-isolates branch November 14, 2018 17:50
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Nov 14, 2018
flutter/engine@99d848a...14ee957

git log 99d848a..14ee957 --no-merges --oneline
14ee957 Set up secondary isolates with all kernel buffers rather than just one buffer. (flutter/engine#6832)
76522eb Roll src/third_party/skia 7f2b6fa66575..f152130ef910 (5 commits) (flutter/engine#6856)


The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants