VM thread shutdown.

VM thread shutdown.

BUG=
R=iposva@google.com, turnidge@google.com

Committed: https://github.com/dart-lang/sdk/commit/7093f2996bf59d89eaf4ab4271b223961c4794f4

[zra, 2015-08-11 22:34:45]

zra@google.com changed reviewers:
+ turnidge@google.com

[zra, 2015-08-11 22:34:47]

Turns out EventHandler::Stop was never called anywhere. John's theory that we
might have been taking the exit code from it instead of the main thread's call
to exit() might be true. Fastest way to see if that's the case would be to take
this for a spin on the bots.

[zra, 2015-08-12 02:23:34]

The bots are a bit red at the moment, so this probably isn't a good plan. I will
test this solution locally this evening.

[zra, 2015-08-13 21:45:14]

zra@google.com changed reviewers:
+ iposva@google.com

[zra, 2015-08-13 21:45:15]

Ivan, the thread pool thread joining changes we discussed earlier today are now
in this CL.

I am splitting off the windows changes into a separate CL here:

https://codereview.chromium.org/1291163002/

and will ping it when it is ready for review.

[Ivan Posva, 2015-08-17 13:35:52]

First round of comments.

-Ivan

https://codereview.chromium.org/1275353005/diff/80001/runtime/bin/gen_snapsho...
File runtime/bin/gen_snapshot.cc (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/bin/gen_snapsho...
runtime/bin/gen_snapshot.cc:555: free(error);
This can lead to problems if the embedder and the Dart VM use different malloc
packages. Please file a bug to clean up the API with regards to this.

https://codereview.chromium.org/1275353005/diff/80001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1042: char* error =
Dart_Initialize(vm_isolate_snapshot_buffer,
Shouldn't the API return a const char*?

https://codereview.chromium.org/1275353005/diff/80001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/include/dart_ap...
runtime/include/dart_api.h:42: #define CHECK_RESULT
?

https://codereview.chromium.org/1275353005/diff/80001/runtime/include/dart_ap...
runtime/include/dart_api.h:867: DART_EXPORT char* Dart_Initialize(
const char*

https://codereview.chromium.org/1275353005/diff/80001/runtime/include/dart_ap...
runtime/include/dart_api.h:885: DART_EXPORT char* Dart_Cleanup();
const char*

https://codereview.chromium.org/1275353005/diff/80001/runtime/tests/vm/dart/s...
File runtime/tests/vm/dart/spawn_shutdown_test.dart (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/tests/vm/dart/s...
runtime/tests/vm/dart/spawn_shutdown_test.dart:19: for (int i = 0; i < count;
i++) {
We should just continuously be spawning isolates as discussed. Because otherwise
you run the risk that the we will have spawned all isolates before the timer
expires.

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:1676: OS::PrintErr("RunLoop returning error\n");
Debug print left over?

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1996: void Isolate::KillIsolate(Isolate* isolate) {
Why is this not an instance method?

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2023:
isolate->ScheduleInterrupts(Isolate::kMessageInterrupt);
What is this interrupt for? It will likely be handled before the message is
enqueued below.

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2052: Isolate::KillIsolate(isolate);
ditto

isolate->Kill();

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/os_thread_an...
File runtime/vm/os_thread_android.cc (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/os_thread_an...
runtime/vm/os_thread_android.cc:153: ThreadId OSThread::GetCurrentThreadId() {
This is currently used by the profiler and relies of gettid().

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/thread.cc
File runtime/vm/thread.cc (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:138: // here can lead to deadlock. Evaluate doing this
check some other way.
Please explain why it can lead to a deadlock in the comment.

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/thread_pool.cc
File runtime/vm/thread_pool.cc (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/thread_pool....
runtime/vm/thread_pool.cc:403: pool_->ReapExitedIdleThreads();
There is no need to hold the monitor_ when calling ReadExitedIdleThreads().
Moving it up after "delete task" avoid lock ordering questions.

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/thread_pool.h
File runtime/vm/thread_pool.h (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/thread_pool....
runtime/vm/thread_pool.h:83: bool done_;
Please move bool fields together.

[zra, 2015-08-18 06:23:14]

https://codereview.chromium.org/1275353005/diff/80001/runtime/bin/gen_snapsho...
File runtime/bin/gen_snapshot.cc (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/bin/gen_snapsho...
runtime/bin/gen_snapshot.cc:555: free(error);
On 2015/08/17 13:35:51, Ivan Posva wrote:
> This can lead to problems if the embedder and the Dart VM use different malloc
> packages. Please file a bug to clean up the API with regards to this.

Filed: https://github.com/dart-lang/sdk/issues/24110

https://codereview.chromium.org/1275353005/diff/80001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1042: char* error =
Dart_Initialize(vm_isolate_snapshot_buffer,
On 2015/08/17 13:35:51, Ivan Posva wrote:
> Shouldn't the API return a const char*?

free() takes a non-const pointer, so the compiler is mad when you try to give it
a const char*.

https://codereview.chromium.org/1275353005/diff/80001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/include/dart_ap...
runtime/include/dart_api.h:42: #define CHECK_RESULT
On 2015/08/17 13:35:51, Ivan Posva wrote:
> ?

Removed

https://codereview.chromium.org/1275353005/diff/80001/runtime/include/dart_ap...
runtime/include/dart_api.h:867: DART_EXPORT char* Dart_Initialize(
On 2015/08/17 13:35:51, Ivan Posva wrote:
> const char*

See above.

https://codereview.chromium.org/1275353005/diff/80001/runtime/include/dart_ap...
runtime/include/dart_api.h:885: DART_EXPORT char* Dart_Cleanup();
On 2015/08/17 13:35:51, Ivan Posva wrote:
> const char*

See above.

https://codereview.chromium.org/1275353005/diff/80001/runtime/tests/vm/dart/s...
File runtime/tests/vm/dart/spawn_shutdown_test.dart (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/tests/vm/dart/s...
runtime/tests/vm/dart/spawn_shutdown_test.dart:19: for (int i = 0; i < count;
i++) {
On 2015/08/17 13:35:51, Ivan Posva wrote:
> We should just continuously be spawning isolates as discussed. Because
otherwise
> you run the risk that the we will have spawned all isolates before the timer
> expires.

Done.

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:1676: OS::PrintErr("RunLoop returning error\n");
On 2015/08/17 13:35:51, Ivan Posva wrote:
> Debug print left over?

Removed.

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1996: void Isolate::KillIsolate(Isolate* isolate) {
On 2015/08/17 13:35:51, Ivan Posva wrote:
> Why is this not an instance method?

Done.

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2023:
isolate->ScheduleInterrupts(Isolate::kMessageInterrupt);
On 2015/08/17 13:35:51, Ivan Posva wrote:
> What is this interrupt for? It will likely be handled before the message is
> enqueued below.

I was following the pattern in the implementation of Dart_InterruptIsolate:
Schedule the interrupt, then post the message. If that's wrong, I'll fix the
order there, too.

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2052: Isolate::KillIsolate(isolate);
On 2015/08/17 13:35:51, Ivan Posva wrote:
> ditto
> 
> isolate->Kill();

Done.

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/os_thread_an...
File runtime/vm/os_thread_android.cc (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/os_thread_an...
runtime/vm/os_thread_android.cc:153: ThreadId OSThread::GetCurrentThreadId() {
On 2015/08/17 13:35:52, Ivan Posva wrote:
> This is currently used by the profiler and relies of gettid().

Restored gettid() here, and added a GetCurrentThreadJoinId() call to OSThread to
return pthread_self().

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/thread.cc
File runtime/vm/thread.cc (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:138: // here can lead to deadlock. Evaluate doing this
check some other way.
On 2015/08/17 13:35:52, Ivan Posva wrote:
> Please explain why it can lead to a deadlock in the comment.

Done.

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/thread_pool.cc
File runtime/vm/thread_pool.cc (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/thread_pool....
runtime/vm/thread_pool.cc:403: pool_->ReapExitedIdleThreads();
On 2015/08/17 13:35:52, Ivan Posva wrote:
> There is no need to hold the monitor_ when calling ReadExitedIdleThreads().
> Moving it up after "delete task" avoid lock ordering questions.

Done.

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/thread_pool.h
File runtime/vm/thread_pool.h (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/vm/thread_pool....
runtime/vm/thread_pool.h:83: bool done_;
On 2015/08/17 13:35:52, Ivan Posva wrote:
> Please move bool fields together.

Done.

[Ivan Posva, 2015-08-19 08:31:07]

Comments from discussion today...

-Ivan

https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1981: MonitorLocker ml(isolates_list_monitor_);
We should probably change this to a Mutex. Different CL.

https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/service_iso...
File runtime/vm/service_isolate.cc (right):

https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/service_iso...
runtime/vm/service_isolate.cc:757: isolate_->Kill();
Need to make sure we own the isolate list lock when coming here.

[Florian Schneider, 2015-08-19 09:02:02]

fschneider@google.com changed reviewers:
+ fschneider@google.com

[Florian Schneider, 2015-08-19 09:02:03]

dbc:

https://codereview.chromium.org/1275353005/diff/80001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/1275353005/diff/80001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1079: error = Dart_Cleanup();
Same question as Ivan:

Dart::Cleanup() return a const char*, so why not just pass it on, and remove the
free(error) call below.

https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1283: return strdup(err_msg);
What the reason for strdup here? Could you just pass on the const char*?

return err_msg;

This would eliminate the need to free() the return value.

https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1367: *error = strdup("Isolate creation failed");
Same here.

https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1391: *error = strdup(error_obj.ToErrorCString());
Same here.

If I see this correctly, error_obj.ToErrorCString() creates a zone-allocated
C-string, which will be freed with the zone destructor.

[Ivan Posva, 2015-08-19 09:21:49]

https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1367: *error = strdup("Isolate creation failed");
On 2015/08/19 09:02:03, Florian Schneider wrote:
> Same here.

Needs the strdup because of the contract about error life time.

https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1391: *error = strdup(error_obj.ToErrorCString());
On 2015/08/19 09:02:03, Florian Schneider wrote:
> Same here.
> 
> If I see this correctly, error_obj.ToErrorCString() creates a zone-allocated
> C-string, which will be freed with the zone destructor.

We pack it into a malloc'd string since that is the contract we have with the
caller of the API. The reason the contract is "return a string in malloc
memory": The zone that holds the ToErrorCString result is going away two lines
further down.

[Florian Schneider, 2015-08-19 09:26:39]

On 2015/08/19 09:21:49, Ivan Posva wrote:
>
https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/dart_api_im...
> File runtime/vm/dart_api_impl.cc (right):
> 
>
https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/dart_api_im...
> runtime/vm/dart_api_impl.cc:1367: *error = strdup("Isolate creation failed");
> On 2015/08/19 09:02:03, Florian Schneider wrote:
> > Same here.
> 
> Needs the strdup because of the contract about error life time.
> 
>
https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/dart_api_im...
> runtime/vm/dart_api_impl.cc:1391: *error = strdup(error_obj.ToErrorCString());
> On 2015/08/19 09:02:03, Florian Schneider wrote:
> > Same here.
> > 
> > If I see this correctly, error_obj.ToErrorCString() creates a zone-allocated
> > C-string, which will be freed with the zone destructor.
> 
> We pack it into a malloc'd string since that is the contract we have with the
> caller of the API. The reason the contract is "return a string in malloc
> memory": The zone that holds the ToErrorCString result is going away two lines
> further down.

Ok, makes sense. I was just about to answer my own question :) Do we need
dynamically allocated error messages from the API, or would an integer that
indexes into a static table of string literals do fine?

[zra, 2015-08-25 18:19:30]

Simplified thread pool locking, and ensured a consistent lock order for sending
the kill message.

https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1981: MonitorLocker ml(isolates_list_monitor_);
On 2015/08/19 08:31:06, Ivan Posva wrote:
> We should probably change this to a Mutex. Different CL.

Acknowledged.

https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/service_iso...
File runtime/vm/service_isolate.cc (right):

https://codereview.chromium.org/1275353005/diff/120001/runtime/vm/service_iso...
runtime/vm/service_isolate.cc:757: isolate_->Kill();
On 2015/08/19 08:31:06, Ivan Posva wrote:
> Need to make sure we own the isolate list lock when coming here.

Added static KillIfExists to Isolate. It uses the IsoalteVisitor, which takes
the list lock.

[zra, 2015-09-08 16:22:19]

This change is ready for review. Please take a fresh look.

[zra, 2015-09-09 17:13:56]

On 2015/09/08 16:22:19, zra wrote:
> This change is ready for review. Please take a fresh look.

Added flag to avoid need to revert, as discussed yesterday.

[zra, 2015-09-10 19:42:53]

On 2015/09/09 17:13:56, zra wrote:
> On 2015/09/08 16:22:19, zra wrote:
> > This change is ready for review. Please take a fresh look.
> 
> Added flag to avoid need to revert, as discussed yesterday.

Ping

[Ivan Posva, 2015-09-14 21:52:14]

-Ivan

https://codereview.chromium.org/1275353005/diff/280001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/1275353005/diff/280001/runtime/bin/main.cc#ne...
runtime/bin/main.cc:1205: Process::TerminateExitCodeHandler();
Why is this done unconditionally, while the EventHandler and
DebuggerConnectionHandle are stopped only if do_vm_shutdown is true?

https://codereview.chromium.org/1275353005/diff/280001/runtime/tests/vm/dart/...
File runtime/tests/vm/dart/spawn_infinite_loop_test.dart (right):

https://codereview.chromium.org/1275353005/diff/280001/runtime/tests/vm/dart/...
runtime/tests/vm/dart/spawn_infinite_loop_test.dart:12: while (true) {}
How do you ensure that the spawned isolate has actually reached the point of
looping before the whole test is brought down?

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/isolate.h#n...
runtime/vm/isolate.h:763: void Kill();
Since Kill assumes the the monitor is locked then it should be named
KillLocked(). Also why is this a public method if the monitor is private?

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool.cc
File runtime/vm/thread_pool.cc (right):

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:106: if (current->id_ !=
OSThread::GetCurrentThreadId()) {
Cache the current thread it outside the loop?

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:121: // Join non-idle threads.
Move outside the exit_monitor_ block.

Verify that no workers are added to the join list and synchronize properly on
the required lock, even if not exactly necessary during shutdown as we should be
the only worker running now...

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:268: void ThreadPool::JoinList::Add(ThreadJoinId id,
JoinList** list) {
AddLocked(). Ideally we want to assert that we hold the ThreadPool mutex.

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:274: while (*list) {
*list is not a bool

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:418: JoinList::Add(worker->join_id_,
&worker->pool_->join_list_);
JoinList should be consistent on the lock which protects it. Once we added this
thread's id, we can proceed with the exit.

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:426: // is not due to a ThreadPool Shutdown. Thus, we
simply delete the worker.
Please add comment: If there is a shutdown race, then we will be blocked on this
thread being joined before we entirely unload.

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool...
File runtime/vm/thread_pool_test.cc (right):

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool...
runtime/vm/thread_pool_test.cc:79: explicit SleepTask(
Drop the explicit!

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/verified_me...
File runtime/vm/verified_memory_test.cc (right):

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/verified_me...
runtime/vm/verified_memory_test.cc:20: FLAG_verified_mem = false;
Please explain why this was needed.

[zra, 2015-09-14 22:59:01]

https://codereview.chromium.org/1275353005/diff/280001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/1275353005/diff/280001/runtime/bin/main.cc#ne...
runtime/bin/main.cc:1205: Process::TerminateExitCodeHandler();
On 2015/09/14 21:52:13, Ivan Posva wrote:
> Why is this done unconditionally, while the EventHandler and
> DebuggerConnectionHandle are stopped only if do_vm_shutdown is true?

Process::TerminateExitCodeHandler() was already being called at other places the
VM is about to exit, so adding the call here was more for consistency than for
the shutdown change.

https://codereview.chromium.org/1275353005/diff/280001/runtime/tests/vm/dart/...
File runtime/tests/vm/dart/spawn_infinite_loop_test.dart (right):

https://codereview.chromium.org/1275353005/diff/280001/runtime/tests/vm/dart/...
runtime/tests/vm/dart/spawn_infinite_loop_test.dart:12: while (true) {}
On 2015/09/14 21:52:13, Ivan Posva wrote:
> How do you ensure that the spawned isolate has actually reached the point of
> looping before the whole test is brought down?

There's no check for that here. This is mostly a sanity check. The other test
I've added exercises the case where an isolate is looping reliably.

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/isolate.h#n...
runtime/vm/isolate.h:763: void Kill();
On 2015/09/14 21:52:13, Ivan Posva wrote:
> Since Kill assumes the the monitor is locked then it should be named
> KillLocked(). Also why is this a public method if the monitor is private?

Done.

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool.cc
File runtime/vm/thread_pool.cc (right):

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:106: if (current->id_ !=
OSThread::GetCurrentThreadId()) {
On 2015/09/14 21:52:14, Ivan Posva wrote:
> Cache the current thread it outside the loop?

Done.

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:121: // Join non-idle threads.
On 2015/09/14 21:52:13, Ivan Posva wrote:
> Move outside the exit_monitor_ block.
> 
> Verify that no workers are added to the join list and synchronize properly on
> the required lock, even if not exactly necessary during shutdown as we should
be
> the only worker running now...

Done.

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:268: void ThreadPool::JoinList::Add(ThreadJoinId id,
JoinList** list) {
On 2015/09/14 21:52:13, Ivan Posva wrote:
> AddLocked(). Ideally we want to assert that we hold the ThreadPool mutex.

Done.

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:274: while (*list) {
On 2015/09/14 21:52:14, Ivan Posva wrote:
> *list is not a bool

Done.

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:418: JoinList::Add(worker->join_id_,
&worker->pool_->join_list_);
On 2015/09/14 21:52:14, Ivan Posva wrote:
> JoinList should be consistent on the lock which protects it. Once we added
this
> thread's id, we can proceed with the exit.

Done.

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:426: // is not due to a ThreadPool Shutdown. Thus, we
simply delete the worker.
On 2015/09/14 21:52:13, Ivan Posva wrote:
> Please add comment: If there is a shutdown race, then we will be blocked on
this
> thread being joined before we entirely unload.

Done.

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool...
File runtime/vm/thread_pool_test.cc (right):

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/thread_pool...
runtime/vm/thread_pool_test.cc:79: explicit SleepTask(
On 2015/09/14 21:52:14, Ivan Posva wrote:
> Drop the explicit!

Done.

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/verified_me...
File runtime/vm/verified_memory_test.cc (right):

https://codereview.chromium.org/1275353005/diff/280001/runtime/vm/verified_me...
runtime/vm/verified_memory_test.cc:20: FLAG_verified_mem = false;
On 2015/09/14 21:52:14, Ivan Posva wrote:
> Please explain why this was needed.

Done.

[Ivan Posva, 2015-09-15 16:18:43]

LGTM with one last *Locked rename.

Thanks,
-Ivan

https://codereview.chromium.org/1275353005/diff/300001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1275353005/diff/300001/runtime/vm/isolate.h#n...
runtime/vm/isolate.h:779: void Kill();
KillLocked to indicate that the lock should be held. Such as in
TryAllocateLocked.

[turnidge, 2015-09-15 16:48:22]

lgtm w/ 2 minor comments.

Ok with me once other reviewers sign off.

https://codereview.chromium.org/1275353005/diff/300001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://codereview.chromium.org/1275353005/diff/300001/runtime/vm/dart.cc#new...
runtime/vm/dart.cc:218: // Shut dwon profiling.
dwon -> down

https://codereview.chromium.org/1275353005/diff/300001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1275353005/diff/300001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1530: ISOLATE_METRIC_LIST(ISOLATE_METRIC_PRINT);
ISOLATE_METRIC_LIST tabbed in by 2 more here since it isn't part of the macro
body?  A blank line between the macro and its use might not hurt.

[turnidge, 2015-09-15 17:19:59]

Found some additional stuff while sitting with zra...

https://codereview.chromium.org/1275353005/diff/300001/runtime/vm/thread_pool.cc
File runtime/vm/thread_pool.cc (right):

https://codereview.chromium.org/1275353005/diff/300001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:107: if (current->id_ != id) {
May need worker lock while reading current->id.

https://codereview.chromium.org/1275353005/diff/300001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:222: count_running_--;
Add blank line and a comment...

https://codereview.chromium.org/1275353005/diff/300001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:419: worker->join_id_ =
OSThread::GetCurrentThreadJoinId();
cache these in locals so we don't need to lock to get them below...

https://codereview.chromium.org/1275353005/diff/300001/runtime/vm/thread_pool...
runtime/vm/thread_pool.cc:439: worker->join_id_ =
OSThread::kInvalidThreadJoinId;
Setting these not under lock should work, but is inconsistent.  Could do
debug-only....

[turnidge, 2015-09-15 17:46:42]

https://codereview.chromium.org/1275353005/diff/300001/runtime/vm/thread_pool.h
File runtime/vm/thread_pool.h (right):

https://codereview.chromium.org/1275353005/diff/300001/runtime/vm/thread_pool...
runtime/vm/thread_pool.h:66: ThreadId id() const { return id_; }
From offline: iposva suggests putting locking inside this accessor...

https://codereview.chromium.org/1275353005/diff/300001/runtime/vm/thread_pool...
runtime/vm/thread_pool.h:67: ThreadJoinId join_id() const { return join_id_; }
From offline discussions with zra, iposva, and me -- we can get rid of join_id()
and join_id_ entirely and always just use the OSThread::CurrentJoinWhateverId().

[zra, 2015-09-15 19:49:56]

Committed patchset #18 (id:340001) manually as
7093f2996bf59d89eaf4ab4271b223961c4794f4 (presubmit successful).