OOB messages and general message refactor.

OOB messages and general message refactor.

(1)
No longer give embedders direct access to inter-isolate messages.
Instead, keep these messages in an internal queue and rely on the
embedder only to provide notifications.  This simplifies the api in
numerous ways:

 - post message callback drops parameters and becomes message notify callback.
 - close port callback goes away
 - Dart_Message type goes away
 - Dart_HandleMessage gets a simpler signature

Updated the custom_isolate_test to show what use of the new apis looks like.

(2)
Begin to add support for out-of-band (OOB) messages.  These messages
supercede regular messages in the queue.  We will attempt to deliver
these messages even while code is running, by using the isolate
interrupt mechanism.  These messages are not user-visible.  They will
be used by the runtime internally to implement things like reflection.

Renamed PortMessage to Message.

Refactored message sending apis in PortMap and Isolate.

Modified MessageQueue to be aware of multiple priorities.

Modify Dart_HandleMessage to process multiple OOB messages

[Anton Muhin, 2012-01-12 12:58:16]

Todd, I won't be able to prepare today a CL to migrate to new API, please, give
me Fri to do that.

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/custom_isolate_te...
File runtime/vm/custom_isolate_test.cc (right):

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/custom_isolate_te...
runtime/vm/custom_isolate_test.cc:88: virtual bool IsMessageEvent(Dart_Isolate
isolate) {
maybe now move this logic into StartEvent and add a check that it's never
invoked for StartEvent::isolate_ isolate?  Overall, cannot you provide an access
to isolate_ field and filter just by it?

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/custom_isolate_te...
runtime/vm/custom_isolate_test.cc:122: tail_ = NULL;
technically, you don't need this assignment, correct? (I see it's moved)

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/custom_isolate_te...
runtime/vm/custom_isolate_test.cc:128: void Remove(Dart_Isolate isolate) {
nit: longer name like RemoveEventsForIsolate may make life of reader somewhat
easier.

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/custom_isolate_te...
runtime/vm/custom_isolate_test.cc:154: Event* current_event;
it looks like you don't need current_event any more, correct?

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:734: } while (message->priority() <=
Message::kOOBPriority);
you have deleted message couple of lines above, no?

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:734: } while (message->priority() <=
Message::kOOBPriority);
up to you, but it's hard to remember if normal priority > or < than OOBPriority.
 If you're not planning to add more priorities, maybe introduce something like
is_oob_message() and is_normal_message()?  That would apparently make the code
more readable.

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/isolate.h#newcode235
runtime/vm/isolate.h:235: kInterruptsMask = 0x3,
maybe kInterruptsMask = kApiInterrupt | kMessageInterrupt ?

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/message_queue.cc
File runtime/vm/message_queue.cc (right):

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/message_queue.cc#...
runtime/vm/message_queue.cc:54: Message* MessageQueue::DequeueNoWait() {
you don't lock on monitor_, is it ok?

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/message_queue.cc#...
runtime/vm/message_queue.cc:60: // The following update to tail_ is not strictly
needed.
maybe move under DEBUG?

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/message_queue_tes...
File runtime/vm/message_queue_test.cc (right):

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/message_queue_tes...
runtime/vm/message_queue_test.cc:17: bool HasMessage() const {
that's a pure paranoia, but technically you're not guaranteed to see no NULL as
you don't have any write barriers, correct?  Or this is only single-threaded
case?  If yes, maybe worth documenting.

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/message_queue_tes...
runtime/vm/message_queue_test.cc:166: Message* msg = new Message(i+10, i+100,
reinterpret_cast<uint8_t*>(data),
nit: spaces around +'s

[turnidge, 2012-01-12 19:01:55]

Hi Anton,

Thanks for the quick review.

Take the time you need to code up your changes.  Maybe we can shoot for Monday
or so, depending on the outcome of my vm-team reviews?

Todd

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/custom_isolate_te...
File runtime/vm/custom_isolate_test.cc (right):

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/custom_isolate_te...
runtime/vm/custom_isolate_test.cc:88: virtual bool IsMessageEvent(Dart_Isolate
isolate) {
On 2012/01/12 12:58:16, antonmuhin wrote:
> maybe now move this logic into StartEvent and add a check that it's never
> invoked for StartEvent::isolate_ isolate?  Overall, cannot you provide an
access
> to isolate_ field and filter just by it?

I have added an isolate_ field to the base Event class and now use that.  Is
this what you meant?

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/custom_isolate_te...
runtime/vm/custom_isolate_test.cc:122: tail_ = NULL;
On 2012/01/12 12:58:16, antonmuhin wrote:
> technically, you don't need this assignment, correct? (I see it's moved)

Added a comment.

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/custom_isolate_te...
runtime/vm/custom_isolate_test.cc:128: void Remove(Dart_Isolate isolate) {
On 2012/01/12 12:58:16, antonmuhin wrote:
> nit: longer name like RemoveEventsForIsolate may make life of reader somewhat
> easier.

Done.

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/custom_isolate_te...
runtime/vm/custom_isolate_test.cc:154: Event* current_event;
On 2012/01/12 12:58:16, antonmuhin wrote:
> it looks like you don't need current_event any more, correct?

Thanks, removed.

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:734: } while (message->priority() <=
Message::kOOBPriority);
On 2012/01/12 12:58:16, antonmuhin wrote:
> you have deleted message couple of lines above, no?

Yikes.  Fixed.

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:734: } while (message->priority() <=
Message::kOOBPriority);
On 2012/01/12 12:58:16, antonmuhin wrote:
> up to you, but it's hard to remember if normal priority > or < than
OOBPriority.
>  If you're not planning to add more priorities, maybe introduce something like
> is_oob_message() and is_normal_message()?  That would apparently make the code
> more readable.

Changed to an == for now.

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/isolate.h#newcode235
runtime/vm/isolate.h:235: kInterruptsMask = 0x3,
On 2012/01/12 12:58:16, antonmuhin wrote:
> maybe kInterruptsMask = kApiInterrupt | kMessageInterrupt ?

Done.

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/message_queue.cc
File runtime/vm/message_queue.cc (right):

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/message_queue.cc#...
runtime/vm/message_queue.cc:54: Message* MessageQueue::DequeueNoWait() {
On 2012/01/12 12:58:16, antonmuhin wrote:
> you don't lock on monitor_, is it ok?

It is not okay.  Fixed.

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/message_queue.cc#...
runtime/vm/message_queue.cc:60: // The following update to tail_ is not strictly
needed.
On 2012/01/12 12:58:16, antonmuhin wrote:
> maybe move under DEBUG?

It's not expensive and it makes me feel happier.

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/message_queue_tes...
File runtime/vm/message_queue_test.cc (right):

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/message_queue_tes...
runtime/vm/message_queue_test.cc:17: bool HasMessage() const {
On 2012/01/12 12:58:16, antonmuhin wrote:
> that's a pure paranoia, but technically you're not guaranteed to see no NULL
as
> you don't have any write barriers, correct?  Or this is only single-threaded
> case?  If yes, maybe worth documenting.

Added lock.

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/message_queue_tes...
runtime/vm/message_queue_test.cc:166: Message* msg = new Message(i+10, i+100,
reinterpret_cast<uint8_t*>(data),
On 2012/01/12 12:58:16, antonmuhin wrote:
> nit: spaces around +'s

Done.

[Anton Muhin, 2012-01-13 14:26:53]

LGTM.

Please, let me know when this CL is ready to submit, and I'll adjust the
bindings.

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/custom_isolate_te...
File runtime/vm/custom_isolate_test.cc (right):

http://codereview.chromium.org/9182001/diff/9001/runtime/vm/custom_isolate_te...
runtime/vm/custom_isolate_test.cc:88: virtual bool IsMessageEvent(Dart_Isolate
isolate) {
Yes, thank you.

On 2012/01/12 19:01:55, turnidge wrote:
> On 2012/01/12 12:58:16, antonmuhin wrote:
> > maybe now move this logic into StartEvent and add a check that it's never
> > invoked for StartEvent::isolate_ isolate?  Overall, cannot you provide an
> access
> > to isolate_ field and filter just by it?
> 
> I have added an isolate_ field to the base Event class and now use that.  Is
> this what you meant?

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/message_queue_te...
File runtime/vm/message_queue_test.cc (right):

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/message_queue_te...
runtime/vm/message_queue_test.cc:20: queue_->monitor_.Enter();
nit: can you use locker here?

[Ivan Posva, 2012-01-23 22:00:43]

http://codereview.chromium.org/9182001/diff/13004/runtime/include/dart_api.h
File runtime/include/dart_api.h (left):

http://codereview.chromium.org/9182001/diff/13004/runtime/include/dart_api.h#...
runtime/include/dart_api.h:556: * \return True if the message was posted.
Why are you removing the documentation for these two functions?

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:888: // TODO(turnidge): Implement this when needed.
Should this be an UNIMPLEMENTED() then?

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:724: UNIMPLEMENTED();
Can you briefly describe in a comment what should happen in this case?

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:736: } while (priority == Message::kOOBPriority);
How about:
while (priority >= Message::kOOBPriority)

This would break if we ever start using a priority higher than out-of-band.

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:87: if (message->priority() <= Message::kOOBPriority) {
Shouldn't this be ">="? Only priorities of OOB and higher should interrupt the
isolate.

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:314: UNIMPLEMENTED();
ditto. Short comment needed.

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/message_queue.cc
File runtime/vm/message_queue.cc (right):

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/message_queue.cc...
runtime/vm/message_queue.cc:30: // isolate.  Should MonitorLocker really be a
StackResource?
MonitorLocker should still stay a stack resource, but in this case we might want
to use some other scoped lock mechanism which could be rolled into platform
headers, e.g. ScopedMonitor. Please coordinate with Søren.

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/message_queue.cc...
runtime/vm/message_queue.cc:58: Message* MessageQueue::DequeueNoWaitInternal() {
How about DequeueNoLock to make it clear that this part does not grab the lock?
Internal does not convey the no locking nature of this method.

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/port.cc
File runtime/vm/port.cc (right):

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/port.cc#newcode194
runtime/vm/port.cc:194: mutex_->Lock();
MutexLocker ml(mutex_);

[turnidge, 2012-01-23 22:32:52]

http://codereview.chromium.org/9182001/diff/13004/runtime/include/dart_api.h
File runtime/include/dart_api.h (left):

http://codereview.chromium.org/9182001/diff/13004/runtime/include/dart_api.h#...
runtime/include/dart_api.h:556: * \return True if the message was posted.
On 2012/01/23 22:00:43, Ivan Posva wrote:
> Why are you removing the documentation for these two functions?

In an earlier rev, I made these void functions.  Then I undid it and forgot to
fix the docs.  Thanks.  Fixed.

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/code_generator.c...
runtime/vm/code_generator.cc:888: // TODO(turnidge): Implement this when needed.
On 2012/01/23 22:00:43, Ivan Posva wrote:
> Should this be an UNIMPLEMENTED() then?

Yes.  Changed.

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:724: UNIMPLEMENTED();
On 2012/01/23 22:00:43, Ivan Posva wrote:
> Can you briefly describe in a comment what should happen in this case?

Done.

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:736: } while (priority == Message::kOOBPriority);
Anton didn't like the >= because it is unclear which way priorities are ordered,
so I decided to switch this.  But I am now feeling like switching it back.

Done.

On 2012/01/23 22:00:43, Ivan Posva wrote:
> How about:
> while (priority >= Message::kOOBPriority)
> 
> This would break if we ever start using a priority higher than out-of-band.

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:87: if (message->priority() <= Message::kOOBPriority) {
On 2012/01/23 22:00:43, Ivan Posva wrote:
> Shouldn't this be ">="? Only priorities of OOB and higher should interrupt the
> isolate.

Yes.  Fixed.  (I reversed the order during implementation...)

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:314: UNIMPLEMENTED();
On 2012/01/23 22:00:43, Ivan Posva wrote:
> ditto. Short comment needed.

Done.

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/message_queue.cc
File runtime/vm/message_queue.cc (right):

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/message_queue.cc...
runtime/vm/message_queue.cc:30: // isolate.  Should MonitorLocker really be a
StackResource?
Updated.

On 2012/01/23 22:00:43, Ivan Posva wrote:
> MonitorLocker should still stay a stack resource, but in this case we might
want
> to use some other scoped lock mechanism which could be rolled into platform
> headers, e.g. ScopedMonitor. Please coordinate with Søren.

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/message_queue.cc...
runtime/vm/message_queue.cc:58: Message* MessageQueue::DequeueNoWaitInternal() {
On 2012/01/23 22:00:43, Ivan Posva wrote:
> How about DequeueNoLock to make it clear that this part does not grab the
lock?
> Internal does not convey the no locking nature of this method.

How about DequeueNoWaitHoldsLock?

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/port.cc
File runtime/vm/port.cc (right):

http://codereview.chromium.org/9182001/diff/13004/runtime/vm/port.cc#newcode194
runtime/vm/port.cc:194: mutex_->Lock();
On 2012/01/23 22:00:43, Ivan Posva wrote:
> MutexLocker ml(mutex_);

We can post from a non-isolate via Dart_PostIntArray and MutexLocker is a stack
resource.  Added a TODO.

[Ivan Posva, 2012-01-23 22:35:55]

LGTM -ip