This reland commit 5a32d8bc7c with a fix
for thread leak (Issue #56717): when `ThreadPool` is shutting down
asynchronously the last worker should detach itself to prevent
leaking associated low-level data structures, because no thread will
join it.
A hang in service isolate shutdown (caused by an existing bug) was fixed by commit 157a0dc7f9.
This CL turns native ports into a thin abstraction over underlying
thread pool instead of building them as full fledged MessageHandler.
This allows to easily implement a variation of native ports which can
handle messages concurrently with the given degree of concurrency.
This type of port can be used to greatly simplify implementation of
IOService - which previously had to do its own concurrency management
on top of "single threaded" native ports. This capability is exposed
as `Dart_NewConcurrentNativePort` API.
The new implementation is in general much cleaner then the old one
with one exception: `Dart_CloseNativePort` API has unfortunate design
where underlying message handler is destroyed asynchronously and
`Dart_CloseNativePort` returns immediately without waiting for pending
tasks to complete. Implementing this on top of `ThreadPool` requires
some changes to thread pool implementation.
Issue https://github.com/dart-lang/sdk/issues/55844
Closes https://github.com/dart-lang/sdk/issues/56717
TEST=ci
Change-Id: Ic68bfb60757685afd75c80a70cdec66cc13c149b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/385000
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Slava Egorov <vegorov@google.com>
This reverts commit 5a32d8bc7c.
Reason for revert: DartIsolateTest.CanCreateServiceIsolate flutter engine unit test hangs on Windows.
Original change's description:
> [vm] Simplify implementation of native ports
>
> This CL turns native ports into a thin abstraction over underlying
> thread pool instead of building them as full fledged MessageHandler.
>
> This allows to easily implement a variation of native ports which can
> handle messages concurrently with the given degree of concurrency.
> This type of port can be used to greatly simplify implementation of
> IOService - which previously had to do its own concurrency management
> on top of "single threaded" native ports. This capability is exposed
> as `Dart_NewConcurrentNativePort` API.
>
> The new implementation is in general much cleaner then the old one
> with one exception: `Dart_CloseNativePort` API has unfortunate design
> where underlying message handler is destroyed asynchronously and
> `Dart_CloseNativePort` returns immediately without waiting for pending
> tasks to complete. Implementing this on top of `ThreadPool` requires
> some changes to thread pool implementation.
>
> Issue https://github.com/dart-lang/sdk/issues/55844
>
> TEST=ci
>
> Change-Id: I062040ff233e93962ae93684e9b044d8facdaffc
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/382163
> Commit-Queue: Slava Egorov <vegorov@google.com>
> Reviewed-by: Martin Kustermann <kustermann@google.com>
Change-Id: I7de27793a54072e974bf1a9f17a07c12159a202d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/384481
Commit-Queue: Slava Egorov <vegorov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
This CL turns native ports into a thin abstraction over underlying
thread pool instead of building them as full fledged MessageHandler.
This allows to easily implement a variation of native ports which can
handle messages concurrently with the given degree of concurrency.
This type of port can be used to greatly simplify implementation of
IOService - which previously had to do its own concurrency management
on top of "single threaded" native ports. This capability is exposed
as `Dart_NewConcurrentNativePort` API.
The new implementation is in general much cleaner then the old one
with one exception: `Dart_CloseNativePort` API has unfortunate design
where underlying message handler is destroyed asynchronously and
`Dart_CloseNativePort` returns immediately without waiting for pending
tasks to complete. Implementing this on top of `ThreadPool` requires
some changes to thread pool implementation.
Issue https://github.com/dart-lang/sdk/issues/55844
TEST=ci
Change-Id: I062040ff233e93962ae93684e9b044d8facdaffc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/382163
Commit-Queue: Slava Egorov <vegorov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
The main purpose of the low-level [PortMap] is to coordinate between
ports being opened & closed and concurrent message senders. That's the
only thing it should do.
Each isolate owns [ReceivePort]s. Only the isolate mutator can create
ports, delete them or change their "keeps-isolate-alive" state. Right
now it requires going via [PortMap] (which acquires lock) and
[MessageHandler] (which acquires lock) to change the
"keeps-isolate-alive" state of a port.
We'll move information whether a dart [ReceivePort] is closed and
whether it keeps the isolate alive into the [ReceivePort] object itself.
=> Changing the "keeps-isolate-alive" state of the port no longer
requires any locks. We could even avoid the runtime call itself in a
future CL.
Isolates are kept alive if there's any open receive ports (that have not
been marked as "does not keep isolate alive"). This is a property of an
isolate not of the message handler. For native message handlers we do
have a 1<->1 correspondence between port and handler (i.e. there's no
"number of open ports" tracking needed).
=> We'll move the logic of counting open receive ports and ports that
keep the isolate alive to the [Isolate].
=> We'll also remove locking around incrementing/decrementing or
accessing the counts.
=> The [IsolateMessageHandler] will ask the [Isolate] whether there's
any open ports for determining whether to shut down.
=> For native ports, the `Dart_NewNativePort()` & `Dart_CloseNativePort()`
functions will manage the lifetime (as their name also suggests).
Overall this makes the [Isolate] responsible for creation of dart
[ReceivePort]s and tracking whether the isolate should be kept alive:
* Isolate::CreateReceivePort()
* Isolate::SetReceivePortKeepAliveState()
* Isolate::CloseReceivePort()
TEST=ci
Change-Id: I847ae357c26254d3810cc277962e05deca18a1de
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/317960
Reviewed-by: Alexander Aprelev <aam@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
The current hot-reload implementation [0] will perform a reload by
first sending OOB messages to all isolates and waiting until those OOB
messages are being handled. The handler of the OOB message will block
the thread (and unschedule isolate) and notify the thread performing
reload it's ready.
This requires that all isolates within a group can actually run & block.
This is the case for the VM implementation of isolates (as they are
run an unlimited size thread pool).
Though flutter seems to multiplex several engine isolates on the same OS
thread. Reloading can then result in one engine isolate performing
reload waiting for another to act on the OOB message (which it will not
do as it's multiplexed on the same thread as the former).
Now that we have a more flexible safepointing mechanism (introduced in
[1]) we can utilize for hot reloading by introducing a new "reloading"
safepoint level.
Reload safepoints
-----------------------
We introduce a new safepoint level (SafepointLevel::kGCAndDeoptAndReload).
Being at a "reload safepoint" implies being at a "deopt safepoint"
which implies being at a "gc safepoint".
Code has to explicitly opt-into making safepoint checks participate /
check into "reload safepoints" using [ReloadParticipationScope]. We do
that at certain well-defined places where reload is possible (e.g. event
loop boundaries, descheduling of isolates, OOM message processing, ...).
While running under [NoReloadScope] we disable checking into "reload
safepoints".
Initiator of hot-reload
-----------------------
When a mutator initiates a reload operation (e.g. as part of a
`ReloadSources` `vm-service` API call) it will use a
[ReloadSafepointOperationScope] to get all other mutators to a
safepoint.
For mutators that aren't already at a "reload safepoint", we'll
notify them via an OOB message (instead of scheduling kVMInterrupt).
While waiting for all mutators to check into a "reload safepoint", the
thread is itself at a safepoint (as other mutators may perform lower
level safepoint operations - e.g. GC, Deopt, ...)
Once all mutators are at a "reload safepoint" the thread will take
ownership of all safepoint levels.
Other mutators
-----------------------
Mutators can be at a "reload safepoint" already (e.g. isolate is not
scheduled). If they try to exit safepoint they will block until the
reload operation is finished.
Mutators that are not at a "reload safepoint" (e.g. executing Dart or VM
code) will be sent an OOB message indicating it should check into a
"reload safepoint". We assume mutators make progress until they can
process OOB message.
Mutators may run under a [NoReloadScope] when handling the OOM message.
In that case they will not check into the "reload safepoint" and simply
ignore the message. To ensure the thread will eventually check-in,
we'll make the destructor of [~NoReloadScope] check & send itself a new OOB
message indicating reload should happen. Eventually getting the mutator
to process the OOM message (which is a well-defined place where we can
check into the reload safepoint).
Non-isolate mutators such as the background compiler do not react to OOB
messages. This means that either those mutators have to be stopped (e.g.
bg compiler) before initiating a reload safepoint operation, the
threads have to explicitly opt-into participating in reload safepoints
or the threads have to deschedule themselves eventually.
Misc
----
Owning a reload safepoint operation implies also owning the deopt &
gc safepoint operation. Yet some code would like to ensure it actually
runs under a [DeoptSafepointOperatoinScope]/[GCSafepointOperationScope].
=> The `Thread::OwnsGCSafepoint()` handles that.
While performing hot-reload we may exercise common code (e.g. kernel
loader, ...) that acquires safepoint locks. Normally it's disallows to
acquire safepoint locks while holding a safepoint operation (since
mutators may be stopped at places where they hold locks, creating
deadlock scenarios).
=> We explicitly opt code into participating in reload safepointing
requests. Those well-defined places aren't holding safepoint locks.
=> The `Thread::CanAcquireSafepointLocks()` will return `true` despite
owning a reload operation. (But if one also holds deopt/gc safepoint
operation it will return false)
Example where this matters: As part of hot-reload, we load kernel which
may create new symbols. The symbol creation code may acquire the symbol
lock and `InsertNewOrGet()` a symbol. This is safe as other mutators
don't hold the symbol lock at reload safepoints. The same cannot be said
for Deopt/GC safepoint operations - as they can interrupt code at many
more places where there's no guarantee that no locks are held.
[0] https://dart-review.googlesource.com/c/sdk/+/187461
[1] https://dart-review.googlesource.com/c/sdk/+/196927
Issue https://github.com/flutter/flutter/issues/124546
TEST=Newly added Reload_* tests.
Change-Id: I6842d7d2b284d043cc047fd702b7c5c7dd1fa3c5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/296183
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Slava Egorov <vegorov@google.com>
Our TLAB sizes and maximum new space size constrain the number of
parallel mutator threads we can have. Having too many mutator threads
would cause constant races between threads to acquire TLABs.
In reality we should constrain the number of threads to be at most the
number of cores, since at most that many threads can run in parallel
(i.e. at the same time).
This CL extends the TreadPool implementation to be constrained by a
maximum size. Furthermore it makes each isolate group's have it's own
pool with constrained size and schedule all group member
mutator / message handler tasks on that pool.
Issue https://github.com/dart-lang/sdk/issues/36097
Change-Id: I095c749adad827ab892f33713a32be594d7606d1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/145382
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Alexander Aprelev <aam@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
This CL:
* Moves [Heap]/[SharedClassTable] from [Isolate] to [IsolateGroup], which
will make all isolates in the group use the same heap. The GC will use
the shared class table for object size information.
* Adds support for entering/leaving an isolate group as a helper thread
(e.g. via [Thread::EnterIsolateGroupAsHelper]). The current active
isolate group can be accessed via TLS `IsolateGroup::Current()` or
`Thread::isolate_group_`. When entering as a helper thread there will be
no current isolate.
* Changes the GC to use the above mechanism and ensures GC works without
a currently active isolate. The GC will use information purely available via
[IsolateGroup]. The GC will iterate all isolates within an isolate
group e.g. for scanning roots.
* Makes spawning of new isolates start in their own isolate group.
Once the isolate is fully functional it's heap will be merged into
the original isolate group
* Moves ApiState, containing persistent and weak persistent handles,
from [Isolate] to [IsolateGroup], plus adds appropriate locking.
Issue https://github.com/dart-lang/sdk/issues/36097
Change-Id: Ia8e1d8aa78750e8400864200f4825395a182c004
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/126646
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Right now a message handler is taking care of detecting idle timeouts
and notifying the isolate (or rather it's heap) that a compaction can
start.
Once we move the heap to the isolate group, we should only notify the
heap that we're idle if *all* isolates are idle. As a preparatory step
we move the idle timer functionality into it's own class and make the
isolate own it.
When the heap gets moved from isolate to isolate group, we'll also move
the idle time handler.
Issue https://github.com/dart-lang/sdk/issues/36097
Change-Id: If9b244baf0425ef343f909ee3a8a1147f966845b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/116200
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
This CL moves the thread registry and the safepoint handler to the
[IsolateGroup]. This will cause all threads belonging to the isolate
group to safepoint together.
=> We will therefore start to get an idea of the impact this will have on
pause times.
So far it was only possible to enter a particular isolate (e.g. as mutator
thread or auxiliary thread). This CL adds support for entering an isolate
group (instead of a particular isolate) as an auxiliarly thread. The
current isolate group is available via `IsolateGroup::Current()`.
=> This is a preparation step to let GC threads enter the isolate
group as auxiliary threads, not associated with a particular isolate but
to an isolate group as a whole.
Issue https://github.com/dart-lang/sdk/issues/36097
Change-Id: I7069d07130938d370869f02060570143bfeb1b48
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/108801
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
MessageHandler doesn't actually need a reference to the
ThreadPool::Task, it just wants to know if one is running or not.
Replacing it with a simple boolean will simplify switching
ThreadPool::Task to use std::unique_ptr.
Updates #37244.
Change-Id: Ie69ec38523f009ba559678fd544efa4cc8ead7dd
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/106008
Reviewed-by: Régis Crelier <regis@google.com>
Commit-Queue: Matthew Dempsky <mdempsky@google.com>
Message is a C++ type with a simple ownership model appropriate for
std::unique_ptr. This CL applies the following changes:
1. All uses of "new Message(...)" are replaced with
"Message::New(...)", which is effectively
"std::make_unique<Message>(...)". (The latter was only added in C++14,
but Dart still compiles in C++11 mode.)
2. All owning Message* are replaced with std::unique_ptr<Message>. The
notable exception is MessageQueue, which still uses raw Message*
internally to simplify the linked list handling.
3. All "delete message;" statements are removed.
4. Uses of "NULL" replaced with "nullptr" as necessary.
Change-Id: I05b5804289f2a225bfa05d3c1631129358fed373
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/101222
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
Commit-Queue: Matthew Dempsky <mdempsky@google.com>
This fixes an issue where service RPCs are not answered by the
vm-service.
Currently new isolates are visible via the service protocol immediately
after creation. This means a vm-service client can start doing service
RPCs to a particular isolate. If the embedder has not-yet installed a message
handler the OOB messages will be queued up.
Once the embedder installs an isolate message notify handler, that
handler needs to be invoked if there are pending messages.
Change-Id: Ie75878daf55a3b380e2b2ec7930e15b002f3f520
Reviewed-on: https://dart-review.googlesource.com/c/83680
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Clement Skau <cskau@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
Rework the idle notification mechanism to not use the global
IdleNotifier class as it was resulting in lock inversion issues
causing the monitor to be deleted while it could still be
required in another thread.
The new mechanism wires the idle notification check into the existing
message handling loop and uses the 'paused_for_messages_' mechanism
to wake up a idle time waiting handler when new messages arrive.
Bug: 34888
Change-Id: Ifd75e5cf4c0b6944d586146fe518b9e3e053a76e
Reviewed-on: https://dart-review.googlesource.com/c/82705
Commit-Queue: Siva Annamalai <asiva@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
If a thread pool isolate has not processed a message in FLAG_idle_timeout_micros, run idle tasks.
Change-Id: If506d7805eb1213c3d1f9383d835226822012fff
Reviewed-on: https://dart-review.googlesource.com/26004
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Zach Anderson <zra@google.com>
The only fix needed for relanding is adding _ensureScheduleImmediate
to the list of vm entrypoints in //runtime/vm/compiler/aot/precompiler.cc
Original commit message:
Adds a top-level call waitForEventSync to dart:io that blocks the
thread an Isolate is running on until messages are available.
Before the thread blocks, the microtask queue is drained.
Before waitForEventSync returns, all messages are handled.
Lifting this up from a comment:
This is apropos of the request that nweiz@ sent to the mailing list a
couple weeks back. I'm not sure we should land this. We certainly
shouldn't land it without some annotations that will make the analyzer
complain a lot in most configurations, but I don't know what those
annotations are.
fixes#31102
Change-Id: Id96de46cc5f10e1847045cfafb7cfed6a38bce16
Reviewed-on: https://dart-review.googlesource.com/28920
Reviewed-by: Siva Annamalai <asiva@google.com>
Commit-Queue: Zach Anderson <zra@google.com>
Adds a top-level call waitForEventSync to dart:io that blocks the
thread an Isolate is running on until messages are available.
Before the thread blocks, the microtask queue is drained.
Before waitForEventSync returns, all messages are handled.
Lifting this up from a comment:
This is apropos of the request that nweiz@ sent to the mailing list a
couple weeks back. I'm not sure we should land this. We certainly
shouldn't land it without some annotations that will make the analyzer
complain a lot in most configurations, but I don't know what those
annotations are.
Change-Id: If8286f4525994a162dd4f4563fefccb9d0984f7c
Reviewed-on: https://dart-review.googlesource.com/25281
Commit-Queue: Zach Anderson <zra@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>
Status:
- Currently the getIsolate API tries to guess the status of the isolate
when it is in between the IsolateRunnable and PauseStart, by reporting
it as already in the PauseStart status.
This can potentially make flaky all the tests that uses PauseStart to
synchronize with the actual status of the Isolate.
- In the previous situation the timestamp of the event is guessed too,
potentially reporting a wrong value.
- Even with the previous fix the is still a race condition that can lead
to a getIsolate responde with a PauseStart status to be sent before
the actual PauseStart event.
Changes:
- Fallback to None pause event if the isolate is in between
IsolateRunnable and PauseStart, avoiding double posting.
- Send the PauseStart event before the actual status change, partially
avoiding the race conditions.
- Set the pause timestamp before the actual status change, fully
avoiding the race condition.
Closes https://github.com/dart-lang/sdk/issues/28624R=asiva@google.com, rmacnak@google.com
Review-Url: https://codereview.chromium.org/3006883002 .
When a MessageHandler gets to the paused_on_exist state it should just handle service events.
If ports are open though, at the next message the isolate will consider itself still alive and handle any kind of request.
The MessageHandler is now checking for its paused_on_exit state and avoid to handle normal messages in that state.
Closes https://github.com/dart-lang/sdk/issues/30555R=asiva@google.com
Review-Url: https://codereview.chromium.org/3010503002 .
At the moment isolated has a name field which is on the main_port the
only exception is the vm-service isolate.
The name is used just for logging.
The service protocol though uses an extra field called debugger_name,
that at this moment is redundant and is optimized away during
compilation.
BUG=
R=rmacnak@google.com, zra@google.com
This CL:
- Removes the debugger_name in favor of a modifiable name
- Adds to all the log where the name was logged the main_port (that is
the only real identifier)
- Changes the default format of the isolate name from:
<script_uri>$<main>
to:
<script_uri>:<main>()
Review-Url: https://codereview.chromium.org/3004563003 .
- [x] The first caller of the tag handler blocks, recursive callers queue work and exit.
- [x] Use a NativeMessageHandler to receive I/O results from the service isolate.
- [x] Preserve load error message format.
- [x] Move packages map into service isolate.
- [x] Wire up Todd's native URI code.
R=turnidge@google.com
Review URL: https://codereview.chromium.org/1998963003 .
Vyacheslav Egorov