mojo: MessageBuilder

mojo: MessageBuilder

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223304

[vtl, 2013-09-11 17:37:15]

I haven't looked at the implementation super-closely, but here are some
comments....

Also, I've been thinking about the directory structure a bit, and the situation
is a bit odd/complicated.

I've been thinking about putting stuff under:

mojo/public -- for stuff visible to clients/apps
mojo/internal -- for stuff which isn't

Since the serializer code is visible to clients/apps (e.g., they probably want
to link with it), I guess it'd go under mojo/public.

Of course, stuff in mojo/public could be split into a bunch of categories:
- declarations of the primitives (e.g., WriteMessage())
- declarations of APIs (IDL, generated wrappers)
- "needed" libraries, such as the serializer -- though a client/app could
substitute their own
- "needed" tools (e.g., IDL compiler)

I put my public headers (errr, actually, I currently only have one) in
mojo/public/system, and the implementation in mojo/internal/system (internal,
since it's not meant to be visible to clients/apps).... But maybe that should be
mojo/public/core and mojo/system/core (or mojo/internal/core)?

https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/core/ipc_handle.h
File mojo/ipc/core/ipc_handle.h (right):

https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/core/ipc_handle....
mojo/ipc/core/ipc_handle.h:12: struct Handle {
Probably the C++ layer should build on top of a C layer?

(I'm ambivalent about wrapping a single value in a struct, though it does have
good type-safety advantages.)

I guess the C API would probably use the uint32_t directly? I've been using
typedefs, e.g., |typedef uint32_t MojoHandle;|, but this isn't super-important.

More significantly, I'm thinking that handles should have
movable-but-not-copyable semantics, which could actually be modelled/enforced in
a C++ wrapper. I.e., when you pass a handle, you no longer have access to it. If
you want to keep access to that object, you need to "duplicate" the handle
explicitly and the semantics of that depend on the type of object (and may not
even be allowed).

https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/README
File mojo/ipc/message/README (right):

https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/README#n...
mojo/ipc/message/README:28: [ U32 ]
I guess by "U32" here, you mean any 32 bits of data (per FieldType). Which I
guess also implies that chars, bools, int16_ts, etc. all get padded out to 32
bits (which I guess is fine), right?

https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/README#n...
mojo/ipc/message/README:29: [ U64 ]
Is there any desire to have 64-bit quantities be aligned (on 8-byte boundaries)?

If added <empty> as a possible FieldValue, we could reserve FieldName=0,
FieldType=0 as an empty padding field. (We could also do some optimizations,
e.g., optimize bools by having separate "bool_true" and "bool_false"
FieldTypes.)

https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/README#n...
mojo/ipc/message/README:30: [ FieldSize | FieldBytes ]
Similarly, do we want to insist that FieldBytes be padded out to a multiple of 4
bytes?

https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/README#n...
mojo/ipc/message/README:31: 
It occurs to me that we want to assign ordinals to passed handles also. Should
this be done by adding data to the message data itself (e.g., a handle
FieldType, with the FieldValue indicating the index in the array of handles) or
separately?

(I favor the former, since it keeps the message serialization format separate
from the transport primitives.)

https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/ipc_mess...
File mojo/ipc/message/ipc_message.h (right):

https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/ipc_mess...
mojo/ipc/message/ipc_message.h:29: kIPCMessageFieldType_Handle  = 0xE,
I see!

https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/ipc_mess...
File mojo/ipc/message/ipc_message_builder.cc (right):

https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/ipc_mess...
mojo/ipc/message/ipc_message_builder.cc:88: if (!AppendU32(name,
kIPCMessageFieldType_Handle, handle.value))
handle.value isn't going to be a universal (cross-process) thing, but probably
you should append its index into handles_?

[vtl, 2013-09-12 01:16:10]

https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/README
File mojo/ipc/message/README (right):

https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/README#n...
mojo/ipc/message/README:25: [ U16 ]
Another thought occurred to me: Maybe we should steal a few bits from this and
encode the size?

E.g., take 2 bits and have them mean:

00 - 0 bytes
01 - 4 bytes
10 - 8 bytes
11 - size specified by FieldSize

(obviously, taking 3 bits gives you a bit more flexibility).

The advantage is forwards-compatibility, so that you can skip fields whose
FieldTypes that you don't know about.

[darin (slow to review), 2013-09-12 05:15:09]

On Wed, Sep 11, 2013 at 10:37 AM, <vtl@google.com> wrote:

> I haven't looked at the implementation super-closely, but here are some
> comments....
>
> Also, I've been thinking about the directory structure a bit, and the
> situation
> is a bit odd/complicated.
>
> I've been thinking about putting stuff under:
>
> mojo/public -- for stuff visible to clients/apps
> mojo/internal -- for stuff which isn't
>
> Since the serializer code is visible to clients/apps (e.g., they probably
> want
> to link with it), I guess it'd go under mojo/public.
>
> Of course, stuff in mojo/public could be split into a bunch of categories:
> - declarations of the primitives (e.g., WriteMessage())
> - declarations of APIs (IDL, generated wrappers)
> - "needed" libraries, such as the serializer -- though a client/app could
> substitute their own
> - "needed" tools (e.g., IDL compiler)
>
> I put my public headers (errr, actually, I currently only have one) in
> mojo/public/system, and the implementation in mojo/internal/system
> (internal,
> since it's not meant to be visible to clients/apps).... But maybe that
> should be
> mojo/public/core and mojo/system/core (or mojo/internal/core)?
>

It seems like there are two kinds of public things:

1) core runtime interface
2) helper libraries

You have to use #1, but #2 is optional. Serializers are in bucket #2, for
example.



>
>
> https://codereview.chromium.**org/23629032/diff/10001/mojo/**
>
ipc/core/ipc_handle.h<https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/core/ipc_handle.h>
> File mojo/ipc/core/ipc_handle.h (right):
>
> https://codereview.chromium.**org/23629032/diff/10001/mojo/**
>
ipc/core/ipc_handle.h#**newcode12<https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/core/ipc_handle.h#newcode12>
> mojo/ipc/core/ipc_handle.h:12: struct Handle {
> Probably the C++ layer should build on top of a C layer?
>
> (I'm ambivalent about wrapping a single value in a struct, though it
> does have good type-safety advantages.)
>

I think we should probably do it for type-safety reasons. It allows you to
write method overrides that distinguish uint32_t from MojoHandle.



>
> I guess the C API would probably use the uint32_t directly? I've been
> using typedefs, e.g., |typedef uint32_t MojoHandle;|, but this isn't
> super-important.
>

Yeah, the C API can just use typedefs.



> More significantly, I'm thinking that handles should have
> movable-but-not-copyable semantics, which could actually be
> modelled/enforced in a C++ wrapper. I.e., when you pass a handle, you no
> longer have access to it. If you want to keep access to that object, you
> need to "duplicate" the handle explicitly and the semantics of that
> depend on the type of object (and may not even be allowed).
>

I had typed a "Duplicate(MojoHandle)" API a couple times (and deleted it
each time). Wasn't sure if we wanted to go there. It seems like the system
can take care of acquiring a reference to the handle when it is passed in.

What issues are you worried about? I can imagine that passing a pipe should
transfer it because you don't want to end up with multiple readers on the
same pipe. Is that the core motivation for transfer semantics being default?



>
> https://codereview.chromium.**org/23629032/diff/10001/mojo/**
>
ipc/message/README<https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/README>
> File mojo/ipc/message/README (right):
>
> https://codereview.chromium.**org/23629032/diff/10001/mojo/**
>
ipc/message/README#newcode28<https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/README#newcode28>
> mojo/ipc/message/README:28: [ U32 ]
> I guess by "U32" here, you mean any 32 bits of data (per FieldType).
> Which I guess also implies that chars, bools, int16_ts, etc. all get
> padded out to 32 bits (which I guess is fine), right?
>

Yeah, that's the idea.



>
> https://codereview.chromium.**org/23629032/diff/10001/mojo/**
>
ipc/message/README#newcode29<https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/README#newcode29>
> mojo/ipc/message/README:29: [ U64 ]
> Is there any desire to have 64-bit quantities be aligned (on 8-byte
> boundaries)?
>

I'm not sure. That's a good question. It might be low-cost to do that since
U32 fields would already take 8-bytes due to the FieldHeader. It would just
imply extra padding for variable length fields. Hmm... I think I'll align
to 8-bytes then :)



>
> If added <empty> as a possible FieldValue, we could reserve FieldName=0,
> FieldType=0 as an empty padding field. (We could also do some
> optimizations, e.g., optimize bools by having separate "bool_true" and
> "bool_false" FieldTypes.)
>

Hmm... is this to provide future support for padding? I think bool_true /
bool_false would imply that bools could fit within a single word, but that
would complicate achieving 8-byte padding. Hmm...



>
> https://codereview.chromium.**org/23629032/diff/10001/mojo/**
>
ipc/message/README#newcode30<https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/README#newcode30>
> mojo/ipc/message/README:30: [ FieldSize | FieldBytes ]
> Similarly, do we want to insist that FieldBytes be padded out to a
> multiple of 4 bytes?
>

That's what the code does. I can change it to 8-bytes.


>
> https://codereview.chromium.**org/23629032/diff/10001/mojo/**
>
ipc/message/README#newcode31<https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/README#newcode31>
> mojo/ipc/message/README:31:
> It occurs to me that we want to assign ordinals to passed handles also.
> Should this be done by adding data to the message data itself (e.g., a
> handle FieldType, with the FieldValue indicating the index in the array
> of handles) or separately?
>
> (I favor the former, since it keeps the message serialization format
> separate from the transport primitives.)
>

I like the approach of serializing indices into the handles vector. Then it
will be possible for entirely different message formats to be used. The
kernel should never have to decode a message.



>
> https://codereview.chromium.**org/23629032/diff/10001/mojo/**
>
ipc/message/ipc_message.h<https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/ipc_message.h>
> File mojo/ipc/message/ipc_message.h (right):
>
> https://codereview.chromium.**org/23629032/diff/10001/mojo/**
>
ipc/message/ipc_message.h#**newcode29<https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/ipc_message.h#newcode29>
> mojo/ipc/message/ipc_message.**h:29: kIPCMessageFieldType_Handle  = 0xE,
> I see!
>
> https://codereview.chromium.**org/23629032/diff/10001/mojo/**
>
ipc/message/ipc_message_**builder.cc<https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/ipc_message_builder.cc>
> File mojo/ipc/message/ipc_message_**builder.cc (right):
>
> https://codereview.chromium.**org/23629032/diff/10001/mojo/**
>
ipc/message/ipc_message_**builder.cc#newcode88<https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/ipc_message_builder.cc#newcode88>
> mojo/ipc/message/ipc_message_**builder.cc:88: if (!AppendU32(name,
> kIPCMessageFieldType_Handle, handle.value))
> handle.value isn't going to be a universal (cross-process) thing, but
> probably you should append its index into handles_?
>

Done.


>
>
https://codereview.chromium.**org/23629032/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2013-09-12 05:15:25]

On 2013/09/12 01:16:10, vtl wrote:
> https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/README
> File mojo/ipc/message/README (right):
> 
>
https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/README#n...
> mojo/ipc/message/README:25: [ U16 ]
> Another thought occurred to me: Maybe we should steal a few bits from this and
> encode the size?
> 
> E.g., take 2 bits and have them mean:
> 
> 00 - 0 bytes
> 01 - 4 bytes
> 10 - 8 bytes
> 11 - size specified by FieldSize
> 
> (obviously, taking 3 bits gives you a bit more flexibility).
> 
> The advantage is forwards-compatibility, so that you can skip fields whose
> FieldTypes that you don't know about.

Great idea! Done.

[darin (slow to review), 2013-09-12 05:17:39]

I think I'll actually leave out the 0 byte case for now. I don't see a use
case given the idea of padding to 8-bytes.

-Darin


On Wed, Sep 11, 2013 at 10:15 PM, <darin@chromium.org> wrote:

> On 2013/09/12 01:16:10, vtl wrote:
>
>> https://codereview.chromium.**org/23629032/diff/10001/mojo/**
>>
ipc/message/README<https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/README>
>> File mojo/ipc/message/README (right):
>>
>
>
> https://codereview.chromium.**org/23629032/diff/10001/mojo/**
>
ipc/message/README#newcode25<https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/README#newcode25>
>
>> mojo/ipc/message/README:25: [ U16 ]
>> Another thought occurred to me: Maybe we should steal a few bits from
>> this and
>> encode the size?
>>
>
>  E.g., take 2 bits and have them mean:
>>
>
>  00 - 0 bytes
>> 01 - 4 bytes
>> 10 - 8 bytes
>> 11 - size specified by FieldSize
>>
>
>  (obviously, taking 3 bits gives you a bit more flexibility).
>>
>
>  The advantage is forwards-compatibility, so that you can skip fields whose
>> FieldTypes that you don't know about.
>>
>
> Great idea! Done.
>
>
https://codereview.chromium.**org/23629032/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Avi (use Gerrit), 2013-09-12 05:29:32]

drive-by nitpicking

https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/ipc_mess...
File mojo/ipc/message/ipc_message_unittest.cc (right):

https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/ipc_mess...
mojo/ipc/message/ipc_message_unittest.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
No "(c)" in new headers. Style is:

// Copyright 2013 The Chromium Authors. All rights reserved.

[darin (slow to review), 2013-09-12 05:32:32]

Thank you! I always just copy/paste copyright headers. Then the server
modifies my commit, and git has fun merging :-P


On Wed, Sep 11, 2013 at 10:29 PM, <avi@chromium.org> wrote:

> drive-by nitpicking
>
>
> https://codereview.chromium.**org/23629032/diff/10001/mojo/**
>
ipc/message/ipc_message_**unittest.cc<https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/ipc_message_unittest.cc>
> File mojo/ipc/message/ipc_message_**unittest.cc (right):
>
> https://codereview.chromium.**org/23629032/diff/10001/mojo/**
>
ipc/message/ipc_message_**unittest.cc#newcode1<https://codereview.chromium.org/23629032/diff/10001/mojo/ipc/message/ipc_message_unittest.cc#newcode1>
> mojo/ipc/message/ipc_message_**unittest.cc:1: // Copyright (c) 2013 The
> Chromium Authors. All rights reserved.
> No "(c)" in new headers. Style is:
>
> // Copyright 2013 The Chromium Authors. All rights reserved.
>
>
https://codereview.chromium.**org/23629032/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[viettrungluu, 2013-09-13 16:40:52]

LGTM (with issues below fixed), though of course the message format is still
subject to ongoing discussion....

https://codereview.chromium.org/23629032/diff/24001/mojo/public/libs/message/...
File mojo/public/libs/message/message_builder.cc (right):

https://codereview.chromium.org/23629032/diff/24001/mojo/public/libs/message/...
mojo/public/libs/message/message_builder.cc:10: #include <limits>
nit: I think we usually separate C and C++ headers by a blank line, no?

https://codereview.chromium.org/23629032/diff/24001/mojo/public/libs/message/...
mojo/public/libs/message/message_builder.cc:17: // Aligns 'value' by rounding it
up to the next multiple of 'alignment'
nit: needs punctuation

https://codereview.chromium.org/23629032/diff/24001/mojo/public/libs/message/...
mojo/public/libs/message/message_builder.cc:19: return value + (alignment -
(value % alignment)) % alignment;
The second |% alignment| is redundant.

https://codereview.chromium.org/23629032/diff/24001/mojo/public/libs/message/...
mojo/public/libs/message/message_builder.cc:32: type,
I guess this code is written assuming a little-endian processor?

https://codereview.chromium.org/23629032/diff/24001/mojo/public/libs/message/...
mojo/public/libs/message/message_builder.cc:120:
reinterpret_cast<uint32_t*>(&data_[0])[1] = payload_size;
I think this violates strict aliasing, so you should use memcpy() instead?

[commit-bot: I haz the power, 2013-09-15 07:48:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/darin@chromium.org/23629032/22002

[commit-bot: I haz the power, 2013-09-15 08:03:23]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-15 18:20:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/darin@chromium.org/23629032/61001

[commit-bot: I haz the power, 2013-09-15 18:24:50]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-09-15 18:43:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/darin@chromium.org/23629032/61001

[commit-bot: I haz the power, 2013-09-16 02:55:21]

Change committed as 223304