Adds a special case for sending an int over a port with the native API.

Adds a special case for sending an int over a port with the native API.

This improves dart <=> C++ IPC round-trip-times for Mojo by 10-20%.

BUG=

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

[zra, 2015-12-05 04:15:57]

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

[Ivan Posva, 2015-12-08 08:25:55]

https://codereview.chromium.org/1499853004/diff/40001/runtime/vm/message.h
File runtime/vm/message.h (right):

https://codereview.chromium.org/1499853004/diff/40001/runtime/vm/message.h#ne...
runtime/vm/message.h:122: intptr_t integer;  // Active when type_ ==
kIntegerType.
Let's discuss this change in person to figure out whether there are more ways of
optimizing this.

[zra, 2015-12-08 21:56:36]

As discussed, generalized by storing a RawObject pointer in the Message's data_
field when len_ is 0.

I kept the Dart_PostInteger API call to allow passing an integer without
wrapping in a Dart_Handle or a CObject.

[zra, 2015-12-09 22:27:33]

ptal

[Ivan Posva, 2015-12-09 22:35:29]

I had a couple Drafts still on my machine. Not sure whether these still apply...

-Ivan

https://codereview.chromium.org/1499853004/diff/60001/runtime/vm/message.h
File runtime/vm/message.h (right):

https://codereview.chromium.org/1499853004/diff/60001/runtime/vm/message.h#ne...
runtime/vm/message.h:71: ASSERT((priority == kNormalPriority) ||
Also please assert that raw_obj is not a heap object or at least a VM heap
object.

https://codereview.chromium.org/1499853004/diff/60001/runtime/vm/native_api_i...
File runtime/vm/native_api_impl.cc (right):

https://codereview.chromium.org/1499853004/diff/60001/runtime/vm/native_api_i...
runtime/vm/native_api_impl.cc:60: if (!Smi::IsValid(message)) {
How about not allowing failure here in the first place? Then failure is
equivalent to the other failures where posting failed and not because the value
is unexpectedly out of range.

DART_EXPORT bool Dart_PostInteger(Dart_Port port_id, int64_t value) {
  Message* msg;
  if (Smi::IsValid(value)) {
    msg = new Message(port_id, Smi::New(value), Message::kNormalPriority);
  } else {
    // Or use the Dart_CObject API here.
    const Integer& obj = Integer::Handle(Integer::New(value));
    MessageWriter writer(...);
    writer.WriteMessage(obj);
    msg = new Message(port_id, writer.data(), writer.BytesWritten(),
Message::kNormalPriority);
  }
  return PortMap::PostMessage(msg);
}

https://codereview.chromium.org/1499853004/diff/60001/runtime/vm/native_api_i...
runtime/vm/native_api_impl.cc:71: DART_EXPORT bool
Dart_PostSimpleObject(Dart_Port port_id, Dart_Handle message) {
Please remove. The short cutting here should be rolled into the regular
Dart_Post implementation.

https://codereview.chromium.org/1499853004/diff/60001/runtime/vm/native_messa...
File runtime/vm/native_message_handler.cc (right):

https://codereview.chromium.org/1499853004/diff/60001/runtime/vm/native_messa...
runtime/vm/native_message_handler.cc:41: ASSERT(message->len() > 0);
You need to support receiving the RawObject* based messages here as well.

[Ivan Posva, 2015-12-09 22:45:28]

-Ivan

https://codereview.chromium.org/1499853004/diff/80001/runtime/include/dart_na...
File runtime/include/dart_native_api.h (right):

https://codereview.chromium.org/1499853004/diff/80001/runtime/include/dart_na...
runtime/include/dart_native_api.h:115: DART_EXPORT bool
Dart_PostSimpleObject(Dart_Port port_id, Dart_Handle message);
Please remove this as it should be handled by Dart_Post in the general case if
it is an object that could have been sent using the shortcut.

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

https://codereview.chromium.org/1499853004/diff/80001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:472: if (message->len() > 0) {
How about turning it around:
if (message->IsRaw()) {
  msg_obj = message->raw_obj();
  ASSERT(msg_obj.IsSmi() || msg_obj.InVMHeap());
} else {
  // Deserialize the message object.
  ...
}

// Error checking should be done in both cases too.

https://codereview.chromium.org/1499853004/diff/80001/runtime/vm/native_messa...
File runtime/vm/native_message_handler.cc (right):

https://codereview.chromium.org/1499853004/diff/80001/runtime/vm/native_messa...
runtime/vm/native_message_handler.cc:41: ASSERT(message->len() > 0);
You cannot assert this. The poster of the message on the other side is free to
use the shortcut without knowing who the recipient is.

[zra, 2015-12-10 00:07:29]

https://codereview.chromium.org/1499853004/diff/60001/runtime/vm/message.h
File runtime/vm/message.h (right):

https://codereview.chromium.org/1499853004/diff/60001/runtime/vm/message.h#ne...
runtime/vm/message.h:71: ASSERT((priority == kNormalPriority) ||
On 2015/12/09 22:35:29, Ivan Posva wrote:
> Also please assert that raw_obj is not a heap object or at least a VM heap
> object.

Done.

https://codereview.chromium.org/1499853004/diff/60001/runtime/vm/native_api_i...
File runtime/vm/native_api_impl.cc (right):

https://codereview.chromium.org/1499853004/diff/60001/runtime/vm/native_api_i...
runtime/vm/native_api_impl.cc:60: if (!Smi::IsValid(message)) {
On 2015/12/09 22:35:29, Ivan Posva wrote:
> How about not allowing failure here in the first place? Then failure is
> equivalent to the other failures where posting failed and not because the
value
> is unexpectedly out of range.
> 
> DART_EXPORT bool Dart_PostInteger(Dart_Port port_id, int64_t value) {
>   Message* msg;
>   if (Smi::IsValid(value)) {
>     msg = new Message(port_id, Smi::New(value), Message::kNormalPriority);
>   } else {
>     // Or use the Dart_CObject API here.
>     const Integer& obj = Integer::Handle(Integer::New(value));
>     MessageWriter writer(...);
>     writer.WriteMessage(obj);
>     msg = new Message(port_id, writer.data(), writer.BytesWritten(),
> Message::kNormalPriority);
>   }
>   return PortMap::PostMessage(msg);
> }

Done.

https://codereview.chromium.org/1499853004/diff/60001/runtime/vm/native_api_i...
runtime/vm/native_api_impl.cc:71: DART_EXPORT bool
Dart_PostSimpleObject(Dart_Port port_id, Dart_Handle message) {
On 2015/12/09 22:35:29, Ivan Posva wrote:
> Please remove. The short cutting here should be rolled into the regular
> Dart_Post implementation.

Done.

https://codereview.chromium.org/1499853004/diff/80001/runtime/include/dart_na...
File runtime/include/dart_native_api.h (right):

https://codereview.chromium.org/1499853004/diff/80001/runtime/include/dart_na...
runtime/include/dart_native_api.h:115: DART_EXPORT bool
Dart_PostSimpleObject(Dart_Port port_id, Dart_Handle message);
On 2015/12/09 22:45:28, Ivan Posva wrote:
> Please remove this as it should be handled by Dart_Post in the general case if
> it is an object that could have been sent using the shortcut.

Done.

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

https://codereview.chromium.org/1499853004/diff/80001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:472: if (message->len() > 0) {
On 2015/12/09 22:45:28, Ivan Posva wrote:
> How about turning it around:
> if (message->IsRaw()) {
>   msg_obj = message->raw_obj();
>   ASSERT(msg_obj.IsSmi() || msg_obj.InVMHeap());
> } else {
>   // Deserialize the message object.
>   ...
> }
> 
> // Error checking should be done in both cases too.

Done.

https://codereview.chromium.org/1499853004/diff/80001/runtime/vm/native_messa...
File runtime/vm/native_message_handler.cc (right):

https://codereview.chromium.org/1499853004/diff/80001/runtime/vm/native_messa...
runtime/vm/native_message_handler.cc:41: ASSERT(message->len() > 0);
On 2015/12/09 22:45:28, Ivan Posva wrote:
> You cannot assert this. The poster of the message on the other side is free to
> use the shortcut without knowing who the recipient is.

I'm not sure I see a good way to convert a RawObject to a Dart_CObject, here. I
could try to serialize the RawObject as if it had been posted by Dart_Post, but
the problem there is that the NativeMessageHandlers run on a thread that doesn't
have an Isolate or Thread, which the SnapshotWriter assumes exists.

Thoughts?

[zra, 2015-12-10 20:51:33]

https://codereview.chromium.org/1499853004/diff/80001/runtime/vm/native_messa...
File runtime/vm/native_message_handler.cc (right):

https://codereview.chromium.org/1499853004/diff/80001/runtime/vm/native_messa...
runtime/vm/native_message_handler.cc:41: ASSERT(message->len() > 0);
On 2015/12/09 22:45:28, Ivan Posva wrote:
> You cannot assert this. The poster of the message on the other side is free to
> use the shortcut without knowing who the recipient is.

I've added some code to this CL to handle RawObject -> CObject conversion of
Smis and Null. I put it in //runtime/vm/dart_api_message.h. PTAL.

[Ivan Posva, 2015-12-11 21:02:39]

LGTMwC

Thanks,
-Ivan

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

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1592: RawObject* raw_obj =
Api::UnwrapHandle(handle);
Please make sure that this code is wrapped in a NoSafepointScope as you have a
naked RawObject sitting around here.

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/dart_api_me...
File runtime/vm/dart_api_message.h (right):

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/dart_api_me...
runtime/vm/dart_api_message.h:231: static void ConvertSmi(RawObject* raw_obj,
Dart_CObject* c_obj) {
ConvertSmi(RawSmi* raw_smi, ...)

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/dart_api_me...
runtime/vm/dart_api_message.h:233: intptr_t value =
reinterpret_cast<intptr_t>(raw_obj) >> kSmiTagShift;
Smi::Value(raw_smi)

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/dart_api_me...
runtime/vm/dart_api_message.h:243: static void ConvertNull(RawObject* raw_obj,
Dart_CObject* c_obj) {
Drop the raw_obj parameter.

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

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:149: obj_len = writer.BytesWritten();
Why not call SerializeObject() here?

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1216:
PortMap::PostMessage(SerializeObjectToMessage(port_id, response));
How about shortening the name SerializeMessage?

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/json_stream.cc
File runtime/vm/json_stream.cc (right):

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/json_stream...
runtime/vm/json_stream.cc:166: Message::kNormalPriority));
Why not call the shortcut constructor?

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

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/port_test.c...
runtime/vm/port_test.cc:160: port, reinterpret_cast<uint8_t*>(Smi::New(42)), 0,
Shortcut constructor?

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/port_test.c...
runtime/vm/port_test.cc:175: port, reinterpret_cast<uint8_t*>(Object::null()),
0,
ditto

[zra, 2015-12-11 22:01:31]

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

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:1592: RawObject* raw_obj =
Api::UnwrapHandle(handle);
On 2015/12/11 21:02:38, Ivan Posva wrote:
> Please make sure that this code is wrapped in a NoSafepointScope as you have a
> naked RawObject sitting around here.

Done.

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/dart_api_me...
File runtime/vm/dart_api_message.h (right):

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/dart_api_me...
runtime/vm/dart_api_message.h:231: static void ConvertSmi(RawObject* raw_obj,
Dart_CObject* c_obj) {
On 2015/12/11 21:02:38, Ivan Posva wrote:
> ConvertSmi(RawSmi* raw_smi, ...)

Done.

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/dart_api_me...
runtime/vm/dart_api_message.h:233: intptr_t value =
reinterpret_cast<intptr_t>(raw_obj) >> kSmiTagShift;
On 2015/12/11 21:02:38, Ivan Posva wrote:
> Smi::Value(raw_smi)

Done.

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/dart_api_me...
runtime/vm/dart_api_message.h:243: static void ConvertNull(RawObject* raw_obj,
Dart_CObject* c_obj) {
On 2015/12/11 21:02:38, Ivan Posva wrote:
> Drop the raw_obj parameter.

Done.

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

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:149: obj_len = writer.BytesWritten();
On 2015/12/11 21:02:39, Ivan Posva wrote:
> Why not call SerializeObject() here?

Done.

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1216:
PortMap::PostMessage(SerializeObjectToMessage(port_id, response));
On 2015/12/11 21:02:39, Ivan Posva wrote:
> How about shortening the name SerializeMessage?

Done.

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/json_stream.cc
File runtime/vm/json_stream.cc (right):

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/json_stream...
runtime/vm/json_stream.cc:166: Message::kNormalPriority));
On 2015/12/11 21:02:39, Ivan Posva wrote:
> Why not call the shortcut constructor?

Done.

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

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/port_test.c...
runtime/vm/port_test.cc:160: port, reinterpret_cast<uint8_t*>(Smi::New(42)), 0,
On 2015/12/11 21:02:39, Ivan Posva wrote:
> Shortcut constructor?

Done.

https://codereview.chromium.org/1499853004/diff/140001/runtime/vm/port_test.c...
runtime/vm/port_test.cc:175: port, reinterpret_cast<uint8_t*>(Object::null()),
0,
On 2015/12/11 21:02:39, Ivan Posva wrote:
> ditto

Done.

[turnidge, 2015-12-11 22:30:48]

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

https://codereview.chromium.org/1499853004/diff/160001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:138: static Message* SerializeMessage(
Instead of having this helper function, how about we add a PortMap::Post or
PortMap::Send that takes the arguments to the Message constuctor and decides
what to do internally.  It would be nice to get all of the message allocations
in one place so we have the option of using a free list.  It also saves other
callers from needing to duplicate this logic.

https://codereview.chromium.org/1499853004/diff/160001/runtime/vm/native_mess...
File runtime/vm/native_message_handler.cc (right):

https://codereview.chromium.org/1499853004/diff/160001/runtime/vm/native_mess...
runtime/vm/native_message_handler.cc:49: bool success =
ApiObjectConverter::Convert(message->raw_obj(), &object);
As discussed offline, let's see if we can move this conversion stuff inside
reader.ReadMessage().

[Ivan Posva, 2015-12-11 22:53:39]

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

https://codereview.chromium.org/1499853004/diff/160001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:138: static Message* SerializeMessage(
On 2015/12/11 22:30:48, turnidge wrote:
> Instead of having this helper function, how about we add a PortMap::Post or
> PortMap::Send that takes the arguments to the Message constuctor and decides
> what to do internally.  It would be nice to get all of the message allocations
> in one place so we have the option of using a free list.  It also saves other
> callers from needing to duplicate this logic.

Todd,

I tend to agree with your concern about the fact that the message allocation
code is too distributed, but I would like for Zach to be able to land this soon.
Also I don't think the PortMap code is the right abstraction to trigger
serialization.

We should discuss alternatives in person.

Thanks,
-Ivan

[zra, 2015-12-11 23:44:32]

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

https://codereview.chromium.org/1499853004/diff/160001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:138: static Message* SerializeMessage(
On 2015/12/11 22:53:39, Ivan Posva wrote:
> On 2015/12/11 22:30:48, turnidge wrote:
> > Instead of having this helper function, how about we add a PortMap::Post or
> > PortMap::Send that takes the arguments to the Message constuctor and decides
> > what to do internally.  It would be nice to get all of the message
allocations
> > in one place so we have the option of using a free list.  It also saves
other
> > callers from needing to duplicate this logic.
> 
> Todd,
> 
> I tend to agree with your concern about the fact that the message allocation
> code is too distributed, but I would like for Zach to be able to land this
soon.
> Also I don't think the PortMap code is the right abstraction to trigger
> serialization.
> 
> We should discuss alternatives in person.
> 
> Thanks,
> -Ivan

Added TODO here.

https://codereview.chromium.org/1499853004/diff/160001/runtime/vm/native_mess...
File runtime/vm/native_message_handler.cc (right):

https://codereview.chromium.org/1499853004/diff/160001/runtime/vm/native_mess...
runtime/vm/native_message_handler.cc:49: bool success =
ApiObjectConverter::Convert(message->raw_obj(), &object);
On 2015/12/11 22:30:48, turnidge wrote:
> As discussed offline, let's see if we can move this conversion stuff inside
> reader.ReadMessage().

I've added a TODO here.

[zra, 2015-12-12 00:07:18]

Description was changed from

==========
Adds a special case for sending an int over a port with the native API.

This improves dart <=> C++ IPC round-trip-times for Mojo by 10-20%.

BUG=
==========

to

==========
Adds a special case for sending an int over a port with the native API.

This improves dart <=> C++ IPC round-trip-times for Mojo by 10-20%.

BUG=

Committed:
https://github.com/dart-lang/sdk/commit/a0970835f0571a3c5d67813476715dcb7ea06c05
==========

[zra, 2015-12-12 00:07:20]

Committed patchset #11 (id:200001) manually as
a0970835f0571a3c5d67813476715dcb7ea06c05 (presubmit successful).

[Ivan Posva, 2015-12-12 01:10:55]

This is leading to the build failure on Mac at least.

-Ivan

https://codereview.chromium.org/1499853004/diff/200001/runtime/vm/native_api_...
File runtime/vm/native_api_impl.cc (right):

https://codereview.chromium.org/1499853004/diff/200001/runtime/vm/native_api_...
runtime/vm/native_api_impl.cc:67: return Dart_PostCObject(port_id, &cobj);
I noticed that you are calling Dart_PostCObject here, but you need to move the
code out into a shared static and call it from both entry points. Unfortunately
I did not save the comment, sorry!

[zra, 2015-12-12 01:50:50]

https://codereview.chromium.org/1499853004/diff/200001/runtime/vm/native_api_...
File runtime/vm/native_api_impl.cc (right):

https://codereview.chromium.org/1499853004/diff/200001/runtime/vm/native_api_...
runtime/vm/native_api_impl.cc:67: return Dart_PostCObject(port_id, &cobj);
On 2015/12/12 01:10:55, Ivan Posva wrote:
> I noticed that you are calling Dart_PostCObject here, but you need to move the
> code out into a shared static and call it from both entry points.
Unfortunately
> I did not save the comment, sorry!

I won't be able to submit a fix for a couple hours.