Skip to content

Commit 3d14b75

Browse files
mralephcommit-bot@chromium.org
mraleph
authored and
commit-bot@chromium.org
committed
Revert "Reland "[vm/concurrency] Introduce concept of Isolate Groups""
This reverts commit 67ab3be. Reason for revert: Causes non-deterministic failures on front-end bots on Windows with "Isolate creation failed" error. See https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket.appspot.com/8909292129328681248/+/steps/unit_tests/0/stdout Original change's description: > Reland "[vm/concurrency] Introduce concept of Isolate Groups" > > An Isolate Group (IG) is a collection of isolates which were spawned from the > same source. This allows the VM to: > > * have a guarantee that all isolates within one IG can safely exchange > structured objects (currently we rely on embedder for this > guarantee) > > * hot-reload all isolates together (currently we only reload one > isolate, leaving same-source isolates in inconsistent state) > > * make a shared heap for all isolates from the same IG, which paves > the way for faster communication and sharing of immutable objects. > > All isolates within one IG will share the same IsolateGroupSource. > > **Embedder changes** > > This change makes breaking embedder API changes to support this new > concept of Isolate Groups: The existing isolate lifecycle callbacks > given to Dart_Initialize will become Isolate Group lifecycle callbacks. > A new callback `initialize_isolate` callback will be added which can > initialize a new isolate within an existing IG. > > Existing embedders can be updated by performing the following renames > > Dart_CreateIsolate -> Dart_CreateIsolateGroup > Dart_IsolateCreateCallback -> Dart_IsolateGroupCreateCallback > Dart_IsolateCleanupCallback -> Dart_IsolateGroupShutdownCallback > Dart_CreateIsolateFromKernel -> Dart_CreateIsolateGroupFromKernel > Dart_CurrentIsolateData -> Dart_CurrentIsolateGroupData > Dart_IsolateData -> Dart_IsolateGroupData > Dart_GetNativeIsolateData -> Dart_GetNativeIsolateGroupData > Dart_InitializeParams.create -> Dart_InitializeParams.create_group > Dart_InitializeParams.cleanup -> Dart_InitializeParams.shutdown_group > Dart_InitializeParams.shutdown -> Dart_InitializeParams.shutdown_isolate > > By default `Isolate.spawn` will cause the creation of a new IG. > > Though an embedder can opt-into supporting multiple isolates within one IG by > providing a callback to the newly added `Dart_InitializeParams.initialize_isolate`. > The responsibility of this new callback is to initialize an existing > isolate (which was setup by re-using source code from the spawning > isolate - i.e. the one which used `Isolate.spawn`) by setting native > resolvers, initializing global state, etc. > > Issue #36648 > Issue #36097 > > Original review: https://dart-review.googlesource.com/c/sdk/+/105241 > > Difference to original review: > > * Give each isolate it's own [Loader] (for now) > * Sort classes during initialization for spawned isolates if app-jit is used (to match main isolate) > * Fix IsolateData memory leak if isolate startup fails > > Change-Id: I98277d3d10fe275aa9b8a16b6bdd446bbea0b100 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/107506 > Commit-Queue: Martin Kustermann <kustermann@google.com> > Reviewed-by: Ryan Macnak <rmacnak@google.com> TBR=kustermann@google.com,aam@google.com,rmacnak@google.com # Not skipping CQ checks because original CL landed > 1 day ago. Change-Id: Ia4e0f4f9fc317499d3570a371c5bdf9aed799e77 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/108101 Reviewed-by: Vyacheslav Egorov <vegorov@google.com> Commit-Queue: Vyacheslav Egorov <vegorov@google.com>
1 parent 527238e commit 3d14b75

27 files changed

+458
-1199
lines changed

‎runtime/bin/dart_embedder_api_impl.cc‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,9 @@ Dart_Isolate CreateKernelServiceIsolate(const IsolateCreationData& data,
4545
const uint8_t* buffer,
4646
intptr_t buffer_size,
4747
char** error) {
48-
Dart_Isolate kernel_isolate = Dart_CreateIsolateGroupFromKernel(
48+
Dart_Isolate kernel_isolate = Dart_CreateIsolateFromKernel(
4949
data.script_uri, data.main, buffer, buffer_size, data.flags,
50-
data.isolate_group_data, data.isolate_data, error);
50+
data.callback_data, error);
5151
if (kernel_isolate == nullptr) {
5252
return nullptr;
5353
}
@@ -78,9 +78,9 @@ Dart_Isolate CreateVmServiceIsolate(const IsolateCreationData& data,
7878
}
7979
data.flags->load_vmservice_library = true;
8080

81-
Dart_Isolate service_isolate = Dart_CreateIsolateGroupFromKernel(
81+
Dart_Isolate service_isolate = Dart_CreateIsolateFromKernel(
8282
data.script_uri, data.main, kernel_buffer, kernel_buffer_size, data.flags,
83-
data.isolate_group_data, data.isolate_data, error);
83+
data.callback_data, error);
8484
if (service_isolate == nullptr) {
8585
return nullptr;
8686
}

‎runtime/bin/gen_snapshot.cc‎

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -745,25 +745,23 @@ static int CreateIsolateAndSnapshot(const CommandLineOptions& inputs) {
745745
isolate_flags.entry_points = no_entry_points;
746746
}
747747

748-
auto isolate_group_data =
749-
new IsolateGroupData(nullptr, nullptr, nullptr, nullptr, false);
748+
IsolateData* isolate_data = new IsolateData(NULL, NULL, NULL, NULL);
750749
Dart_Isolate isolate;
751750
char* error = NULL;
752751
if (isolate_snapshot_data == NULL) {
753752
// We need to capture the vmservice library in the core snapshot, so load it
754753
// in the main isolate as well.
755754
isolate_flags.load_vmservice_library = true;
756-
isolate = Dart_CreateIsolateGroupFromKernel(
757-
NULL, NULL, kernel_buffer, kernel_buffer_size, &isolate_flags,
758-
isolate_group_data, /*isolate_data=*/nullptr, &error);
755+
isolate = Dart_CreateIsolateFromKernel(NULL, NULL, kernel_buffer,
756+
kernel_buffer_size, &isolate_flags,
757+
isolate_data, &error);
759758
} else {
760-
isolate = Dart_CreateIsolateGroup(NULL, NULL, isolate_snapshot_data,
761-
isolate_snapshot_instructions, NULL, NULL,
762-
&isolate_flags, isolate_group_data,
763-
/*isolate_data=*/nullptr, &error);
759+
isolate = Dart_CreateIsolate(NULL, NULL, isolate_snapshot_data,
760+
isolate_snapshot_instructions, NULL, NULL,
761+
&isolate_flags, isolate_data, &error);
764762
}
765763
if (isolate == NULL) {
766-
delete isolate_group_data;
764+
delete isolate_data;
767765
Syslog::PrintErr("%s\n", error);
768766
free(error);
769767
return kErrorExitCode;
@@ -785,9 +783,9 @@ static int CreateIsolateAndSnapshot(const CommandLineOptions& inputs) {
785783
// If the input dill file does not have a root library, then
786784
// Dart_LoadScript will error.
787785
//
788-
// TODO(kernel): Dart_CreateIsolateGroupFromKernel should respect the root
789-
// library in the kernel file, though this requires auditing the other
790-
// loading paths in the embedders that had to work around this.
786+
// TODO(kernel): Dart_CreateIsolateFromKernel should respect the root library
787+
// in the kernel file, though this requires auditing the other loading paths
788+
// in the embedders that had to work around this.
791789
result = Dart_SetRootLibrary(
792790
Dart_LoadLibraryFromKernel(kernel_buffer, kernel_buffer_size));
793791
CHECK_RESULT(result);

‎runtime/bin/isolate_data.cc‎

Lines changed: 17 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -9,54 +9,45 @@
99
namespace dart {
1010
namespace bin {
1111

12-
IsolateGroupData::IsolateGroupData(const char* url,
13-
const char* package_root,
14-
const char* packages_file,
15-
AppSnapshot* app_snapshot,
16-
bool isolate_run_app_snapshot)
12+
IsolateData::IsolateData(const char* url,
13+
const char* package_root,
14+
const char* packages_file,
15+
AppSnapshot* app_snapshot)
1716
: script_url((url != NULL) ? strdup(url) : NULL),
1817
package_root(NULL),
18+
packages_file(NULL),
19+
loader_(NULL),
1920
app_snapshot_(app_snapshot),
2021
dependencies_(NULL),
2122
resolved_packages_config_(NULL),
2223
kernel_buffer_(NULL),
23-
kernel_buffer_size_(0),
24-
isolate_run_app_snapshot_(isolate_run_app_snapshot) {
24+
kernel_buffer_size_(0) {
2525
if (package_root != NULL) {
2626
ASSERT(packages_file == NULL);
27-
package_root = strdup(package_root);
27+
this->package_root = strdup(package_root);
2828
} else if (packages_file != NULL) {
29-
packages_file_ = strdup(packages_file);
29+
this->packages_file = strdup(packages_file);
3030
}
3131
}
3232

33-
IsolateGroupData::~IsolateGroupData() {
33+
void IsolateData::OnIsolateShutdown() {
34+
}
35+
36+
IsolateData::~IsolateData() {
3437
free(script_url);
3538
script_url = NULL;
3639
free(package_root);
3740
package_root = NULL;
38-
free(packages_file_);
39-
packages_file_ = NULL;
41+
free(packages_file);
42+
packages_file = NULL;
4043
free(resolved_packages_config_);
4144
resolved_packages_config_ = NULL;
4245
kernel_buffer_ = NULL;
4346
kernel_buffer_size_ = 0;
47+
delete app_snapshot_;
48+
app_snapshot_ = NULL;
4449
delete dependencies_;
4550
}
4651

47-
IsolateData::IsolateData(IsolateGroupData* isolate_group_data)
48-
: isolate_group_data_(isolate_group_data),
49-
loader_(nullptr),
50-
packages_file_(nullptr) {
51-
if (isolate_group_data->packages_file_ != nullptr) {
52-
packages_file_ = strdup(isolate_group_data->packages_file_);
53-
}
54-
}
55-
56-
IsolateData::~IsolateData() {
57-
free(packages_file_);
58-
packages_file_ = nullptr;
59-
}
60-
6152
} // namespace bin
6253
} // namespace dart

‎runtime/bin/isolate_data.h‎

Lines changed: 35 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -28,54 +28,62 @@ class AppSnapshot;
2828
class EventHandler;
2929
class Loader;
3030

31-
// Data associated with every isolate group in the standalone VM
31+
// Data associated with every isolate in the standalone VM
3232
// embedding. This is used to free external resources for each isolate
33-
// group when the isolate group shuts down.
34-
class IsolateGroupData {
33+
// when the isolate shuts down.
34+
class IsolateData {
3535
public:
36-
IsolateGroupData(const char* url,
37-
const char* package_root,
38-
const char* packages_file,
39-
AppSnapshot* app_snapshot,
40-
bool isolate_run_app_snapshot);
41-
~IsolateGroupData();
36+
IsolateData(const char* url,
37+
const char* package_root,
38+
const char* packages_file,
39+
AppSnapshot* app_snapshot);
40+
~IsolateData();
4241

4342
char* script_url;
4443
char* package_root;
44+
char* packages_file;
4545

4646
const std::shared_ptr<uint8_t>& kernel_buffer() const {
4747
return kernel_buffer_;
4848
}
4949

5050
intptr_t kernel_buffer_size() const { return kernel_buffer_size_; }
5151

52-
// Associate the given kernel buffer with this IsolateGroupData without
53-
// giving it ownership of the buffer.
52+
// Associate the given kernel buffer with this IsolateData without giving it
53+
// ownership of the buffer.
5454
void SetKernelBufferUnowned(uint8_t* buffer, intptr_t size) {
5555
ASSERT(kernel_buffer_.get() == NULL);
5656
kernel_buffer_ = std::shared_ptr<uint8_t>(buffer, FreeUnownedKernelBuffer);
5757
kernel_buffer_size_ = size;
5858
}
5959

60-
// Associate the given kernel buffer with this IsolateGroupData and give it
61-
// ownership of the buffer. This IsolateGroupData is the first one to own the
60+
// Associate the given kernel buffer with this IsolateData and give it
61+
// ownership of the buffer. This IsolateData is the first one to own the
6262
// buffer.
6363
void SetKernelBufferNewlyOwned(uint8_t* buffer, intptr_t size) {
6464
ASSERT(kernel_buffer_.get() == NULL);
6565
kernel_buffer_ = std::shared_ptr<uint8_t>(buffer, free);
6666
kernel_buffer_size_ = size;
6767
}
6868

69-
// Associate the given kernel buffer with this IsolateGroupData and give it
69+
// Associate the given kernel buffer with this IsolateData and give it
7070
// ownership of the buffer. The buffer is already owned by another
71-
// IsolateGroupData.
71+
// IsolateData.
7272
void SetKernelBufferAlreadyOwned(std::shared_ptr<uint8_t> buffer,
7373
intptr_t size) {
7474
ASSERT(kernel_buffer_.get() == NULL);
7575
kernel_buffer_ = std::move(buffer);
7676
kernel_buffer_size_ = size;
7777
}
7878

79+
void UpdatePackagesFile(const char* packages_file_) {
80+
if (packages_file != NULL) {
81+
free(packages_file);
82+
packages_file = NULL;
83+
}
84+
packages_file = strdup(packages_file_);
85+
}
86+
7987
const char* resolved_packages_config() const {
8088
return resolved_packages_config_;
8189
}
@@ -88,54 +96,6 @@ class IsolateGroupData {
8896
resolved_packages_config_ = strdup(packages_config);
8997
}
9098

91-
MallocGrowableArray<char*>* dependencies() const { return dependencies_; }
92-
void set_dependencies(MallocGrowableArray<char*>* deps) {
93-
dependencies_ = deps;
94-
}
95-
96-
bool RunFromAppSnapshot() const {
97-
// If the main isolate is using an app snapshot the [app_snapshot_] pointer
98-
// will be still nullptr (see main.cc:CreateIsolateGroupAndSetupHelper)
99-
//
100-
// Because of thus we have an additional boolean signaling whether the
101-
// isolate was started from an app snapshot.
102-
return app_snapshot_ != nullptr || isolate_run_app_snapshot_;
103-
}
104-
105-
private:
106-
friend class IsolateData; // For packages_file_
107-
108-
std::unique_ptr<AppSnapshot> app_snapshot_;
109-
MallocGrowableArray<char*>* dependencies_;
110-
char* resolved_packages_config_;
111-
std::shared_ptr<uint8_t> kernel_buffer_;
112-
intptr_t kernel_buffer_size_;
113-
char* packages_file_ = nullptr;
114-
bool isolate_run_app_snapshot_;
115-
116-
static void FreeUnownedKernelBuffer(uint8_t*) {}
117-
118-
DISALLOW_COPY_AND_ASSIGN(IsolateGroupData);
119-
};
120-
121-
// Data associated with every isolate in the standalone VM
122-
// embedding. This is used to free external resources for each isolate
123-
// when the isolate shuts down.
124-
class IsolateData {
125-
public:
126-
explicit IsolateData(IsolateGroupData* isolate_group_data);
127-
~IsolateData();
128-
129-
IsolateGroupData* isolate_group_data() const { return isolate_group_data_; }
130-
131-
void UpdatePackagesFile(const char* packages_file) {
132-
if (packages_file != nullptr) {
133-
free(packages_file_);
134-
packages_file_ = nullptr;
135-
}
136-
packages_file_ = strdup(packages_file);
137-
}
138-
13999
// While loading a loader is associated with the isolate.
140100
bool HasLoader() const { return loader_ != NULL; }
141101
Loader* loader() const {
@@ -146,13 +106,22 @@ class IsolateData {
146106
ASSERT((loader_ == NULL) || (loader == NULL));
147107
loader_ = loader;
148108
}
109+
MallocGrowableArray<char*>* dependencies() const { return dependencies_; }
110+
void set_dependencies(MallocGrowableArray<char*>* deps) {
111+
dependencies_ = deps;
112+
}
149113

150-
const char* packages_file() const { return packages_file_; }
114+
void OnIsolateShutdown();
151115

152116
private:
153-
IsolateGroupData* isolate_group_data_;
154117
Loader* loader_;
155-
char* packages_file_;
118+
AppSnapshot* app_snapshot_;
119+
MallocGrowableArray<char*>* dependencies_;
120+
char* resolved_packages_config_;
121+
std::shared_ptr<uint8_t> kernel_buffer_;
122+
intptr_t kernel_buffer_size_;
123+
124+
static void FreeUnownedKernelBuffer(uint8_t*) {}
156125

157126
DISALLOW_COPY_AND_ASSIGN(IsolateData);
158127
};

‎runtime/bin/loader.cc‎

Lines changed: 15 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -263,18 +263,18 @@ static bool PathContainsSeparator(const char* path) {
263263

264264
void Loader::AddDependencyLocked(Loader* loader, const char* resolved_uri) {
265265
MallocGrowableArray<char*>* dependencies =
266-
loader->isolate_group_data()->dependencies();
266+
loader->isolate_data_->dependencies();
267267
if (dependencies == NULL) {
268268
return;
269269
}
270270
dependencies->Add(strdup(resolved_uri));
271271
}
272272

273273
void Loader::ResolveDependenciesAsFilePaths() {
274-
IsolateGroupData* isolate_group_data =
275-
reinterpret_cast<IsolateGroupData*>(Dart_CurrentIsolateGroupData());
276-
ASSERT(isolate_group_data != NULL);
277-
MallocGrowableArray<char*>* dependencies = isolate_group_data->dependencies();
274+
IsolateData* isolate_data =
275+
reinterpret_cast<IsolateData*>(Dart_CurrentIsolateData());
276+
ASSERT(isolate_data != NULL);
277+
MallocGrowableArray<char*>* dependencies = isolate_data->dependencies();
278278
if (dependencies == NULL) {
279279
return;
280280
}
@@ -454,15 +454,15 @@ bool Loader::ProcessQueueLocked(ProcessResult process_result) {
454454
return !hit_error;
455455
}
456456

457-
void Loader::InitForSnapshot(const char* snapshot_uri,
458-
IsolateData* isolate_data) {
457+
void Loader::InitForSnapshot(const char* snapshot_uri) {
458+
IsolateData* isolate_data =
459+
reinterpret_cast<IsolateData*>(Dart_CurrentIsolateData());
459460
ASSERT(isolate_data != NULL);
460461
ASSERT(!isolate_data->HasLoader());
461462
// Setup a loader. The constructor does a bunch of leg work.
462463
Loader* loader = new Loader(isolate_data);
463464
// Send the init message.
464-
loader->Init(isolate_data->isolate_group_data()->package_root,
465-
isolate_data->packages_file(),
465+
loader->Init(isolate_data->package_root, isolate_data->packages_file,
466466
DartUtils::original_working_directory, snapshot_uri);
467467
// Destroy the loader. The destructor does a bunch of leg work.
468468
delete loader;
@@ -516,15 +516,15 @@ Dart_Handle Loader::SendAndProcessReply(intptr_t tag,
516516
Dart_Handle url,
517517
uint8_t** payload,
518518
intptr_t* payload_length) {
519-
auto isolate_data = reinterpret_cast<IsolateData*>(Dart_CurrentIsolateData());
519+
IsolateData* isolate_data =
520+
reinterpret_cast<IsolateData*>(Dart_CurrentIsolateData());
520521
ASSERT(isolate_data != NULL);
521522
ASSERT(!isolate_data->HasLoader());
522523
Loader* loader = NULL;
523524

524525
// Setup the loader. The constructor does a bunch of leg work.
525526
loader = new Loader(isolate_data);
526-
loader->Init(isolate_data->isolate_group_data()->package_root,
527-
isolate_data->packages_file(),
527+
loader->Init(isolate_data->package_root, isolate_data->packages_file,
528528
DartUtils::original_working_directory, NULL);
529529
ASSERT(loader != NULL);
530530
ASSERT(isolate_data->HasLoader());
@@ -565,10 +565,6 @@ Dart_Handle Loader::ResolveAsFilePath(Dart_Handle url,
565565
payload_length);
566566
}
567567

568-
IsolateGroupData* Loader::isolate_group_data() {
569-
return isolate_data_->isolate_group_data();
570-
}
571-
572568
#if defined(DART_PRECOMPILED_RUNTIME)
573569
Dart_Handle Loader::LibraryTagHandler(Dart_LibraryTag tag,
574570
Dart_Handle library,
@@ -683,7 +679,8 @@ Dart_Handle Loader::LibraryTagHandler(Dart_LibraryTag tag,
683679
}
684680
}
685681

686-
auto isolate_data = reinterpret_cast<IsolateData*>(Dart_CurrentIsolateData());
682+
IsolateData* isolate_data =
683+
reinterpret_cast<IsolateData*>(Dart_CurrentIsolateData());
687684
ASSERT(isolate_data != NULL);
688685
if ((tag == Dart_kScriptTag) && Dart_IsString(library)) {
689686
// Update packages file for isolate.
@@ -710,8 +707,7 @@ Dart_Handle Loader::LibraryTagHandler(Dart_LibraryTag tag,
710707

711708
// Setup the loader. The constructor does a bunch of leg work.
712709
loader = new Loader(isolate_data);
713-
loader->Init(isolate_data->isolate_group_data()->package_root,
714-
isolate_data->packages_file(),
710+
loader->Init(isolate_data->package_root, isolate_data->packages_file,
715711
DartUtils::original_working_directory,
716712
(tag == Dart_kScriptTag) ? url_string : NULL);
717713
} else {

0 commit comments

Comments
 (0)