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

Ensure correct child isolate initialization in enable-isolate-groups dart vm configuration. #26011

Merged
merged 11 commits into from
Aug 2, 2021

Conversation

aam
Copy link
Member

@aam aam commented May 7, 2021

Dart_SetRootLibrary and LoadKernel needs to be done only for a root isolate in the isolate group. When in --enable-isolate-groups dart vm mode child isolates will reuse kernel and object store set up by root isolate.

@flutter-dashboard
Copy link

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.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

@google-cla google-cla bot added the cla: yes label May 7, 2021
@aam aam added the Work in progress (WIP) Not ready (yet) for review! label May 7, 2021
@aam
Copy link
Member Author

aam commented May 11, 2021

cc @mkustermann regarding changes to isolate loading for ltw isolates: avoid loading kernel for children isolates

@chinmaygarde
Copy link
Member

Just curious about progress on this patch. Is there some blocker?

@mkustermann
Copy link
Member

@chinmaygarde

This change will be required iff --enable-isolate-groups is also enabled in JIT mode. So turning on the flag will require these changes.

We could land the PR now (despite being the flag off atm) by guarding the changes via Dart_IsVMFlagSet("--enable-isolate-groups"). Then once flipping the flag, it will do the right thing. Is that something you prefer?

@mkustermann
Copy link
Member

@aam Could you update this PR to guard the changes under the Dart_IsVMFlagSet("--enable-isolate-groups") condition? That would make it easy to enable the flag (and if something goes wrong, disable it again without reverting bigger changes)

@aam
Copy link
Member Author

aam commented Jul 28, 2021

@aam Could you update this PR to guard the changes under the Dart_IsVMFlagSet("--enable-isolate-groups") condition? That would make it easy to enable the flag (and if something goes wrong, disable it again without reverting bigger changes)

Done, but I left the code in dart_vm.cc that enables the flag too.

@mkustermann
Copy link
Member

Done, but I left the code in dart_vm.cc that enables the flag too.

@aam Could you limit this change to only avoid the kernel loading (i.e. not enable the flag, not roll DEPS and all the other changes)?

I think we will avoid flipping the flag here in the embedder and instead turn it on by-default in the VM instead, which will transparently make us use the right code path here.

@aam
Copy link
Member Author

aam commented Jul 29, 2021

@aam Could you limit this change to only avoid the kernel loading (i.e. not enable the flag, not roll DEPS and all the other changes)?

There should be only four files changed in this PR(runtime/dart_isolate.cc, runtime/dart_isolate.h, runtime/dart_vm.cc, runtime/isolate_configuration.cc).
Do you see more(DEPS, anything else)?

@aam
Copy link
Member Author

aam commented Jul 29, 2021

I think we will avoid flipping the flag here in the embedder

Done.

@aam aam changed the title [dns] Try enabling lightweight isolates in jit Ensure correct child isolate initialization in enable-isolate-groups dart vm configuration. Jul 29, 2021
@mkustermann
Copy link
Member

Do you see more(DEPS, anything else)?

I did. Not sure why, but after your latest push the Files changed shows the right thing.

@aam aam removed the Work in progress (WIP) Not ready (yet) for review! label Aug 2, 2021
@aam aam merged commit bf06b19 into flutter:master Aug 2, 2021
@aam aam deleted the try-ltw-jit branch August 2, 2021 14:57
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 2, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 3, 2021
naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021
…dart vm configuration. (flutter#26011)

* Ensure child isolate doesn't load kernel

* Guard changes to isolate initialization by enable-isolate-groups flag check
filmil pushed a commit to filmil/engine that referenced this pull request Apr 21, 2022
…dart vm configuration. (flutter#26011)

* Ensure child isolate doesn't load kernel

* Guard changes to isolate initialization by enable-isolate-groups flag check
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.

3 participants