Create separate objects for isolate state and isolate group state #14268
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jason-simmons
requested review from
chinmaygarde
and removed request for
GaryQian
chinmaygarde
reviewed
Dec 9, 2019
runtime/dart_isolate.h
Outdated
| // Object holding state associated with a Dart isolate group. An instance of | ||
| // this class will be provided to Dart_CreateIsolateGroup as the | ||
| // isolate_group_data. | ||
| class DartIsolateGroupData { |
There was a problem hiding this comment.
Let's move this into its own translation unit and also document the threading characteristics and how it differs from just the DartIsolateData.
There was a problem hiding this comment.
Nit: Consider using doxygen documentation format to document the class and public members as done in DartIsolate.
runtime/dart_isolate.h
Outdated
| const std::string advisory_script_entrypoint_; | ||
| ChildIsolatePreparer child_isolate_preparer_; | ||
| const fml::closure isolate_create_callback_; | ||
| const fml::closure isolate_shutdown_callback_; |
There was a problem hiding this comment.
Nit: Disallow copy and assign.
runtime/dart_isolate.cc
Outdated
| is_root_isolate_(is_root_isolate), | ||
| is_group_root_isolate_(is_group_root_isolate) { | ||
| FML_DCHECK(isolate_snapshot_) << "Must contain a valid isolate snapshot."; | ||
| is_root_isolate_(is_root_isolate) { | ||
| phase_ = Phase::Uninitialized; | ||
| } | ||
|
|
||
| DartIsolate::~DartIsolate() = default; |
There was a problem hiding this comment.
Can we add an assertion here that checks that if there is a message handling task runner set, the destructor is on its thread?
runtime/dart_isolate.cc
Outdated
| operator bool() const { return str_ != nullptr; } | ||
|
|
||
| private: | ||
| char* str_ = nullptr; |
There was a problem hiding this comment.
Nit: Disallow copy and assign. This is trivially copyable you might run into situations where a double free happens if this is copied.
jason-simmons
force-pushed
the
isolate_group_state
branch
2 times, most recently
from
c3b47d5
to
abd75b7
Compare
|
Done - PTAL |
jason-simmons
force-pushed
the
isolate_group_state
branch
from
abd75b7
to
c02b5fd
Compare
chinmaygarde
approved these changes
Dec 9, 2019
jason-simmons
force-pushed
the
isolate_group_state
branch
from
c02b5fd
to
c4ae6c6
Compare
Isolate data may need to be deleted on the same thread where it was allocated. In particular, the task observer set up in the UIDartState ctor must be removed from the same message loop where it was added. The engine had been using the same DartIsolate object as the root isolate data and as the isolate group data. This object would be deleted when the isolate group was shut down. However, group shutdown may occur on a thread associated with a secondary isolate. When this happens, cleanup of any state tied to the root isolate's thread will fail. This change adds a DartIsolateGroupData object holding state that is common among all isolates in a group. DartIsolateGroupData can be deleted on any thread. See flutter/flutter#45578
jason-simmons
force-pushed
the
isolate_group_state
branch
from
c4ae6c6
to
21373cc
Compare
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 10, 2019
This was referenced Dec 10, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 10, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 10, 2019
This was referenced Dec 10, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
This was referenced Dec 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Dec 11, 2019
iskakaushik
pushed a commit
to iskakaushik/flutter
that referenced
this pull request
Dec 12, 2019
flutter/engine@12bf95f...db60ebc git log 12bf95f..db60ebc --first-parent --oneline 2019-12-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from h4iiT... to otkJA... (flutter/engine#14327) 2019-12-11 skia-flutter-autoroll@skia.org Roll src/third_party/dart bd008dd1e406..d9fa37e85d5c (1 commits) (flutter/engine#14325) 2019-12-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia 5afc7b2af854..75799967be60 (2 commits) (flutter/engine#14324) 2019-12-11 skia-flutter-autoroll@skia.org Roll src/third_party/dart bcd18e67dcae..bd008dd1e406 (3 commits) (flutter/engine#14322) 2019-12-11 chinmaygarde@google.com Verify accounting for loop counts in Gif and WebP assets is consistent. (flutter/engine#14321) 2019-12-11 iska.kaushik@gmail.com [fuchsia] Do not Execute paint tasks when there is no vsync (flutter/engine#14298) 2019-12-11 skia-flutter-autoroll@skia.org Roll src/third_party/dart c74a8ec2c46e..bcd18e67dcae (9 commits) (flutter/engine#14317) 2019-12-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from nqJnP... to UdfLO... (flutter/engine#14316) 2019-12-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia 732c49739fa5..5afc7b2af854 (16 commits) (flutter/engine#14315) 2019-12-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from 9C6UA... to h4iiT... (flutter/engine#14314) 2019-12-11 iska.kaushik@gmail.com [web] Update build_web_compilers to 2.7.1 (flutter/engine#14305) 2019-12-11 liyuqian@google.com Cleanup the IO thread GrContext (flutter/engine#14265) 2019-12-10 50856934+nturgut@users.noreply.github.com Fix for tab not working (flutter/engine#14165) 2019-12-10 jason-simmons@users.noreply.github.com [SkParagraph] Convert the height override flag in text styles (flutter/engine#14283) 2019-12-10 bkonyi@google.com Roll src/third_party/dart 02a8b015ad..98c13ba18f (5 commits) (flutter/engine#14280) 2019-12-10 tvolkert@users.noreply.github.com Remove specificity on Android and iOS (flutter/engine#14282) 2019-12-10 jason-simmons@users.noreply.github.com Create separate objects for isolate state and isolate group state (flutter/engine#14268) 2019-12-10 bkonyi@google.com Roll src/third_party/dart 8b8894648f..02a8b015ad (26 commits) (flutter/engine#14278) 2019-12-10 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from Zkpa_... to nqJnP... (flutter/engine#14274) 2019-12-10 skia-flutter-autoroll@skia.org Roll src/third_party/skia e56cc054dbae..ab26643258ad (3 commits) (flutter/engine#14273) 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 kaushikiska@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
iskakaushik
pushed a commit
to flutter/flutter
that referenced
this pull request
Dec 13, 2019
* 571c999 Roll src/third_party/skia e56cc054dbae..ab26643258ad (3 commits) (flutter/engine#14273) * 140818a Roll fuchsia/sdk/core/linux-amd64 from Zkpa_... to nqJnP... (flutter/engine#14274) * ed6830e Roll src/third_party/dart 8b8894648f..02a8b015ad (26 commits) (flutter/engine#14278) * b7d4278 Create separate objects for isolate state and isolate group state (flutter/engine#14268) * 57afd86 Remove specificity on Android and iOS (flutter/engine#14282) * b7c947d Roll src/third_party/dart 02a8b015ad..98c13ba18f (5 commits) (flutter/engine#14280) * 76d264e [SkParagraph] Convert the height override flag in text styles (flutter/engine#14283) * deb8e57 Fix for tab not working (flutter/engine#14165) * 212fbba Cleanup the IO thread GrContext (flutter/engine#14265) * 3e55f64 [web] Update build_web_compilers to 2.7.1 (flutter/engine#14305) * b6d4fd1 Roll fuchsia/sdk/core/mac-amd64 from 9C6UA... to h4iiT... (flutter/engine#14314) * c0b1dc0 Roll src/third_party/skia 732c49739fa5..5afc7b2af854 (16 commits) (flutter/engine#14315) * 5b5206e Roll fuchsia/sdk/core/linux-amd64 from nqJnP... to UdfLO... (flutter/engine#14316) * 058b4bc Roll src/third_party/dart c74a8ec2c46e..bcd18e67dcae (9 commits) (flutter/engine#14317) * 6430ecf [fuchsia] Do not Execute paint tasks when there is no vsync (flutter/engine#14298) * 49d6552 Verify accounting for loop counts in Gif and WebP assets is consistent. (flutter/engine#14321) * 2ae0d42 Roll src/third_party/dart bcd18e67dcae..bd008dd1e406 (3 commits) (flutter/engine#14322) * ac95536 Roll src/third_party/skia 5afc7b2af854..75799967be60 (2 commits) (flutter/engine#14324) * 02fb9c1 Roll src/third_party/dart bd008dd1e406..d9fa37e85d5c (1 commits) (flutter/engine#14325) * db60ebc Roll fuchsia/sdk/core/mac-amd64 from h4iiT... to otkJA... (flutter/engine#14327)
filmil
pushed a commit
to filmil/engine
that referenced
this pull request
Mar 13, 2020
…utter#14268) Isolate data may need to be deleted on the same thread where it was allocated. In particular, the task observer set up in the UIDartState ctor must be removed from the same message loop where it was added. The engine had been using the same DartIsolate object as the root isolate data and as the isolate group data. This object would be deleted when the isolate group was shut down. However, group shutdown may occur on a thread associated with a secondary isolate. When this happens, cleanup of any state tied to the root isolate's thread will fail. This change adds a DartIsolateGroupData object holding state that is common among all isolates in a group. DartIsolateGroupData can be deleted on any thread. See flutter/flutter#45578
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Isolate data may need to be deleted on the same thread where it was allocated.
In particular, the task observer set up in the UIDartState ctor must be removed
from the same message loop where it was added.
The engine had been using the same DartIsolate object as the root isolate data
and as the isolate group data. This object would be deleted when the isolate
group was shut down. However, group shutdown may occur on a thread associated
with a secondary isolate. When this happens, cleanup of any state tied to the
root isolate's thread will fail.
This change adds a DartIsolateGroupData object holding state that is common
among all isolates in a group. DartIsolateGroupData can be deleted on any
thread.
See flutter/flutter#45578