Use the ThreadPool for all isolates and native ports. Previously,

Use the ThreadPool for all isolates and native ports.  Previously,
each isolate or native port had a dedicated thread.

Refactored the MessageHandler api...

- Added a Run function to allow a MessageHandler to run on a
  ThreadPool.  These functions take a start and end callback to allow for
  isolate initialization and shutdown.

- Made the queue private to the MessageHandler and moved all message
  processing code inside the MessageHandler (got rid of all of the
  different flavors of RunLoop).  This helps remove some code
  duplication and hides the details of how messages are handled.

- Moved all locking and notification out of MessageQueue and moved it
  up to MessageHandler.  Moved OOB support out of MessageQueue and up
  to MessageHandler.  These changes make the MessageQueue much
  simpler.

- Refactored native port and isolate MessageHandlers to share more code.

- Improved --trace_isolates output.

- Added tests for MessageHandler.

Refactored lib/isolate code...

- Use the new MessageHandler::Run api.

- Got rid of the LongJump stuff in RunIsolate.  No longer needed.

- Use the new StartIsolateScope/SwitchIsolateScope to make the code
  less verbose and less error-prone.

- Store top-level isolate errors in the sticky_error.

Added StartIsolateScope/SwitchIsolateScope classes.
Committed: https://code.google.com/p/dart/source/detail?r=6762

[Ivan Posva, 2012-04-08 22:58:27]

First round of comments.

-Ivan

https://chromiumcodereview.appspot.com/9924015/diff/1/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://chromiumcodereview.appspot.com/9924015/diff/1/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:298: if (new_isolate) {
if (new_isolate != NULL) {

https://chromiumcodereview.appspot.com/9924015/diff/1/runtime/vm/message_hand...
File runtime/vm/message_handler.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/1/runtime/vm/message_hand...
runtime/vm/message_handler.cc:246: error = !start_function_(start_data_);
I find this style a bit hard to read. The double-negative tends to throw me off.
How about:
bool ok = true;
...
  ok = start_function_(start_data_);
...
if (ok) {
  ok = HandleMessages();
}

https://chromiumcodereview.appspot.com/9924015/diff/1/runtime/vm/native_messa...
File runtime/vm/native_message_handler.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/1/runtime/vm/native_messa...
runtime/vm/native_message_handler.cc:49: UNIMPLEMENTED();
Should OOB messages be accepted by the native message handlers at all?

[turnidge, 2012-04-11 19:37:16]

Ready for another look.

https://chromiumcodereview.appspot.com/9924015/diff/1/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://chromiumcodereview.appspot.com/9924015/diff/1/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:298: if (new_isolate) {
On 2012/04/08 22:58:27, Ivan Posva wrote:
> if (new_isolate != NULL) {

Done.

https://chromiumcodereview.appspot.com/9924015/diff/1/runtime/vm/message_hand...
File runtime/vm/message_handler.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/1/runtime/vm/message_hand...
runtime/vm/message_handler.cc:246: error = !start_function_(start_data_);
True.  Fixed.

On 2012/04/08 22:58:27, Ivan Posva wrote:
> I find this style a bit hard to read. The double-negative tends to throw me
off.
> How about:
> bool ok = true;
> ...
>   ok = start_function_(start_data_);
> ...
> if (ok) {
>   ok = HandleMessages();
> }

https://chromiumcodereview.appspot.com/9924015/diff/1/runtime/vm/native_messa...
File runtime/vm/native_message_handler.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/1/runtime/vm/native_messa...
runtime/vm/native_message_handler.cc:49: UNIMPLEMENTED();
On 2012/04/08 22:58:27, Ivan Posva wrote:
> Should OOB messages be accepted by the native message handlers at all?

With the current setup, HandleMessage is virtual, so it seemed to make sense to
me detect oob messages here.  Another option would be to add another virtual
HandleOOBMessage and only implement that for IsolateMessageHandler.  I could do
that if you'd like.

In the future, I could see using a native port to implement port forwarding to
other servers.  In that case, it might make sense to allow oob messages to
forward to other servers.  But there is currently no use for oob messages in
native ports.

[siva, 2012-04-14 00:29:53]

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/lib/isolate....
runtime/lib/isolate.cc:136: // For now, we only support a non-parameterized or
raw target class.
Is this comment still valid considering the new isolate invocation APIs

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/lib/isolate....
runtime/lib/isolate.cc:168: StoreError(isolate, local_port);
need a "return false"; here?
We don't want to invoke the target function after an error has occurred.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/lib/isolate....
runtime/lib/isolate.cc:183: return true;
In the old code if the new isolate had some errors during invocation (e.g: class
not found or script compilation error etc.) it was shutting itself down. Where
does that happen in the new scenario?

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/lib/isolate....
runtime/lib/isolate.cc:523: if (result != 0) {
result has just been initialized to 0, how can it be non zero, is the check
needed?

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/dart.cc#n...
runtime/vm/dart.cc:45: thread_pool_ = new ThreadPool();
I guess we don't have a Dart::Cleanup(...) where we can
shutdown the thread pool.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/dart_api_...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:775: SetIsolateScope set_scope(NULL);
Why is NULL passed in and not isolate? Won't it end up setting current isolate
to NULL?

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/isolate.c...
runtime/vm/isolate.cc:101: if (message->priority() >= Message::kOOBPriority) {
could we have something like message->IsMirrorMessage() to make it more
readable.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/isolate.h...
runtime/vm/isolate.h:224: class SetIsolateScope {
In two out of the three places where this is used the saved_isolate_ must be
NULL (I think there should be an assertion for that).

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message.cc
File runtime/vm/message.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message.c...
runtime/vm/message.cc:28: if (head_ == NULL) {
ASSERT(tail_ == NULL)?

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message.h
File runtime/vm/message.h (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message.h...
runtime/vm/message.h:71: // message is available.
Perhaps you should add in the comment that this does not block at all to be more
explicit.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
File runtime/vm/message_handler.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler.cc:69: MonitorLocker ml(&monitor_);
What is this lock protecting?

It seems like it is possible to change values of blocking_, start_function_ etc.
while a task_ is being run in the pool_

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler.cc:89: MonitorLocker ml(&monitor_);
Ditto question about what is this lock protecting.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler.cc:138: pool_->Run(task_);
The task_ is run with the lock being held whereas in the Run method above you
were releasing the lock, why the variance?

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler.cc:168: monitor_.Exit();
Where is the monitor_.Enter() corresponding to this Exit in the first iteration
of the while loop?

If the monitor is being held on call to this method then we probably need an
assertion that the lock is indeed held.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler.cc:209: }
HandleNextMessage seems identical to HandleMessages is there a possibility of
some code sharing?

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler.cc:240: MonitorLocker ml(&monitor_);
Is the lock being done here primarily to protect access to start_function_ ?

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler.cc:286: void
MessageHandler::CheckTermination(MonitorLocker* ml, bool error) {
Need an assertion that the lock is indeed held when this method is called.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
File runtime/vm/message_handler.h (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler.h:77: // These functions should only be called from
the PortMap.
How do we ensure that they are only called from PortMap, maybe make them
protected and PortMap a friend class here?

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler.h:144: intptr_t live_ports_;
Missing DISALLOW_COPY_AND_ASSIGN(....);

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
File runtime/vm/message_handler_test.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler_test.cc:16: MessageQueue* oob_queue() { return
handler_->oob_queue_; }
const qualifier for these two accessor methods.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler_test.cc:19: MessageHandler* handler_;
Missing DISALLOW_COPY_AND_ASSIGN(...).

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler_test.cc:71: bool result_;
DISALLOW_COPY_AND_ASSIGN...

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler_test.cc:151: handler.PostMessage(oob_message1);
You should probably post this before message2 to mix things up a bit.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_t...
File runtime/vm/message_test.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_t...
runtime/vm/message_test.cc:20: bool result = (queue_->head_ != NULL ||
queue_->head_ != NULL);
why repeat the condition twice?

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/port_test.cc
File runtime/vm/port_test.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/port_test...
runtime/vm/port_test.cc:139: 
Into which file did the ThreadedPort test go.

[turnidge, 2012-04-17 23:46:54]

Hi Siva,

Thanks for your comments... you found some serious problems.

Ready for another look.

Todd

https://chromiumcodereview.appspot.com/9924015/diff/1/runtime/vm/native_messa...
File runtime/vm/native_message_handler.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/1/runtime/vm/native_messa...
runtime/vm/native_message_handler.cc:49: UNIMPLEMENTED();
Thought about this some more.  Changed the comment and changed the UNIMPLEMENTED
to UNREACHABLE.

On 2012/04/11 19:37:16, turnidge wrote:
> On 2012/04/08 22:58:27, Ivan Posva wrote:
> > Should OOB messages be accepted by the native message handlers at all?
> 
> With the current setup, HandleMessage is virtual, so it seemed to make sense
to
> me detect oob messages here.  Another option would be to add another virtual
> HandleOOBMessage and only implement that for IsolateMessageHandler.  I could
do
> that if you'd like.
> 
> In the future, I could see using a native port to implement port forwarding to
> other servers.  In that case, it might make sense to allow oob messages to
> forward to other servers.  But there is currently no use for oob messages in
> native ports.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/lib/isolate....
runtime/lib/isolate.cc:136: // For now, we only support a non-parameterized or
raw target class.
On 2012/04/14 00:29:53, asiva wrote:
> Is this comment still valid considering the new isolate invocation APIs

RunIsolate implements the old isolate api.  The new isolate api uses
RunIsolate2.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/lib/isolate....
runtime/lib/isolate.cc:168: StoreError(isolate, local_port);
On 2012/04/14 00:29:53, asiva wrote:
> need a "return false"; here?
> We don't want to invoke the target function after an error has occurred.

Done.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/lib/isolate....
runtime/lib/isolate.cc:183: return true;
On 2012/04/14 00:29:53, asiva wrote:
> In the old code if the new isolate had some errors during invocation (e.g:
class
> not found or script compilation error etc.) it was shutting itself down. Where
> does that happen in the new scenario?

Okay.  Thanks, I had that all wrong.  I made some major changes to the CL to
handle termination correctly.  I have dropped RunBlocking and I now pass an
end_callback to Run() so that each time we run an isolate on the thread pool we
can specify what it should do on termination.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/lib/isolate....
runtime/lib/isolate.cc:523: if (result != 0) {
On 2012/04/14 00:29:53, asiva wrote:
> result has just been initialized to 0, how can it be non zero, is the check
> needed?

The check is no longer needed.  It is now a FATAL in thread_pool.cc.

Do you think that is too severe?  I could restore the old behavior by adding a
return value to ThreadPool::Run()...

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/dart.cc#n...
runtime/vm/dart.cc:45: thread_pool_ = new ThreadPool();
On 2012/04/14 00:29:53, asiva wrote:
> I guess we don't have a Dart::Cleanup(...) where we can
> shutdown the thread pool.

True.  Added a TODO.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/dart_api_...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:775: SetIsolateScope set_scope(NULL);
On 2012/04/14 00:29:53, asiva wrote:
> Why is NULL passed in and not isolate? Won't it end up setting current isolate
> to NULL?

Yeah.  When I call RunBlocking, it actually runs the isolate on another thread
and uses a monitor to tell me when it is done.  It seemed correct to give up the
current isolate before we started to execute it in a different thread.

I have added assertions in RunBlocking that there is no current isolate when
these functions are called.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/isolate.c...
runtime/vm/isolate.cc:101: if (message->priority() >= Message::kOOBPriority) {
On 2012/04/14 00:29:53, asiva wrote:
> could we have something like message->IsMirrorMessage() to make it more
> readable.

I've added message->IsOOB().

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/isolate.h...
runtime/vm/isolate.h:224: class SetIsolateScope {
On 2012/04/14 00:29:53, asiva wrote:
> In two out of the three places where this is used the saved_isolate_ must be
> NULL (I think there should be an assertion for that).

Added a new StartIsolateScope for those cases.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message.cc
File runtime/vm/message.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message.c...
runtime/vm/message.cc:28: if (head_ == NULL) {
On 2012/04/14 00:29:53, asiva wrote:
> ASSERT(tail_ == NULL)?

Done.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message.h
File runtime/vm/message.h (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message.h...
runtime/vm/message.h:71: // message is available.
On 2012/04/14 00:29:53, asiva wrote:
> Perhaps you should add in the comment that this does not block at all to be
more
> explicit.

Done.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
File runtime/vm/message_handler.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler.cc:69: MonitorLocker ml(&monitor_);
On 2012/04/14 00:29:53, asiva wrote:
> What is this lock protecting?
> 
> It seems like it is possible to change values of blocking_, start_function_
etc.
> while a task_ is being run in the pool_

Discuessed offline...

My intention was to always hold the lock when MessageHandler methods are running
-- essentially protect all fields.

I've moved the call to pool_->Run under the lock...

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler.cc:138: pool_->Run(task_);
On 2012/04/14 00:29:53, asiva wrote:
> The task_ is run with the lock being held whereas in the Run method above you
> were releasing the lock, why the variance?

You are right.  I have changed Run() so that it no longer releases the lock.  It
was unnecessary.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler.cc:168: monitor_.Exit();
I will add "lock held" comments to the appropriate functions.

On 2012/04/14 00:29:53, asiva wrote:
> Where is the monitor_.Enter() corresponding to this Exit in the first
iteration
> of the while loop?
> 
> If the monitor is being held on call to this method then we probably need an
> assertion that the lock is indeed held.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler.cc:209: }
On 2012/04/14 00:29:53, asiva wrote:
> HandleNextMessage seems identical to HandleMessages is there a possibility of
> some code sharing?

Refactored to share more code.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
File runtime/vm/message_handler.h (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler.h:77: // These functions should only be called from
the PortMap.
On 2012/04/14 00:29:53, asiva wrote:
> How do we ensure that they are only called from PortMap, maybe make them
> protected and PortMap a friend class here?

Done.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler.h:144: intptr_t live_ports_;
On 2012/04/14 00:29:53, asiva wrote:
> Missing DISALLOW_COPY_AND_ASSIGN(....);

Done.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
File runtime/vm/message_handler_test.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler_test.cc:16: MessageQueue* oob_queue() { return
handler_->oob_queue_; }
On 2012/04/14 00:29:53, asiva wrote:
> const qualifier for these two accessor methods.

Done.

Why should we bother with const qualifications for a class that is used only in
testing?

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler_test.cc:19: MessageHandler* handler_;
On 2012/04/14 00:29:53, asiva wrote:
> Missing DISALLOW_COPY_AND_ASSIGN(...).

Done.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler_test.cc:71: bool result_;
On 2012/04/14 00:29:53, asiva wrote:
> DISALLOW_COPY_AND_ASSIGN...

Done.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler_test.cc:151: handler.PostMessage(oob_message1);
On 2012/04/14 00:29:53, asiva wrote:
> You should probably post this before message2 to mix things up a bit.

Done.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_t...
File runtime/vm/message_test.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_t...
runtime/vm/message_test.cc:20: bool result = (queue_->head_ != NULL ||
queue_->head_ != NULL);
On 2012/04/14 00:29:53, asiva wrote:
> why repeat the condition twice?

Oops.  Fixed.

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/port_test.cc
File runtime/vm/port_test.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/port_test...
runtime/vm/port_test.cc:139: 
On 2012/04/14 00:29:53, asiva wrote:
> Into which file did the ThreadedPort test go.

I deleted it.  But you're right, I'm not covering the same functionality in
tests now.  I've just added a test called MessageHandler_Run in
message_handler_test.cc.

[turnidge, 2012-04-17 23:49:20]

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/dart_api_...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:775: SetIsolateScope set_scope(NULL);
On 2012/04/17 23:46:55, turnidge wrote:
> On 2012/04/14 00:29:53, asiva wrote:
> > Why is NULL passed in and not isolate? Won't it end up setting current
isolate
> > to NULL?
> 
> Yeah.  When I call RunBlocking, it actually runs the isolate on another thread
> and uses a monitor to tell me when it is done.  It seemed correct to give up
the
> current isolate before we started to execute it in a different thread.
> 
> I have added assertions in RunBlocking that there is no current isolate when
> these functions are called.

These comments are out of date, now that I've removed RunBlocking.

[siva, 2012-04-18 22:04:59]

lgtm

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/lib/isolate....
runtime/lib/isolate.cc:523: if (result != 0) {
FATAL is fine for now, we could revisit once the new code has baked for some
time.

On 2012/04/17 23:46:55, turnidge wrote:
> On 2012/04/14 00:29:53, asiva wrote:
> > result has just been initialized to 0, how can it be non zero, is the check
> > needed?
> 
> The check is no longer needed.  It is now a FATAL in thread_pool.cc.
> 
> Do you think that is too severe?  I could restore the old behavior by adding a
> return value to ThreadPool::Run()...

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
File runtime/vm/message_handler_test.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/8001/runtime/vm/message_h...
runtime/vm/message_handler_test.cc:16: MessageQueue* oob_queue() { return
handler_->oob_queue_; }
I have tried to be consistent with the style in test code also.

On 2012/04/17 23:46:55, turnidge wrote:
> On 2012/04/14 00:29:53, asiva wrote:
> > const qualifier for these two accessor methods.
> 
> Done.
> 
> Why should we bother with const qualifications for a class that is used only
in
> testing?

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/lib/isolate...
runtime/lib/isolate.cc:117: intptr_t port_id = data->port_id_;
When is data deleted in the new scheme?

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/dart_api...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:778: RunLoopData* data =
reinterpret_cast<RunLoopData*>(param);
ASSERT(data->monitor != NULL);

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/isolate....
runtime/vm/isolate.h:229: // StartIsolateScope.
It is not necessarily a new isolate right, anytime we execute code in an isolate
(i.e handle a message) we call StartIsolateScope.

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/isolate....
runtime/vm/isolate.h:241: ~StartIsolateScope() {
Should we set the stack limit of the isolate we are about to switch out of to
some value like 0. It will ensure that we don't ever run code without going
through the constructor above.

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/isolate....
runtime/vm/isolate.h:266: Isolate::SetCurrent(saved_isolate_);
Shouldn't we be restoring the stack limit of new_isolate (isolate we are going
to switch out of) to what it was before you set it to 0.

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/message_...
File runtime/vm/message_handler.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/message_...
runtime/vm/message_handler.cc:16: : handler_(handler) {
ASSERT(handler != NULL);

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/message_...
runtime/vm/message_handler.cc:173: ASSERT(pool_ == NULL);
The assert should be after grabbing the lock?

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/message_...
File runtime/vm/message_handler_test.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/message_...
runtime/vm/message_handler_test.cc:256: }
if there is a bug this test will hang forever, do you think it would make sense
to quite after a certain number of retries ?

[turnidge, 2012-04-19 19:46:55]

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/lib/isolate...
runtime/lib/isolate.cc:117: intptr_t port_id = data->port_id_;
On 2012/04/18 22:05:00, asiva wrote:
> When is data deleted in the new scheme?

That was missing.  Added.

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/dart_api...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/dart_api...
runtime/vm/dart_api_impl.cc:778: RunLoopData* data =
reinterpret_cast<RunLoopData*>(param);
On 2012/04/18 22:05:00, asiva wrote:
> ASSERT(data->monitor != NULL);

Done.

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/isolate....
runtime/vm/isolate.h:229: // StartIsolateScope.
On 2012/04/18 22:05:00, asiva wrote:
> It is not necessarily a new isolate right, anytime we execute code in an
isolate
> (i.e handle a message) we call StartIsolateScope.

Done.

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/isolate....
runtime/vm/isolate.h:241: ~StartIsolateScope() {
On 2012/04/18 22:05:00, asiva wrote:
> Should we set the stack limit of the isolate we are about to switch out of to
> some value like 0. It will ensure that we don't ever run code without going
> through the constructor above.

Done.  Had to rework ShutdownIsolate a bit in lib/isolate.cc to avoid deleting
the isolate during a StartIsolateScope.

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/isolate....
runtime/vm/isolate.h:266: Isolate::SetCurrent(saved_isolate_);
On 2012/04/18 22:05:00, asiva wrote:
> Shouldn't we be restoring the stack limit of new_isolate (isolate we are going
> to switch out of) to what it was before you set it to 0.

Done.

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/message_...
File runtime/vm/message_handler.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/message_...
runtime/vm/message_handler.cc:16: : handler_(handler) {
On 2012/04/18 22:05:00, asiva wrote:
> ASSERT(handler != NULL);

Done.

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/message_...
runtime/vm/message_handler.cc:173: ASSERT(pool_ == NULL);
On 2012/04/18 22:05:00, asiva wrote:
> The assert should be after grabbing the lock?

Done.

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/message_...
File runtime/vm/message_handler_test.cc (right):

https://chromiumcodereview.appspot.com/9924015/diff/18007/runtime/vm/message_...
runtime/vm/message_handler_test.cc:256: }
On 2012/04/18 22:05:00, asiva wrote:
> if there is a bug this test will hang forever, do you think it would make
sense
> to quite after a certain number of retries ?

I've added code to limit the test to about 20 seconds.