From 69485e9b28d3e578c195e37c55f78fb6f1d1be8d Mon Sep 17 00:00:00 2001 From: Alexander Aprelev Date: Wed, 23 Apr 2025 15:26:21 -0700 Subject: [PATCH] [vm] Move origin_id from isolate to isolate group. Isolates in one group share same origin_id anyway, so it makes sense to store it on the group too. Remove isolate's _originNumber from service api - isolate group should be used instead. Based on feedback from https://dart-review.git.corp.google.com/c/sdk/+/418503/21/runtime/lib/isolate.cc#107 TEST=ci Change-Id: Iab4b6393a042c9302e911a276a6afc6dab63e70d Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/424140 Commit-Queue: Alexander Aprelev Reviewed-by: Ryan Macnak --- pkg/vm_service/test/get_isolate_rpc_test.dart | 1 - .../http_get_isolate_rpc_common.dart | 1 - runtime/lib/isolate.cc | 33 ++++++------------- .../lib/src/elements/debugger.dart | 1 - .../observatory/lib/src/service/object.dart | 2 -- .../tests/service/get_isolate_rpc_test.dart | 1 - .../service/http_get_isolate_rpc_common.dart | 1 - runtime/vm/dart_api_impl.cc | 1 - runtime/vm/isolate.cc | 18 ++-------- runtime/vm/isolate.h | 11 ++----- runtime/vm/object.cc | 2 +- runtime/vm/port.cc | 2 +- runtime/vm/service.cc | 2 +- runtime/vm/service_isolate.cc | 4 +-- 14 files changed, 20 insertions(+), 60 deletions(-) diff --git a/pkg/vm_service/test/get_isolate_rpc_test.dart b/pkg/vm_service/test/get_isolate_rpc_test.dart index f747468beca..e9522aa5942 100644 --- a/pkg/vm_service/test/get_isolate_rpc_test.dart +++ b/pkg/vm_service/test/get_isolate_rpc_test.dart @@ -17,7 +17,6 @@ final tests = [ expect(result.isolateFlags, isNotNull); expect(result.isolateFlags!.length, isPositive); expect(result.isSystemIsolate, isFalse); - expect(result.json!['_originNumber'], result.number); expect(result.startTime, isPositive); expect(result.livePorts, isPositive); expect(result.pauseOnExit, isFalse); diff --git a/pkg/vm_service/test/http_invocations/http_get_isolate_rpc_common.dart b/pkg/vm_service/test/http_invocations/http_get_isolate_rpc_common.dart index 4bff2792feb..49e2d3e67cd 100644 --- a/pkg/vm_service/test/http_invocations/http_get_isolate_rpc_common.dart +++ b/pkg/vm_service/test/http_invocations/http_get_isolate_rpc_common.dart @@ -39,7 +39,6 @@ final httpGetIsolateRpcTests = [ )! as Isolate; Expect.isTrue(result.id!.startsWith('isolates/')); Expect.isNotNull(result.number); - Expect.equals(result.json!['_originNumber'], result.number); Expect.isTrue(result.startTime! > 0); Expect.isTrue(result.livePorts! > 0); Expect.isFalse(result.pauseOnExit); diff --git a/runtime/lib/isolate.cc b/runtime/lib/isolate.cc index cb2c9157f7b..87e218191ec 100644 --- a/runtime/lib/isolate.cc +++ b/runtime/lib/isolate.cc @@ -102,12 +102,9 @@ DEFINE_NATIVE_ENTRY(SendPort_get_hashcode, 0, 1) { return Smi::New(hash); } -static bool InSameGroup(Isolate* sender, const SendPort& receiver) { - // Cannot determine whether sender is in same group (yet). - if (sender->origin_id() == ILLEGAL_PORT) return false; - +static bool InSameGroup(IsolateGroup* sender, const SendPort& receiver) { // Only allow arbitrary messages between isolates of the same IG. - return sender->origin_id() == receiver.origin_id(); + return sender->id() == receiver.origin_id(); } DEFINE_NATIVE_ENTRY(SendPort_sendInternal_, 0, 2) { @@ -115,7 +112,8 @@ DEFINE_NATIVE_ENTRY(SendPort_sendInternal_, 0, 2) { GET_NON_NULL_NATIVE_ARGUMENT(Instance, obj, arguments->NativeArgAt(1)); const Dart_Port destination_port_id = port.Id(); - const bool same_group = InSameGroup(isolate, port); + IsolateGroup* group = thread->isolate_group(); + const bool same_group = InSameGroup(group, port); #if defined(DEBUG) if (same_group) { ASSERT(PortMap::IsReceiverInThisIsolateGroupOrClosed(destination_port_id, @@ -505,11 +503,11 @@ DEFINE_NATIVE_ENTRY(Isolate_exit_, 0, 2) { if (!port.IsNull()) { GET_NATIVE_ARGUMENT(Instance, obj, arguments->NativeArgAt(1)); - const bool same_group = InSameGroup(isolate, port); + IsolateGroup* group = thread->isolate_group(); + const bool same_group = InSameGroup(group, port); #if defined(DEBUG) if (same_group) { - ASSERT(PortMap::IsReceiverInThisIsolateGroupOrClosed(port.Id(), - isolate->group())); + ASSERT(PortMap::IsReceiverInThisIsolateGroupOrClosed(port.Id(), group)); } #endif if (!same_group) { @@ -557,7 +555,6 @@ DEFINE_NATIVE_ENTRY(Isolate_exit_, 0, 2) { class IsolateSpawnState { public: IsolateSpawnState(Dart_Port parent_port, - Dart_Port origin_id, const char* script_url, PersistentHandle* closure_tuple_handle, SerializedObjectBuffer* message_buffer, @@ -582,7 +579,6 @@ class IsolateSpawnState { ~IsolateSpawnState(); Dart_Port parent_port() const { return parent_port_; } - Dart_Port origin_id() const { return origin_id_; } Dart_Port on_exit_port() const { return on_exit_port_; } Dart_Port on_error_port() const { return on_error_port_; } const char* script_url() const { return script_url_; } @@ -606,7 +602,6 @@ class IsolateSpawnState { private: Dart_Port parent_port_; - Dart_Port origin_id_ = ILLEGAL_PORT; Dart_Port on_exit_port_; Dart_Port on_error_port_; const char* script_url_; @@ -630,7 +625,6 @@ static const char* NewConstChar(const char* chars) { } IsolateSpawnState::IsolateSpawnState(Dart_Port parent_port, - Dart_Port origin_id, const char* script_url, PersistentHandle* closure_tuple_handle, SerializedObjectBuffer* message_buffer, @@ -642,7 +636,6 @@ IsolateSpawnState::IsolateSpawnState(Dart_Port parent_port, const char* debug_name, IsolateGroup* isolate_group) : parent_port_(parent_port), - origin_id_(origin_id), on_exit_port_(on_exit_port), on_error_port_(on_error_port), script_url_(script_url), @@ -876,11 +869,6 @@ class SpawnIsolateTask : public ThreadPool::Task { return; } - if (state_->origin_id() != ILLEGAL_PORT) { - // origin_id is set to parent isolate main port id when spawning via - // spawnFunction. - child->set_origin_id(state_->origin_id()); - } bool errors_are_fatal = state_->errors_are_fatal(); Dart_Port on_error_port = state_->on_error_port(); Dart_Port on_exit_port = state_->on_exit_port(); @@ -1119,10 +1107,9 @@ DEFINE_NATIVE_ENTRY(Isolate_spawnFunction, 0, 10) { } std::unique_ptr state(new IsolateSpawnState( - port.Id(), isolate->origin_id(), String2UTF8(script_uri), - closure_tuple_handle, &message_buffer, utf8_package_config, - paused.value(), fatal_errors, on_exit_port, on_error_port, - utf8_debug_name, isolate->group())); + port.Id(), String2UTF8(script_uri), closure_tuple_handle, &message_buffer, + utf8_package_config, paused.value(), fatal_errors, on_exit_port, + on_error_port, utf8_debug_name, isolate->group())); isolate->group()->thread_pool()->Run(isolate, std::move(state)); diff --git a/runtime/observatory/lib/src/elements/debugger.dart b/runtime/observatory/lib/src/elements/debugger.dart index 1a60dcc55dd..8dd125bfc01 100644 --- a/runtime/observatory/lib/src/elements/debugger.dart +++ b/runtime/observatory/lib/src/elements/debugger.dart @@ -1108,7 +1108,6 @@ class IsolateListCommand extends DebuggerCommand { String current = (isolate == debugger.isolate ? '*' : ''); debugger.console .print("${isolate.number.toString().padLeft(maxIdLen, ' ')} " - "${isolate.originNumber.toString().padLeft(maxIdLen, ' ')} " "${isolate.name!.padRight(maxNameLen, ' ')} " "${_isolateRunState(isolate).padRight(maxRunStateLen, ' ')} " "${current}"); diff --git a/runtime/observatory/lib/src/service/object.dart b/runtime/observatory/lib/src/service/object.dart index ccd44a7dbc1..fbd36d29579 100644 --- a/runtime/observatory/lib/src/service/object.dart +++ b/runtime/observatory/lib/src/service/object.dart @@ -1404,7 +1404,6 @@ class Isolate extends ServiceObjectOwner implements M.Isolate { VM get vm => owner as VM; Isolate get isolate => this; int? number; - int? originNumber; DateTime? startTime; Duration? get upTime { if (startTime == null) { @@ -1681,7 +1680,6 @@ class Isolate extends ServiceObjectOwner implements M.Isolate { loading = false; runnable = map['runnable'] == true; _upgradeCollection(map, isolate); - originNumber = int.tryParse(map['_originNumber']); if (map['rootLib'] != null) { rootLibrary = map['rootLib']; } diff --git a/runtime/observatory/tests/service/get_isolate_rpc_test.dart b/runtime/observatory/tests/service/get_isolate_rpc_test.dart index 0706166306b..a839e954f1e 100644 --- a/runtime/observatory/tests/service/get_isolate_rpc_test.dart +++ b/runtime/observatory/tests/service/get_isolate_rpc_test.dart @@ -20,7 +20,6 @@ var tests = [ expect(result['isolateFlags'].length, isPositive); expect(result['isSystemIsolate'], isFalse); expect(result['isolateGroupId'], startsWith('isolateGroups/')); - expect(result['_originNumber'], equals(result['number'])); expect(result['startTime'], isPositive); expect(result['livePorts'], isPositive); expect(result['pauseOnExit'], isFalse); diff --git a/runtime/observatory/tests/service/http_get_isolate_rpc_common.dart b/runtime/observatory/tests/service/http_get_isolate_rpc_common.dart index 8aae0aa4966..1fa7d516d11 100644 --- a/runtime/observatory/tests/service/http_get_isolate_rpc_common.dart +++ b/runtime/observatory/tests/service/http_get_isolate_rpc_common.dart @@ -64,7 +64,6 @@ Future testeeBefore() async { Expect.equals(result['type'], 'Isolate'); Expect.isTrue(result['id'].startsWith('isolates/')); Expect.type(result['number']); - Expect.equals(result['_originNumber'], result['number']); Expect.isTrue(result['startTime'] > 0); Expect.isTrue(result['livePorts'] > 0); Expect.isFalse(result['pauseOnExit']); diff --git a/runtime/vm/dart_api_impl.cc b/runtime/vm/dart_api_impl.cc index 88062942bc7..142bda92faf 100644 --- a/runtime/vm/dart_api_impl.cc +++ b/runtime/vm/dart_api_impl.cc @@ -1403,7 +1403,6 @@ Dart_CreateIsolateInGroup(Dart_Isolate group_member, Isolate* isolate; isolate = CreateWithinExistingIsolateGroup(member->group(), name, error); if (isolate != nullptr) { - isolate->set_origin_id(member->origin_id()); isolate->set_init_callback_data(child_isolate_data); isolate->set_on_shutdown_callback(shutdown_callback); isolate->set_on_cleanup_callback(cleanup_callback); diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc index 4de28e4baa0..530bf99044f 100644 --- a/runtime/vm/isolate.cc +++ b/runtime/vm/isolate.cc @@ -110,7 +110,7 @@ class VerifyOriginId : public IsolateVisitor { public: explicit VerifyOriginId(Dart_Port id) : id_(id) {} - void VisitIsolate(Isolate* isolate) { ASSERT(isolate->origin_id() != id_); } + void VisitIsolate(Isolate* isolate) { ASSERT(isolate->group()->id() != id_); } private: Dart_Port id_; @@ -733,7 +733,7 @@ void IsolateGroup::ForEach(std::function action) { } void IsolateGroup::RunWithIsolateGroup( - uint64_t id, + Dart_Port id, std::function action, std::function not_found) { ReadRwLocker wl(Thread::Current(), isolate_groups_rwlock_); @@ -1944,7 +1944,6 @@ Isolate* Isolate::InitIsolate(const char* name_prefix, VerifyOriginId id_verifier(result->main_port()); Isolate::VisitIsolates(&id_verifier); #endif - result->set_origin_id(result->main_port()); // First we ensure we enter the isolate. This will ensure we're participating // in any safepointing requests from this point on. Other threads requesting a @@ -2062,17 +2061,6 @@ int64_t Isolate::UptimeMicros() const { return OS::GetCurrentMonotonicMicros() - start_time_micros_; } -Dart_Port Isolate::origin_id() { - MutexLocker ml(&origin_id_mutex_); - return origin_id_; -} - -void Isolate::set_origin_id(Dart_Port id) { - MutexLocker ml(&origin_id_mutex_); - ASSERT((id == main_port_ && origin_id_ == 0) || (origin_id_ == main_port_)); - origin_id_ = id; -} - void Isolate::set_finalizers(const GrowableObjectArray& value) { finalizers_ = value.ptr(); } @@ -3217,8 +3205,6 @@ void Isolate::PrintJSON(JSONStream* stream, bool ref) { if (ref) { return; } - jsobj.AddPropertyF("_originNumber", "%" Pd64 "", - static_cast(origin_id())); int64_t uptime_millis = UptimeMicros() / kMicrosecondsPerMillisecond; int64_t start_time = OS::GetCurrentTimeMillis() - uptime_millis; jsobj.AddPropertyTimeMillis("startTime", start_time); diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h index adf79f8f06b..8272df75cf1 100644 --- a/runtime/vm/isolate.h +++ b/runtime/vm/isolate.h @@ -679,13 +679,13 @@ class IsolateGroup : public IntrusiveDListEntry { Become* become() const { return become_; } void set_become(Become* become) { become_ = become; } - uint64_t id() const { return id_; } + Dart_Port id() const { return id_; } static void Init(); static void Cleanup(); static void ForEach(std::function action); - static void RunWithIsolateGroup(uint64_t id, + static void RunWithIsolateGroup(Dart_Port id, std::function action, std::function not_found); @@ -886,7 +886,7 @@ class IsolateGroup : public IntrusiveDListEntry { static IntrusiveDList* isolate_groups_; static Random* isolate_group_random_; - uint64_t id_ = 0; + Dart_Port id_ = 0; std::unique_ptr store_buffer_; std::unique_ptr heap_; @@ -1065,8 +1065,6 @@ class Isolate : public IntrusiveDListEntry { ASSERT(main_port_ == 0); // Only set main port once. main_port_ = port; } - Dart_Port origin_id(); - void set_origin_id(Dart_Port id); void set_pause_capability(uint64_t value) { pause_capability_ = value; } uint64_t pause_capability() const { return pause_capability_; } void set_terminate_capability(uint64_t value) { @@ -1665,9 +1663,6 @@ class Isolate : public IntrusiveDListEntry { Dart_IsolateCleanupCallback on_cleanup_callback_ = nullptr; char* name_ = nullptr; Dart_Port main_port_ = 0; - // Isolates created by Isolate.spawn have the same origin id. - Dart_Port origin_id_ = 0; - Mutex origin_id_mutex_; uint64_t pause_capability_ = 0; uint64_t terminate_capability_ = 0; void* init_callback_data_ = nullptr; diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index e32cc051c8d..1b23e78ab85 100644 --- a/runtime/vm/object.cc +++ b/runtime/vm/object.cc @@ -26408,7 +26408,7 @@ ReceivePortPtr ReceivePort::New(Dart_Port id, Thread* thread = Thread::Current(); Zone* zone = thread->zone(); const SendPort& send_port = - SendPort::Handle(zone, SendPort::New(id, thread->isolate()->origin_id())); + SendPort::Handle(zone, SendPort::New(id, thread->isolate_group()->id())); #if !defined(PRODUCT) const StackTrace& allocation_location_ = HasStack() ? GetCurrentStackTrace(0) : StackTrace::Handle(); diff --git a/runtime/vm/port.cc b/runtime/vm/port.cc index cd5e893f76e..dc041c982c4 100644 --- a/runtime/vm/port.cc +++ b/runtime/vm/port.cc @@ -203,7 +203,7 @@ Dart_Port PortMap::GetOriginId(Dart_Port id) { // Message handler is a native port instead of an isolate. return ILLEGAL_PORT; } - return isolate->origin_id(); + return isolate->group()->id(); } bool PortMap::IsOwnedByCurrentThread(Dart_Port id) { diff --git a/runtime/vm/service.cc b/runtime/vm/service.cc index a322dc58658..80e0d835092 100644 --- a/runtime/vm/service.cc +++ b/runtime/vm/service.cc @@ -1642,7 +1642,7 @@ static void ActOnIsolateGroup(JSONStream* js, PrintInvalidParamError(js, "isolateGroupId"); return; } - uint64_t isolate_group_id = UInt64Parameter::Parse( + Dart_Port isolate_group_id = Int64Parameter::Parse( String::Handle(String::SubString(s, prefix.Length())).ToCString()); IsolateGroup::RunWithIsolateGroup( isolate_group_id, diff --git a/runtime/vm/service_isolate.cc b/runtime/vm/service_isolate.cc index c484409143c..fba50468b3d 100644 --- a/runtime/vm/service_isolate.cc +++ b/runtime/vm/service_isolate.cc @@ -151,7 +151,7 @@ bool ServiceIsolate::IsRunning() { bool ServiceIsolate::IsServiceIsolateDescendant(Isolate* isolate) { MonitorLocker ml(monitor_); - return isolate->origin_id() == origin_; + return isolate->group()->id() == origin_; } Dart_Port ServiceIsolate::Port() { @@ -297,7 +297,7 @@ void ServiceIsolate::SetServiceIsolate(Isolate* isolate) { isolate_ = isolate; if (isolate_ != nullptr) { ASSERT(isolate->is_service_isolate()); - origin_ = isolate_->origin_id(); + origin_ = isolate_->group()->id(); } }