Add a mid-sized integration test for the Dart Embedding Api which

Add a mid-sized integration test for the Dart Embedding Api which
demonstrates how to create a custom isolate abstraction.  In this
test, we use an event queue to share a single thread among our custom
isolates.

Add a callback which allows embedders to see when a port is created.
Not sure if I should keep this or not.

New apis:
  Dart_CreatePort() -- allocates a port id and adds a port->isolate mapping.
  Dart_IsolateHasActivePorts() -- does the current isolate have open ports?
  (this name a bit awkward...)

Bail out of PortMap::ClosePorts() early if there are no open ports.
This suppresses calls to the close_port_callback when there are no
open ports.

Use DART_CHECK_VALID to provide better error output when the test lib
has errors.

-------------------------
Sample output of the test:

-- (isolate=0x815600) Constructing isolate
-- Enter: CustomIsolateImpl_start --
-- Adding port (7111) -> isolate (0x830800) --
-- Adding StartEvent to queue --
-- Exit: CustomIsolateImpl_start --
-- Adding port (7112) -> isolate (0x815600) --
-- Posting message dest(7111) reply(0) --
-- Adding MessageEvent to queue --
-- Starting event loop --
>> StartEvent with isolate(0x830800)--
-- (isolate=0x830800) Running isolateMain
$$ MessageEvent with dest port 7111--
-- (isolate=0x830800) Received: 42
-- Posting message dest(7112) reply(0) --
-- Adding MessageEvent to queue --
$$ MessageEvent with dest port 7112--
-- Closing port (7112) --
-- Adding ShutdownEvent to queue --
-- (isolate=0x815600) Received: 43
<< ShutdownEvent with isolate(0x815600)--
-- Finished event loop --
Committed: https://code.google.com/p/dart/source/detail?r=1906

[Anton Muhin, 2011-11-18 14:56:41]

That all looks pretty neat.  I am experimenting around pretty similar lines, up
to names of main methods :)

http://codereview.chromium.org/8588040/diff/1/runtime/vm/custom_isolate_test.cc
File runtime/vm/custom_isolate_test.cc (right):

http://codereview.chromium.org/8588040/diff/1/runtime/vm/custom_isolate_test....
runtime/vm/custom_isolate_test.cc:126: Dart_Handle recv_port =
Dart_InvokeStatic(core_lib,
shouldn't we have an embedder API for this?

http://codereview.chromium.org/8588040/diff/1/runtime/vm/custom_isolate_test....
runtime/vm/custom_isolate_test.cc:263: static void ClosePort(Dart_Isolate
isolate,
do we need port create/close callbacks?  what's the planned usage?  maybe
instead we should have all ports are closed callback?

http://codereview.chromium.org/8588040/diff/1/runtime/vm/custom_isolate_test....
runtime/vm/custom_isolate_test.cc:349: Dart_Handle send_port =
Dart_InvokeStatic(lib,
ditto here for embedder's API.

Overall, cannot Dart_CreateIsolate create a send port to it?  and probably
receiver as well with api like Dart_GetIsolateReceivePort?

[turnidge, 2011-11-21 18:19:08]

Another round on this, minus the create port callbacks, plus some convenience
functions.  Let me know what you think.

http://codereview.chromium.org/8588040/diff/1/runtime/vm/custom_isolate_test.cc
File runtime/vm/custom_isolate_test.cc (right):

http://codereview.chromium.org/8588040/diff/1/runtime/vm/custom_isolate_test....
runtime/vm/custom_isolate_test.cc:126: Dart_Handle recv_port =
Dart_InvokeStatic(core_lib,
On 2011/11/18 14:56:42, antonmuhin wrote:
> shouldn't we have an embedder API for this?

I've added Dart_NewSendPort and Dart_NewReceivePort.  They are basically
convenience methods right now -- they are implemented purely using existing dart
apis.  Trying this out to see how it looks...

http://codereview.chromium.org/8588040/diff/1/runtime/vm/custom_isolate_test....
runtime/vm/custom_isolate_test.cc:263: static void ClosePort(Dart_Isolate
isolate,
On 2011/11/18 14:56:42, antonmuhin wrote:
> do we need port create/close callbacks?  what's the planned usage?  maybe
> instead we should have all ports are closed callback?

Got rid of the create port callbacks.  The close port callbacks are needed to
allow the embedder to delete pending events on closed ports.  I have added some
of this kind of code to the test.

http://codereview.chromium.org/8588040/diff/1/runtime/vm/custom_isolate_test....
runtime/vm/custom_isolate_test.cc:349: Dart_Handle send_port =
Dart_InvokeStatic(lib,
On 2011/11/18 14:56:42, antonmuhin wrote:
> ditto here for embedder's API.
> 
> Overall, cannot Dart_CreateIsolate create a send port to it?  and probably
> receiver as well with api like Dart_GetIsolateReceivePort?

Intriguing idea.  I will discuss with vm team when I get in.

If I automatically create a receive port for each isolate, then the run loop for
that isolate won't exit until that receive port is closed.  This could be
problematic for the main isolate.  I have some ideas about this that I will kick
around a bit.

[Anton Muhin, 2011-11-22 12:06:47]

Pretty neat and LGTM.

But indeed, not sure if it should be submitted.

[turnidge, 2011-11-23 01:52:45]

Sending out the latest incarnation of this change.

Adds around 5 new files to the change.

I've added a notion of a "main port" for each isolate.  See more detailed
comments below.

This cl isn't ready to submit as-is, but is meant to provoke the next round of
comments.

http://codereview.chromium.org/8588040/diff/1/runtime/vm/custom_isolate_test.cc
File runtime/vm/custom_isolate_test.cc (right):

http://codereview.chromium.org/8588040/diff/1/runtime/vm/custom_isolate_test....
runtime/vm/custom_isolate_test.cc:349: Dart_Handle send_port =
Dart_InvokeStatic(lib,
On 2011/11/21 18:19:08, turnidge wrote:
> On 2011/11/18 14:56:42, antonmuhin wrote:
> > ditto here for embedder's API.
> > 
> > Overall, cannot Dart_CreateIsolate create a send port to it?  and probably
> > receiver as well with api like Dart_GetIsolateReceivePort?
> 
> Intriguing idea.  I will discuss with vm team when I get in.
> 
> If I automatically create a receive port for each isolate, then the run loop
for
> that isolate won't exit until that receive port is closed.  This could be
> problematic for the main isolate.  I have some ideas about this that I will
kick
> around a bit.

Ok, I've played around a bit with this idea.

I can make it allocate a Dart_Port during isolate creation.  The challenge here
is that the run loop, as it stands, checks the number of active ports as its
exit condition and so the main isolate won't exit.  To make this work, I had to
add a notion of "live" ports -- these are ReceivePorts that have actually called
"receive".  Those that do not call "receive" are functionally dead.  I
implemented this by adding a counter in ReceivePortImpl at the dart level.  This
seems to work.

Now, can we create the SendPort/ReceivePort instances automatically during
isolate creation?

The SendPort is problematic.  It needs the port number from the new isolate, but
it needs to be allocated in the parent isolate.  If I make an api like
Dart_GetSendPortFor(Dart_Isolate child_isolate), then I run into synchronization
issues -- what if the child_isolate has been deallocated before I can allocate
my send port?  Then I would access a broken Dart_Isolate.  It seems to me that
the safest way to carry the port information across the boundary from the child
isolate back to the parent isolate is with the Dart_Port integer.  This leads me
to think that the current Dart_NewSendPort api is the best I can do.

I could probably implement a Dart_GetMainReceivePort() convenience function, but
it is oddly unparallel with what I have to do for the SendPort.  So I like it
better to keep Dart_NewReceivePort in this case.

[turnidge, 2011-11-23 01:58:21]

(Adding siggi to the cc list.  No comments required, you just might find the
example interesting.)

[Anton Muhin, 2011-11-23 19:28:14]

Still pretty neat

http://codereview.chromium.org/8588040/diff/14001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8588040/diff/14001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:448: * Gets the main Dart_Port for the current
isolate.
nit: 'main' sounds too generic to me at least.

http://codereview.chromium.org/8588040/diff/14001/runtime/lib/isolate.dart
File runtime/lib/isolate.dart (right):

http://codereview.chromium.org/8588040/diff/14001/runtime/lib/isolate.dart#ne...
runtime/lib/isolate.dart:48: static int _getNumLivePorts() {
nit: I'd write

static int _getNumLivePorts() => _numLivePorts === null ? 0 : _numLivePorts;

http://codereview.chromium.org/8588040/diff/14001/runtime/vm/custom_isolate_t...
File runtime/vm/custom_isolate_test.cc (right):

http://codereview.chromium.org/8588040/diff/14001/runtime/vm/custom_isolate_t...
runtime/vm/custom_isolate_test.cc:124: Dart_Port port = Dart_GetMainPort();
do you need this port here?

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

http://codereview.chromium.org/8588040/diff/14001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:913: OS::Print("%s\n", obj.ToCString());
nit: debugging stuff

[turnidge, 2011-11-23 21:45:37]

Ok, ready for a VM team review.  Changing reviewer to Siva.

http://codereview.chromium.org/8588040/diff/14001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8588040/diff/14001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:448: * Gets the main Dart_Port for the current
isolate.
On 2011/11/23 19:28:14, antonmuhin wrote:
> nit: 'main' sounds too generic to me at least.

What do you think it should be called?  Open to suggestions.  Whatever the name
is, it is good if it is simple.

http://codereview.chromium.org/8588040/diff/14001/runtime/lib/isolate.dart
File runtime/lib/isolate.dart (right):

http://codereview.chromium.org/8588040/diff/14001/runtime/lib/isolate.dart#ne...
runtime/lib/isolate.dart:48: static int _getNumLivePorts() {
On 2011/11/23 19:28:14, antonmuhin wrote:
> nit: I'd write
> 
> static int _getNumLivePorts() => _numLivePorts === null ? 0 : _numLivePorts;

Done.

http://codereview.chromium.org/8588040/diff/14001/runtime/vm/custom_isolate_t...
File runtime/vm/custom_isolate_test.cc (right):

http://codereview.chromium.org/8588040/diff/14001/runtime/vm/custom_isolate_t...
runtime/vm/custom_isolate_test.cc:124: Dart_Port port = Dart_GetMainPort();
On 2011/11/23 19:28:14, antonmuhin wrote:
> do you need this port here?

The variable is used, but only once.  I have combined this line and the next.

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

http://codereview.chromium.org/8588040/diff/14001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:913: OS::Print("%s\n", obj.ToCString());
On 2011/11/23 19:28:14, antonmuhin wrote:
> nit: debugging stuff

Oops, thanks.

[siva, 2011-11-24 00:52:31]

http://codereview.chromium.org/8588040/diff/23001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8588040/diff/23001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:492: DART_EXPORT Dart_Handle
Dart_NewSendPort(Dart_Port port);
If I call this function multiple times with the same port id I will be getting
multiple SendPort objects back. I am not sure if that is correct.

Maybe this should be called Dart_GetSendPort(Dart_Port port);

http://codereview.chromium.org/8588040/diff/23001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:497: DART_EXPORT Dart_Handle
Dart_NewReceivePort(Dart_Port port);
Ditto comment on multiple receive ports.

http://codereview.chromium.org/8588040/diff/23001/runtime/lib/isolate.dart
File runtime/lib/isolate.dart (right):

http://codereview.chromium.org/8588040/diff/23001/runtime/lib/isolate.dart#ne...
runtime/lib/isolate.dart:21: _numLivePorts++;
If I call receive multiple times on the same receive port then will I end up
with my numLivePorts being off.
e.g:
port.receive(handler1);
port.receive(handler2);
port.receive(null);

Maybe you should increment only if _onMessage is null and decrement if
_onMessage is set to null

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

http://codereview.chromium.org/8588040/diff/23001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:654: DARTSCOPE(isolate);
The isolate variable is not used in this method is there a need to have a
isolate variable.

http://codereview.chromium.org/8588040/diff/23001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:722: Dart_EnterScope();
Why is a Dart_EnterScope needed here? Normally we have assumed that people
calling into the dart api functions would have a scope setup.

http://codereview.chromium.org/8588040/diff/23001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:798: Resolver::kIsQualified));
The function resolution code seems to be identical and repeated many times, I am
wondering if it would be possible to have a static function that takes a class
name and a function name and returns a RawFunction.

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

http://codereview.chromium.org/8588040/diff/23001/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:40: main_port_(0),
maybe -1 would be better value or we should have an assert in
PortMap::InitOnce() that PortMap::next_port_ is not initialized to 0.

http://codereview.chromium.org/8588040/diff/23001/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:246: while (Dart_HasLivePorts()) {
Seems pretty expensive to call a dart function from here to determine if there
are live ports. Since we are in the VM is there a way to get this data directly.

[turnidge, 2011-11-29 01:01:31]

PTAL.

Based on our discussion, this changed direction a bit.

A port is now considered "live" once it has had a ReceivePort created for it. 
Most ports are, therefore, live, except for the main port of the main isolate
(and I guess any ReceivePort which receives an exception in its constructor). 
We no longer pay special attention to "null" as a ReceivePort handler -- it is
just a very poor handler, nothing more.

I now keep track of live ports in the PortMap.

http://codereview.chromium.org/8588040/diff/23001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8588040/diff/23001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:492: DART_EXPORT Dart_Handle
Dart_NewSendPort(Dart_Port port);
Discussed offline, multiple SendPorts will work.

On 2011/11/24 00:52:31, asiva wrote:
> If I call this function multiple times with the same port id I will be getting
> multiple SendPort objects back. I am not sure if that is correct.
> 
> Maybe this should be called Dart_GetSendPort(Dart_Port port);

http://codereview.chromium.org/8588040/diff/23001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:497: DART_EXPORT Dart_Handle
Dart_NewReceivePort(Dart_Port port);
Multiple ReceivePorts don't work.  Added a comment for now.

On 2011/11/24 00:52:31, asiva wrote:
> Ditto comment on multiple receive ports.

http://codereview.chromium.org/8588040/diff/23001/runtime/lib/isolate.dart
File runtime/lib/isolate.dart (right):

http://codereview.chromium.org/8588040/diff/23001/runtime/lib/isolate.dart#ne...
runtime/lib/isolate.dart:21: _numLivePorts++;
On 2011/11/24 00:52:31, asiva wrote:
> If I call receive multiple times on the same receive port then will I end up
> with my numLivePorts being off.
> e.g:
> port.receive(handler1);
> port.receive(handler2);
> port.receive(null);
> 
> Maybe you should increment only if _onMessage is null and decrement if
> _onMessage is set to null

I redid the change so that isolate.dart doesn't need to change.  Instead I keep
track of the live ports on the C++ side.  I have also changed the definition of
"live" from "has a non-null handler" to "has a ReceivePort created for it".  I
did this because of an offline discussion about what the meaning of "null" for a
handler should be.

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

http://codereview.chromium.org/8588040/diff/23001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:654: DARTSCOPE(isolate);
On 2011/11/24 00:52:31, asiva wrote:
> The isolate variable is not used in this method is there a need to have a
> isolate variable.

Fixed.

http://codereview.chromium.org/8588040/diff/23001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:722: Dart_EnterScope();
On 2011/11/24 00:52:31, asiva wrote:
> Why is a Dart_EnterScope needed here? Normally we have assumed that people
> calling into the dart api functions would have a scope setup.

Completely redid how this works.

http://codereview.chromium.org/8588040/diff/23001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:798: Resolver::kIsQualified));
Added a TODO.  Is that okay?

On 2011/11/24 00:52:31, asiva wrote:
> The function resolution code seems to be identical and repeated many times, I
am
> wondering if it would be possible to have a static function that takes a class
> name and a function name and returns a RawFunction.

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

http://codereview.chromium.org/8588040/diff/23001/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:40: main_port_(0),
On 2011/11/24 00:52:31, asiva wrote:
> maybe -1 would be better value or we should have an assert in
> PortMap::InitOnce() that PortMap::next_port_ is not initialized to 0.

There is already an assert in PortMap::AllocatePort.  We also use 0 as a special
value in dart_api.h for kNoReplyPort and kCloseAllPorts.

http://codereview.chromium.org/8588040/diff/23001/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:246: while (Dart_HasLivePorts()) {
On 2011/11/24 00:52:31, asiva wrote:
> Seems pretty expensive to call a dart function from here to determine if there
> are live ports. Since we are in the VM is there a way to get this data
directly.

Now this uses live_ports() > 0.

[siva, 2011-11-29 19:50:27]

LGTM

We need to resolve the semantics of Dart_NewSendPort and Dart_NewReceivePort

http://codereview.chromium.org/8588040/diff/23003/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8588040/diff/23003/runtime/include/dart_api.h#...
runtime/include/dart_api.h:457: */
Is this comment still valid?

http://codereview.chromium.org/8588040/diff/23003/runtime/include/dart_api.h#...
runtime/include/dart_api.h:492: DART_EXPORT Dart_Handle
Dart_NewSendPort(Dart_Port port);
I would call the variable port_id to avoid having it seem as a port object.

http://codereview.chromium.org/8588040/diff/23003/runtime/include/dart_api.h#...
runtime/include/dart_api.h:497: * Do not create multiple ReceivePorts with the
same id.
Is this a comment for the user stating that they should not call this multiple
times for the same id?

http://codereview.chromium.org/8588040/diff/23003/runtime/include/dart_api.h#...
runtime/include/dart_api.h:499: DART_EXPORT Dart_Handle
Dart_NewReceivePort(Dart_Port port);
port => port_id

http://codereview.chromium.org/8588040/diff/23003/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

http://codereview.chromium.org/8588040/diff/23003/runtime/lib/isolate.cc#newc...
runtime/lib/isolate.cc:109: PortMap::SetLive(port_id);
I think we should add a comment that a port once it becomes live will become non
live only if it is closed. That is not obvious when reading the code. The dart
reference is held in the port_map object ensuring that the reference is not
lost.

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

http://codereview.chromium.org/8588040/diff/23003/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:740: DART_EXPORT Dart_Handle
Dart_NewSendPort(Dart_Port port) {
I am still not clear why this should not be
Dart_GetSendPort(Dart_Port port);
which will return the port object if one already exists or create a new one if
there in no corresponding port object.

or another alternative is to report an error if a port object already exists for
the id.

http://codereview.chromium.org/8588040/diff/23003/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:764: DART_EXPORT Dart_Handle
Dart_NewReceivePort(Dart_Port port) {
Ditto comment about why not Dart_GetReceivePort(port);

One potential issue with having this is if I have a dart port created, called
receive on it to setup a handler and then I somehow endup calling
Dart_NewReceivePort with the same port id then it will create a new port object
for it and the handler I setup is lost.

[turnidge, 2011-12-05 17:53:33]

(change already submitted, but forgot to send out these comments).

http://codereview.chromium.org/8588040/diff/23003/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8588040/diff/23003/runtime/include/dart_api.h#...
runtime/include/dart_api.h:457: */
On 2011/11/29 19:50:27, asiva wrote:
> Is this comment still valid?

Updated.

http://codereview.chromium.org/8588040/diff/23003/runtime/include/dart_api.h#...
runtime/include/dart_api.h:492: DART_EXPORT Dart_Handle
Dart_NewSendPort(Dart_Port port);
On 2011/11/29 19:50:27, asiva wrote:
> I would call the variable port_id to avoid having it seem as a port object.

Done.  I also updated all other Dart_Port parameters in the api at the same
time.

http://codereview.chromium.org/8588040/diff/23003/runtime/include/dart_api.h#...
runtime/include/dart_api.h:497: * Do not create multiple ReceivePorts with the
same id.
On 2011/11/29 19:50:27, asiva wrote:
> Is this a comment for the user stating that they should not call this multiple
> times for the same id?

Changed this function to Dart_GetReceivePort to imply that there is only one and
updated the code to return the same ReceivePort.

http://codereview.chromium.org/8588040/diff/23003/runtime/include/dart_api.h#...
runtime/include/dart_api.h:499: DART_EXPORT Dart_Handle
Dart_NewReceivePort(Dart_Port port);
On 2011/11/29 19:50:27, asiva wrote:
> port => port_id

Done.

http://codereview.chromium.org/8588040/diff/23003/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

http://codereview.chromium.org/8588040/diff/23003/runtime/lib/isolate.cc#newc...
runtime/lib/isolate.cc:109: PortMap::SetLive(port_id);
Added comment in port.h.

On 2011/11/29 19:50:27, asiva wrote:
> I think we should add a comment that a port once it becomes live will become
non
> live only if it is closed. That is not obvious when reading the code. The dart
> reference is held in the port_map object ensuring that the reference is not
> lost.