- Add a minimal implementation of Capability.

- Add a minimal implementation of Capability.
- Make RawReceivePort and SendPort VM internal objects.
- Rationalize the creation of ports and their handling within the VM.

R=asiva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=35325

[Ivan Posva, 2014-04-22 17:43:00]

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:15: /* patch */ factory Capability() {
This will be hooked up in a follow up change.

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:100: bool operator==(var other) {
operator == and hashCode will move entirely into native in a next cycle.

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:139: // TODO(iposva): Ideally keep this map in
the VM.
This map and related functionality will move to native code in a followup CL.

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:160: _get_id() native "SendPortImpl_get_id";
These will be removed in a future CL. But are still needed since some
implementation functions are in Dart code.

[turnidge, 2014-04-22 18:40:45]

DBC -turnidge

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate.cc#n...
runtime/lib/isolate.cc:33: return Capability::New(id);
Do we worry about collisions?

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/message_handl...
File runtime/vm/message_handler.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/message_handl...
runtime/vm/message_handler.cc:159: OS::Print("[>] Message handled:\n"
Change [>] to [<].

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:17916: return "Capability";
Consider adding Id() to the ToCString, here and below.  You can use
zone->PrintToString() to avoid computing buffer size.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object_store.cc
File runtime/vm/object_store.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object_store....
runtime/vm/object_store.cc:52: capability_class_(Class::null()),
Should you add the receive port and send port classes here as well?

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/port_test.cc
File runtime/vm/port_test.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/port_test.cc#...
runtime/vm/port_test.cc:137: TEST_CASE(PortMap_PostMessageInvalidPort) {
Change test name to PostMessageClosedPort?

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:607: js.Setup(zone.GetZone(), SendPort::Cast(
strange line break.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1714: js.Setup(zone.GetZone(), SendPort::Cast(
ditto

[Cutch, 2014-04-22 20:15:22]

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

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/isolate.cc#ne...
runtime/vm/isolate.cc:125: // the receive port is closed, we drop the message
without deserializing it.
Comment needs updating.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:17944: return "ReceivePort";
Include the port id here and in SendPort::ToCString()

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.h#newc...
runtime/vm/object.h:6487: class ReceivePort: public Instance {
missing space before :

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1698: FATAL("SendPort expected.");
Why not ASSERT(reply_port.IsSendPort()) here and above?

[Ivan Posva, 2014-04-22 21:28:29]

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate.cc#n...
runtime/lib/isolate.cc:33: return Capability::New(id);
On 2014/04/22 18:40:45, turnidge wrote:
> Do we worry about collisions?

No.

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

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/isolate.cc#ne...
runtime/vm/isolate.cc:125: // the receive port is closed, we drop the message
without deserializing it.
On 2014/04/22 20:15:22, Cutch wrote:
> Comment needs updating.

Updated.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/message_handl...
File runtime/vm/message_handler.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/message_handl...
runtime/vm/message_handler.cc:159: OS::Print("[>] Message handled:\n"
On 2014/04/22 18:40:45, turnidge wrote:
> Change [>] to [<].

[.]

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:17916: return "Capability";
On 2014/04/22 18:40:45, turnidge wrote:
> Consider adding Id() to the ToCString, here and below.  You can use
> zone->PrintToString() to avoid computing buffer size.

Not printing it on purpose.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:17944: return "ReceivePort";
On 2014/04/22 20:15:22, Cutch wrote:
> Include the port id here and in SendPort::ToCString()

Not printing it on purpose.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.h#newc...
runtime/vm/object.h:6487: class ReceivePort: public Instance {
On 2014/04/22 20:15:22, Cutch wrote:
> missing space before :

Done.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object_store.cc
File runtime/vm/object_store.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object_store....
runtime/vm/object_store.cc:52: capability_class_(Class::null()),
On 2014/04/22 18:40:45, turnidge wrote:
> Should you add the receive port and send port classes here as well?

There is no need to follow that outdated pattern of adding the classes in here.
I removed them from the ObjectStore entirely. Thanks for pointing this out!

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/port_test.cc
File runtime/vm/port_test.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/port_test.cc#...
runtime/vm/port_test.cc:137: TEST_CASE(PortMap_PostMessageInvalidPort) {
On 2014/04/22 18:40:45, turnidge wrote:
> Change test name to PostMessageClosedPort?

A closed port is an invalid port, but I agree that your name describes better
what is going on.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:607: js.Setup(zone.GetZone(), SendPort::Cast(
On 2014/04/22 18:40:45, turnidge wrote:
> strange line break.

Changed, but there is no good way to make this fit well.

[siva, 2014-04-23 00:20:30]

LGTM with some comments.

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:93: _handlerMap.remove(this._closeInternal());
I find _closeInternal returning the id of the port it just closed a bit weird,
why not leave it the way it is now as. I understand it results in two native
calls but with the proposed plan to move the handlerMap also into native code it
would become just one native call.

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:111: _get_sendport() native
"RawReceivePortImpl_get_sendport";
Missing return types for these.

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:161: _get_hashcode() native
"SendPortImpl_get_hashcode";
ditto comment about missing return types.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl.cc:1246: }
Missing check:

if (port_id == NULL) {
  RETURN_NULL_ERROR(port_id);
}

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl.cc:1248: ASSERT(*port_id != 0);
Maybe this assert needs to be inside 'Id()' ?

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:17937: result.raw_ptr()->send_port_ = send_port.raw();
I think this line should also move inside the NoGCScope as it is dealing with
raw pointers.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:17962: result.raw_ptr()->id_ = id;
Ditto comment about this line moving into the scope above.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.h#newc...
runtime/vm/object.h:6489: Dart_Port Id() const { return
raw_ptr()->send_port_->ptr()->id_; }
maybe

send_port()->ptr()->id_;

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.h#newc...
runtime/vm/object.h:6494: void set_handler(const Instance& value);
where is set_handler implemented ?
maybe it should also be a const.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/raw_object_sn...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/raw_object_sn...
runtime/vm/raw_object_snapshot.cc:2535: UNIMPLEMENTED();
Should this be UNREACHABLE() we don't expect to send receive ports out in
messages correct?

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/raw_object_sn...
runtime/vm/raw_object_snapshot.cc:2543: UNIMPLEMENTED();
Ditto comment.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/snapshot.h
File runtime/vm/snapshot.h (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/snapshot.h#ne...
runtime/vm/snapshot.h:62: class RawSendPort;
Why are these forward declarations needed here?

[Ivan Posva, 2014-04-23 19:43:35]

Thanks!

-Ivan

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate_patc...
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:93: _handlerMap.remove(this._closeInternal());
On 2014/04/23 00:20:31, siva wrote:
> I find _closeInternal returning the id of the port it just closed a bit weird,
> why not leave it the way it is now as. I understand it results in two native
> calls but with the proposed plan to move the handlerMap also into native code
it
> would become just one native call.

I will update it once the _handlerMap is removed.

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:111: _get_sendport() native
"RawReceivePortImpl_get_sendport";
On 2014/04/23 00:20:31, siva wrote:
> Missing return types for these.

_get_sendport() uses the return type of the sendPort public getter. _get_id()
will be removed.

https://codereview.chromium.org/243973002/diff/20001/runtime/lib/isolate_patc...
runtime/lib/isolate_patch.dart:161: _get_hashcode() native
"SendPortImpl_get_hashcode";
On 2014/04/23 00:20:31, siva wrote:
> ditto comment about missing return types.

ditto about piggybacking on the return type of hashCode.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl.cc:1246: }
On 2014/04/23 00:20:31, siva wrote:
> Missing check:
> 
> if (port_id == NULL) {
>   RETURN_NULL_ERROR(port_id);
> }

Done.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl.cc:1248: ASSERT(*port_id != 0);
On 2014/04/23 00:20:31, siva wrote:
> Maybe this assert needs to be inside 'Id()' ?

ASSERT was unnecessary.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:17937: result.raw_ptr()->send_port_ = send_port.raw();
On 2014/04/23 00:20:31, siva wrote:
> I think this line should also move inside the NoGCScope as it is dealing with
> raw pointers.

Done.

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/raw_object_sn...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/243973002/diff/20001/runtime/vm/raw_object_sn...
runtime/vm/raw_object_snapshot.cc:2535: UNIMPLEMENTED();
On 2014/04/23 00:20:31, siva wrote:
> Should this be UNREACHABLE() we don't expect to send receive ports out in
> messages correct?

Throwing an exception in that case.

[Ivan Posva, 2014-04-23 19:44:22]

Committed patchset #4 manually as r35325 (presubmit successful).