[L] Change in dart/sdk[main]: [vm/shared] Introduce isolate event loop handling dart api.
Alexander Aprelev (Gerrit)
Alexander Aprelev added 1 comment![]( "Open in Gerrit")
Hi Slava, wanted to share with you what I have so far to check whether we are on the same page regarding the approach for dart implementation of isolate api.
Related details
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov added 11 comments![]( "Open in Gerrit")
// TODO(aam): Ensure isolate group doesn't disappear after we exit,It should not disappear because we have a thread stack which uses it. That would be a serious bug if that could happen. We should block any sort of graceful shutdown for a group if there are threads like this.
Thread::ExitIsolateGroupAsMutator(/*bypass_safepoint=*/false);It's a bit strange that we need to exit isolate group and then re-enter - especially given that we are creating a new isolate within the same isolate group.
I think we should not require exiting the whole group to create an isolate in it - though we should exit the current isolate.
class BorrowScope {nit: `: public ValueObject`.
nit: consider `IsolateBorrowScope`.
Also this class needs a comment explaining what it does and what expectations we should have about its behavior.
Put this class into anonymous namespace if it is only used in this file.
DEFINE_NATIVE_ENTRY(Isolate_shutdown_, 0, 1) {I wonder if we should call this `shutdownSync`?
DEFINE_NATIVE_ENTRY(Isolate_runEventLoopSync_, 0, 1) {nit: We should make sure that we have tests covering situations where mutator slots are exhausted - the code to acquire target isolate should still continue to work correctly and we should not deadlock
String::Handle(String::New("Failed to enter isolate in time"));Should this be "Reached timeout while trying to enter isolate"?
Dart_Handle result_dart_handle = Dart_RunLoop();`Dart_RunLoop` actually runs loop on thread pool, while `runEventLoopSync` is intentionally specified to run on the current thread.
(Please also add a test to cover this requirement).
case Isolate::kYieldMutatorMsg: {If this is an OOB message then it can be processed in the middle of the other normal message because we schedule an interrupt. I think this is problematic because it allows "unnatural" interleaving of Dart code. I would have expected that we send a normal priority message instead.
We probably want a test which checks that this does not happen.
Thread* thread = nullptr;Should this just be
```
auto thread = suspended_thread;
if (thread != nullptr) {
// ...
}
```
/// Execute the given function in the context of the given isolate.nit: please take doc comments from the latest version off the proposal, I made updates based on Lasse's comments.
Related details
- Alexander Aprelev
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev added 11 comments![]( "Open in Gerrit")
thank you for taking a look, Slava!
// TODO(aam): Ensure isolate group doesn't disappear after we exit,It should not disappear because we have a thread stack which uses it. That would be a serious bug if that could happen. We should block any sort of graceful shutdown for a group if there are threads like this.
Yes, makes sense.
Thread::ExitIsolateGroupAsMutator(/*bypass_safepoint=*/false);It's a bit strange that we need to exit isolate group and then re-enter - especially given that we are creating a new isolate within the same isolate group.
I think we should not require exiting the whole group to create an isolate in it - though we should exit the current isolate.
We are exiting isolate group as mutator. I don't think we can be expected to enter isolate (as mutator), which we do when we create new isolate, while still staying as mutator in isolate group, even if isolate is in the same group.
nit: `: public ValueObject`.
nit: consider `IsolateBorrowScope`.Also this class needs a comment explaining what it does and what expectations we should have about its behavior.
Put this class into anonymous namespace if it is only used in this file.
Done
I wonder if we should call this `shutdownSync`?
Done
nit: We should make sure that we have tests covering situations where mutator slots are exhausted - the code to acquire target isolate should still continue to work correctly and we should not deadlock
Done
String::Handle(String::New("Failed to enter isolate in time"));Should this be "Reached timeout while trying to enter isolate"?
Done
`Dart_RunLoop` actually runs loop on thread pool, while `runEventLoopSync` is intentionally specified to run on the current thread.
(Please also add a test to cover this requirement).
Ah, I see. Let me work on it a separate cl.
If this is an OOB message then it can be processed in the middle of the other normal message because we schedule an interrupt. I think this is problematic because it allows "unnatural" interleaving of Dart code. I would have expected that we send a normal priority message instead.
We probably want a test which checks that this does not happen.
Done
Should this just be
```
auto thread = suspended_thread;
if (thread != nullptr) {
// ...
}
```
Done
/// Execute the given function in the context of the given isolate.nit: please take doc comments from the latest version off the proposal, I made updates based on Lasse's comments.
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov added 1 comment![]( "Open in Gerrit")
DEFINE_NATIVE_ENTRY(Isolate_runSync_, 1, 3) {I think this needs to handle the following cases better:
- We are doing `runSync` on the current isolate - should just happen.
- We are dong `runSync` on an isolate which is pinned to the current thread - should just happen.
- We are doing `runSync` on the isolate which is pinned to some other thread - should error
- We are doing `runSync` on an isolate which does not have event loop running - we need to decide what this means, but probably it means we need to acquire its ownership, run the code and then release ownership.
All of this needs correponding tests.
Related details
- Alexander Aprelev
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev added 1 comment![]( "Open in Gerrit")
DEFINE_NATIVE_ENTRY(Isolate_runSync_, 1, 3) {I think this needs to handle the following cases better:
- We are doing `runSync` on the current isolate - should just happen.
- We are dong `runSync` on an isolate which is pinned to the current thread - should just happen.
- We are doing `runSync` on the isolate which is pinned to some other thread - should error
- We are doing `runSync` on an isolate which does not have event loop running - we need to decide what this means, but probably it means we need to acquire its ownership, run the code and then release ownership.
All of this needs correponding tests.
Is there something particular you have in mind regarding handling those cases better?
Pinned-functionality is not implemented yet, it's not part of this CL.
Otherwise case 1 is tested by testRunRunOnCurrentIsolate, case 4 - by testRunOnCooperatingIsolate, testRunOnNonCooperatingIsolate in threading_test.dart.
Related details
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov added 7 comments![]( "Open in Gerrit")
I would like somebody else (in addition to me) to look at this - because it is very subtle code.
I have kinda persuaded myself that this implementation looks reasonable, but maybe we need more eyes.
So I am adding @rma...@google.com as a reviewer.
DEFINE_NATIVE_ENTRY(Isolate_runSync_, 1, 3) {Alexander AprelevI think this needs to handle the following cases better:
- We are doing `runSync` on the current isolate - should just happen.
- We are dong `runSync` on an isolate which is pinned to the current thread - should just happen.
- We are doing `runSync` on the isolate which is pinned to some other thread - should error
- We are doing `runSync` on an isolate which does not have event loop running - we need to decide what this means, but probably it means we need to acquire its ownership, run the code and then release ownership.
All of this needs correponding tests.
Is there something particular you have in mind regarding handling those cases better?
Pinned-functionality is not implemented yet, it's not part of this CL.
Otherwise case 1 is tested by testRunRunOnCurrentIsolate, case 4 - by testRunOnCooperatingIsolate, testRunOnNonCooperatingIsolate in threading_test.dart.
I would like `Isolate.current.runSync` to be short-ciricuited so that we don't do any dances with entering/exiting/borrowing.
Ack regarding pinning functionality. Though note that some notion of pinning already exists (see `Dart_SetCurrentThreadOwnsIsolate` and its implementation plus my comment about it above) so we need to respect it here.
Ignore my comment about event loop - I realized that I misread the code. The message we are sending serves as an "interruption" for event loop, but we are not required to run an event loop to borrow the isolate.
OSThread* owner_tag_ = 0;This needs to be unified with `owner_thread_` / `Isolate::SetOwnerThread`, etc. We can't have multiple different notions of ownership.
bool EnsureOwnership(intptr_t timeout_ms = Monitor::kNoTimeout);Better name `TryAcquireOwnership` (because it can fail).
Maybe document that it can only fail if timeout is supplied.
ASSERT(priority == Isolate::kAsEventAction);Why do we need this dance? Can be just post message like that from the very beginning? It feels strange that we need to post message as OOB first, which then reposts itself as normal message?
(I can see that this is copied from other places - but I think other places are also basically wrong).
void Isolate::HandleBorrowRequests() {It's a bit concerning that worker thread is completely blocked while doing this. I would have imagined that we stop handling messages and return worker thread back to the pool.
{I wonder if this should have a CompareExchange based fast path for fast acquisition of ownership?
Related details
- Alexander Aprelev
- Ryan Macnak
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |