Mojom runtime type info: New implementation for Dart.

Mojom runtime type info: New implementation for Dart.

This is a follow on to https://codereview.chromium.org/1719873003/
and does for the Dart bindings the same thing that that change did for the Go bindings.

Our implementation of runtime type information is changing and this CL implements the change for Dart.

In the older implementation the runtime type structures are computed and constructed by the code generators (thus in Python and the Jinja 2 templates).
In this newer implementation the Mojom parser computes the information and stores it in a mojom_types.RuntimeTypeInfo
structure. Then the code generators insert a literal array of bytes representing a serialized RuntimeTypeInfo into the generated Dart code. At runtime all a Mojo application needs to do
is deserialize the RuntimeTypeInfo struct and use it to answer queries.

One particular change to note is that like with Go, the new implementation generates an implementation of getTopLevelInterface() that returns null for an interface that is not a top-level interface (i.e. is not marked with the "ServiceName" annotation.)

BUG=#448
R=alexfandrianto@google.com, azani@chromium.org, zra@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/04e96bfd15364f988b745d76f17b59e214cb9cc6

[rudominer, 2016-03-02 00:18:56]

Description was changed from

==========
Regenerate the .mojom.dart files.


Mojom runtime type info: New implementation for Dart.

BUG=#448
==========

to

==========
Mojom runtime type info: New implementation for Dart.


BUG=#448
==========

[rudominer, 2016-03-02 00:25:38]

Patchset #1 (id:1) has been deleted

[rudominer, 2016-03-02 00:25:58]

Patchset #1 (id:10001) has been deleted

[rudominer, 2016-03-02 00:28:14]

Description was changed from

==========
Mojom runtime type info: New implementation for Dart.


BUG=#448
==========

to

==========
Mojom runtime type info: New implementation for Dart.

This is a follow on to https://codereview.chromium.org/1719873003/
and does for the Dart bindings the same thing that that change did for the Go
bindings.

Our implementation of runtime type information is changing and this CL
implements the change for Dart.

In the older implementation the runtime type structures are computed and
constructed by the code generators (thus in Python and the Jinja 2 templates).
In this newer implementation the Mojom parser computes the information and
stores it in a mojom_types.RuntimeTypeInfo
structure. Then the code generators insert a literal array of bytes representing
a serialized RuntimeTypeInfo into the generated Dart code. At runtime all a Mojo
application needs to do
is deserialize the RuntimeTypeInfo struct and use it to answer queries.


BUG=#448
==========

[rudominer, 2016-03-02 01:06:48]

Patchset #1 (id:20001) has been deleted

[rudominer, 2016-03-02 01:10:28]

rudominer@chromium.org changed reviewers:
+ alexfandrianto@google.com, azani@chromium.org, zra@google.com

[rudominer, 2016-03-02 01:10:30]

Hi Zach, Alex and Alex,

ptal.

Note that the changes to the non-test .mojom.dart files are all extra
white-space removal only.

- MItch

[alexfandrianto, 2016-03-02 05:27:12]

lgtm

(These changes mirror the Go ones, so no surprise there.)

https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/apptests/dart...
File mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart
(left):

https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/apptests/dart...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:57:
_compare(c, d);
Does this part of the test need to be removed? If possible, I'd like to keep it
so that we can verify that getTypeDefinition is implemented correctly.

If the problem is the type key, we can reuse the computeTypeKey function (and
TODO) you wrote for the embedder test.

+// computeTypeKey encapsulates the algorithm for computing the type key
+// of a type given its fully-qualified name.
+//
+// TODO(rudominer) Delete this when we add accessors for Mojom types
+// in the Dart bindings.
+String computeTypeKey(String fullyQualifiedName) {
+  return "TYPE_KEY:" + fullyQualifiedName;
+}
+

https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart
(right):

https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:877:
responseFactory(null);
Observation: The CppSide interface didn't have a service name; that's why its
service description now returns null for each method.

https://codereview.chromium.org/1753013002/diff/10002/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/1753013002/diff/10002/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:12:
{%- if should_gen_mojom_types and interface.service_name %}
It may be worth noting explicitly in the CL description that, like Go, Dart
interfaces will only receive the full service description if they also have a
service name. Doing so helps explain the changes in some of the
_mojo_for_test_only *.mojom.dart files, where their service description
implementations are now returning null.

https://codereview.chromium.org/1753013002/diff/10002/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl
(right):

https://codereview.chromium.org/1753013002/diff/10002/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl:34:
return runtimeTypeInfo__;
Optionally, you can rewrite 30 to 35 as
{{typepkg}}RuntimeTypeInfo getRuntimeTypeInfo() => runtimeTypeInfo ??
_initRuntimeTypeInfo();

if you also add 'return runtimeTypeInfo__' to _initRuntimeTypeInfo()

[rudominer, 2016-03-02 16:04:55]

Description was changed from

==========
Mojom runtime type info: New implementation for Dart.

This is a follow on to https://codereview.chromium.org/1719873003/
and does for the Dart bindings the same thing that that change did for the Go
bindings.

Our implementation of runtime type information is changing and this CL
implements the change for Dart.

In the older implementation the runtime type structures are computed and
constructed by the code generators (thus in Python and the Jinja 2 templates).
In this newer implementation the Mojom parser computes the information and
stores it in a mojom_types.RuntimeTypeInfo
structure. Then the code generators insert a literal array of bytes representing
a serialized RuntimeTypeInfo into the generated Dart code. At runtime all a Mojo
application needs to do
is deserialize the RuntimeTypeInfo struct and use it to answer queries.


BUG=#448
==========

to

==========
Mojom runtime type info: New implementation for Dart.

This is a follow on to https://codereview.chromium.org/1719873003/
and does for the Dart bindings the same thing that that change did for the Go
bindings.

Our implementation of runtime type information is changing and this CL
implements the change for Dart.

In the older implementation the runtime type structures are computed and
constructed by the code generators (thus in Python and the Jinja 2 templates).
In this newer implementation the Mojom parser computes the information and
stores it in a mojom_types.RuntimeTypeInfo
structure. Then the code generators insert a literal array of bytes representing
a serialized RuntimeTypeInfo into the generated Dart code. At runtime all a Mojo
application needs to do
is deserialize the RuntimeTypeInfo struct and use it to answer queries.

One particular change to note is that like with Go, the new implementation
generates an implementation of getTopLevelInterface() that returns null for an
interface that is not a top-level interface (i.e. is not marked with the
"ServiceName" annotation.)

BUG=#448
==========

[zra, 2016-03-02 17:15:35]

https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart
(right):

https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1324:
return runtimeTypeInfo__;
Change to '_runtimeTypeInfo'

https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1332:
_initRuntimeTypeInfo() {
void _initRuntimeTypeInfo()

https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1336:
var serializedRuntimeTypeInfo = new Uint8List.fromList(const
[24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,48,0,0,0,0,0,0,0,24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,8,0,0,0,0,0,0,0,8,0,0,0,0,0,0,0,24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,216,0,0,0,0,0,0,0,40,0,0,0,4,0,0,0,32,0,0,0,0,0,0,0,64,0,0,0,0,0,0,0,96,0,0,0,0,0,0,0,128,0,0,0,0,0,0,0,36,0,0,0,28,0,0,0,84,89,80,69,95,75,69,89,58,100,97,114,116,95,116,111,95,99,112,112,46,67,112,112,83,105,100,101,0,0,0,0,37,0,0,0,29,0,0,0,84,89,80,69,95,75,69,89,58,100,97,114,116,95,116,111,95,99,112,112,46,68,97,114,116,83,105,100,101,0,0,0,37,0,0,0,29,0,0,0,84,89,80,69,95,75,69,89,58,100,97,114,116,95,116,111,95,99,112,112,46,69,99,104,111,65,114,103,115,0,0,0,41,0,0,0,33,0,0,0,84,89,80,69,95,75,69,89,58,100,97,114,116,95,116,111,95,99,112,112,46,69,99,104,111,65,114,103,115,76,105,115,116,0,0,0,0,0,0,0,72,0,0,0,4,0,0,0,16,0,0,0,3,0,0,0,56,0,0,0,0,0,0,0,16,0,0,0,3,0,0,0,216,10,0,0,0,0,0,0,16,0,0,0,1,0,0,0,184,21,0,0,0,0,0,0,16,0,0,0,1,0,0,0,240,41,0,0,0,0,0,0,32,0,0,0,0,0,0,0,24,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,240,0,0,0,0,0,0,0,72,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,255,255,255,255,48,0,0,0,0,0,0,0,56,0,0,0,0,0,0,0,255,255,255,255,0,0,0,0,72,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,15,0,0,0,7,0,0,0,67,112,112,83,105,100,101,0,27,0,0,0,19,0,0,0,100,97,114,116,95,116,111,95,99,112,112,46,67,112,112,83,105,100,101,0,0,0,0,0,24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,37,0,0,0,10,0,0,0,86,0,0,0,78,0,0,0,47,104,111,109,101,47,114,117,100,111,109,105,110,101,114,47,109,111,106,111,47,115,114,99,47,109,111,106,111,47,100,97,114,116,47,117,110,105,116,116,101,115,116,115,47,101,109,98,101,100,100,101,114,95,116,101,115,116,101,114,47,100,97,114,116,95,116,111,95,99,112,112,46,109,111,106,111,109,0,0,24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,32,0,0,0,0,0,0,0,24,0,0,0,4,0,0,0,1,225,245,5,56,86,76,5,255,224,245,5,0,225,245,5,40,0,0,0,4,0,0,0,32,0,0,0,0,0,0,0,128,3,0,0,0,0,0,0,112,5,0,0,0,0,0,0,96,7,0,0,0,0,0,0,40,0,0,0,0,0,0,0,32,0,0,0,0,0,0,0,232,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1,225,245,5,0,0,0,0,72,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,255,255,255,255,48,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,255,255,255,255,0,0,0,0,48,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,20,0,0,0,12,0,0,0,69,99,104,111,82,101,115,112,111,110,115,101,0,0,0,0,24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,46,0,0,0,2,0,0,0,86,0,0,0,78,0,0,0,47,104,111,109,101,47,114,117,100,111,109,105,110,101,114,47,109,111,106,111,47,115,114,99,47,109,111,106,111,47,100,97,114,116,47,117,110,105,116,116,101,115,116,115,47,101,109,98,101,100,100,101,114,95,116,101,115,116,101,114,47,100,97,114,116,95,116,111,95,99,112,112,46,109,111,106,111,109,0,0,32,0,0,0,0,0,0,0,24,0,0,0,0,0,0,0,232,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
I'd like to understand a little better what these are for. Right now, they're
just being generated for tests in the Mojo tree, but is the plan that generating
these will be common? If so, I don't think it's a good idea to hardcode an array
of bytes into a Dart file like this. I'm not 100% sure, but I think it could
hurt performance for snapshots and startup time. (The Dart code representation
seemed much more compact.) There are lots of 0's in the data, so it seems like
doing something simple like encoding an array of uint64 instead of uint8 could
be a win, but I think it would also be good to consider gzipping this data
before pasting it into the file. (I'm not sure we have the right decoder hooked
up on the Dart side, but we can certainly get that working if it isn't.)

[rudominer, 2016-03-02 18:22:39]

On 2016/03/02 17:15:35, zra wrote:
>
https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/packages/_moj...
> File
> mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart
> (right):
> 
>
https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/packages/_moj...
>
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1324:
> return runtimeTypeInfo__;
> Change to '_runtimeTypeInfo'
> 
>
https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/packages/_moj...
>
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1332:
> _initRuntimeTypeInfo() {
> void _initRuntimeTypeInfo()
> 
>
https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/packages/_moj...
>
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1336:
> var serializedRuntimeTypeInfo = new Uint8List.fromList(const
>
[24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,48,0,0,0,0,0,0,0,24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,8,0,0,0,0,0,0,0,8,0,0,0,0,0,0,0,24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,216,0,0,0,0,0,0,0,40,0,0,0,4,0,0,0,32,0,0,0,0,0,0,0,64,0,0,0,0,0,0,0,96,0,0,0,0,0,0,0,128,0,0,0,0,0,0,0,36,0,0,0,28,0,0,0,84,89,80,69,95,75,69,89,58,100,97,114,116,95,116,111,95,99,112,112,46,67,112,112,83,105,100,101,0,0,0,0,37,0,0,0,29,0,0,0,84,89,80,69,95,75,69,89,58,100,97,114,116,95,116,111,95,99,112,112,46,68,97,114,116,83,105,100,101,0,0,0,37,0,0,0,29,0,0,0,84,89,80,69,95,75,69,89,58,100,97,114,116,95,116,111,95,99,112,112,46,69,99,104,111,65,114,103,115,0,0,0,41,0,0,0,33,0,0,0,84,89,80,69,95,75,69,89,58,100,97,114,116,95,116,111,95,99,112,112,46,69,99,104,111,65,114,103,115,76,105,115,116,0,0,0,0,0,0,0,72,0,0,0,4,0,0,0,16,0,0,0,3,0,0,0,56,0,0,0,0,0,0,0,16,0,0,0,3,0,0,0,216,10,0,0,0,0,0,0,16,0,0,0,1,0,0,0,184,21,0,0,0,0,0,0,16,0,0,0,1,0,0,0,240,41,0,0,0,0,0,0,32,0,0,0,0,0,0,0,24,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,240,0,0,0,0,0,0,0,72,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,255,255,255,255,48,0,0,0,0,0,0,0,56,0,0,0,0,0,0,0,255,255,255,255,0,0,0,0,72,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,15,0,0,0,7,0,0,0,67,112,112,83,105,100,101,0,27,0,0,0,19,0,0,0,100,97,114,116,95,116,111,95,99,112,112,46,67,112,112,83,105,100,101,0,0,0,0,0,24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,37,0,0,0,10,0,0,0,86,0,0,0,78,0,0,0,47,104,111,109,101,47,114,117,100,111,109,105,110,101,114,47,109,111,106,111,47,115,114,99,47,109,111,106,111,47,100,97,114,116,47,117,110,105,116,116,101,115,116,115,47,101,109,98,101,100,100,101,114,95,116,101,115,116,101,114,47,100,97,114,116,95,116,111,95,99,112,112,46,109,111,106,111,109,0,0,24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,32,0,0,0,0,0,0,0,24,0,0,0,4,0,0,0,1,225,245,5,56,86,76,5,255,224,245,5,0,225,245,5,40,0,0,0,4,0,0,0,32,0,0,0,0,0,0,0,128,3,0,0,0,0,0,0,112,5,0,0,0,0,0,0,96,7,0,0,0,0,0,0,40,0,0,0,0,0,0,0,32,0,0,0,0,0,0,0,232,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1,225,245,5,0,0,0,0,72,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,255,255,255,255,48,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,255,255,255,255,0,0,0,0,48,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,20,0,0,0,12,0,0,0,69,99,104,111,82,101,115,112,111,110,115,101,0,0,0,0,24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,46,0,0,0,2,0,0,0,86,0,0,0,78,0,0,0,47,104,111,109,101,47,114,117,100,111,109,105,110,101,114,47,109,111,106,111,47,115,114,99,47,109,111,106,111,47,100,97,114,116,47,117,110,105,116,116,101,115,116,115,47,101,109,98,101,100,100,101,114,95,116,101,115,116,101,114,47,100,97,114,116,95,116,111,95,99,112,112,46,109,111,106,111,109,0,0,32,0,0,0,0,0,0,0,24,0,0,0,0,0,0,0,232,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
> I'd like to understand a little better what these are for. Right now, they're
> just being generated for tests in the Mojo tree, but is the plan that
generating
> these will be common? If so, I don't think it's a good idea to hardcode an
array
> of bytes into a Dart file like this. I'm not 100% sure, but I think it could
> hurt performance for snapshots and startup time. (The Dart code representation
> seemed much more compact.) There are lots of 0's in the data, so it seems like
> doing something simple like encoding an array of uint64 instead of uint8 could
> be a win, but I think it would also be good to consider gzipping this data
> before pasting it into the file. (I'm not sure we have the right decoder
hooked
> up on the Dart side, but we can certainly get that working if it isn't.)

Zach, I completely agree with you and had the same thoughts. This is way too
long. I have several ideas for making it (hopefully much) smaller. One is to
gzip which I guess should help a lot. The other is to remove some of the data
that is not really used. I plan to do that work next.

The only use case I know of for sure at the moment is Vanadium V23 proxy and
Syncbase. The current plan is that these bytes will not be in a generated Dart
file by default--the developer will have to explicitly set a flag in their build
configuration to say he wants it.

So are you comfortable with leaving it as is for a week or so until I can make
it smaller? I will need gzip support in Dart.

[zra, 2016-03-02 18:28:19]

On 2016/03/02 18:22:39, rudominer wrote:
> So are you comfortable with leaving it as is for a week or so until I can make
> it smaller? I will need gzip support in Dart.

sgtm. Thanks!

[azani, 2016-03-02 22:30:53]

lgtm

[rudominer, 2016-03-02 22:46:31]

Patchset #2 (id:30001) has been deleted

[rudominer, 2016-03-02 22:46:50]

Patchset #2 (id:40001) has been deleted

[rudominer, 2016-03-02 22:47:08]

Patchset #2 (id:50001) has been deleted

[rudominer, 2016-03-03 01:06:01]

Patchset #2 (id:60001) has been deleted

[rudominer, 2016-03-03 01:07:01]

Patchset #2 (id:70001) has been deleted

[rudominer, 2016-03-03 01:07:25]

Patchset #2 (id:80001) has been deleted

[rudominer, 2016-03-03 01:09:36]

Zach, AlexF, ptal.

https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/apptests/dart...
File mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart
(left):

https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/apptests/dart...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:57:
_compare(c, d);
On 2016/03/02 05:27:12, alexfandrianto wrote:
> Does this part of the test need to be removed? If possible, I'd like to keep
it
> so that we can verify that getTypeDefinition is implemented correctly.
> 
> If the problem is the type key, we can reuse the computeTypeKey function (and
> TODO) you wrote for the embedder test.
> 
> +// computeTypeKey encapsulates the algorithm for computing the type key
> +// of a type given its fully-qualified name.
> +//
> +// TODO(rudominer) Delete this when we add accessors for Mojom types
> +// in the Dart bindings.
> +String computeTypeKey(String fullyQualifiedName) {
> +  return "TYPE_KEY:" + fullyQualifiedName;
> +}
> +

OK here is what I did: I switched this test to query the PingPongService instead
of the EchoService because the PingPongService uses more than one Mojom type.
Now we can test using getTypeDefinition() the way it is supposed to be used.

https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/packages/_moj...
File
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart
(right):

https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:877:
responseFactory(null);
On 2016/03/02 05:27:12, alexfandrianto wrote:
> Observation: The CppSide interface didn't have a service name; that's why its
> service description now returns null for each method.

Acknowledged.

https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1324:
return runtimeTypeInfo__;
On 2016/03/02 17:15:35, zra wrote:
> Change to '_runtimeTypeInfo'

Done.

https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1332:
_initRuntimeTypeInfo() {
On 2016/03/02 17:15:35, zra wrote:
> void _initRuntimeTypeInfo()

Following a suggestion of alexfandrianto this now has a return value.

https://codereview.chromium.org/1753013002/diff/10002/mojo/dart/packages/_moj...
mojo/dart/packages/_mojo_for_test_only/lib/dart_to_cpp/dart_to_cpp.mojom.dart:1336:
var serializedRuntimeTypeInfo = new Uint8List.fromList(const
[24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,48,0,0,0,0,0,0,0,24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,8,0,0,0,0,0,0,0,8,0,0,0,0,0,0,0,24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,216,0,0,0,0,0,0,0,40,0,0,0,4,0,0,0,32,0,0,0,0,0,0,0,64,0,0,0,0,0,0,0,96,0,0,0,0,0,0,0,128,0,0,0,0,0,0,0,36,0,0,0,28,0,0,0,84,89,80,69,95,75,69,89,58,100,97,114,116,95,116,111,95,99,112,112,46,67,112,112,83,105,100,101,0,0,0,0,37,0,0,0,29,0,0,0,84,89,80,69,95,75,69,89,58,100,97,114,116,95,116,111,95,99,112,112,46,68,97,114,116,83,105,100,101,0,0,0,37,0,0,0,29,0,0,0,84,89,80,69,95,75,69,89,58,100,97,114,116,95,116,111,95,99,112,112,46,69,99,104,111,65,114,103,115,0,0,0,41,0,0,0,33,0,0,0,84,89,80,69,95,75,69,89,58,100,97,114,116,95,116,111,95,99,112,112,46,69,99,104,111,65,114,103,115,76,105,115,116,0,0,0,0,0,0,0,72,0,0,0,4,0,0,0,16,0,0,0,3,0,0,0,56,0,0,0,0,0,0,0,16,0,0,0,3,0,0,0,216,10,0,0,0,0,0,0,16,0,0,0,1,0,0,0,184,21,0,0,0,0,0,0,16,0,0,0,1,0,0,0,240,41,0,0,0,0,0,0,32,0,0,0,0,0,0,0,24,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,240,0,0,0,0,0,0,0,72,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,255,255,255,255,48,0,0,0,0,0,0,0,56,0,0,0,0,0,0,0,255,255,255,255,0,0,0,0,72,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,15,0,0,0,7,0,0,0,67,112,112,83,105,100,101,0,27,0,0,0,19,0,0,0,100,97,114,116,95,116,111,95,99,112,112,46,67,112,112,83,105,100,101,0,0,0,0,0,24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,37,0,0,0,10,0,0,0,86,0,0,0,78,0,0,0,47,104,111,109,101,47,114,117,100,111,109,105,110,101,114,47,109,111,106,111,47,115,114,99,47,109,111,106,111,47,100,97,114,116,47,117,110,105,116,116,101,115,116,115,47,101,109,98,101,100,100,101,114,95,116,101,115,116,101,114,47,100,97,114,116,95,116,111,95,99,112,112,46,109,111,106,111,109,0,0,24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,32,0,0,0,0,0,0,0,24,0,0,0,4,0,0,0,1,225,245,5,56,86,76,5,255,224,245,5,0,225,245,5,40,0,0,0,4,0,0,0,32,0,0,0,0,0,0,0,128,3,0,0,0,0,0,0,112,5,0,0,0,0,0,0,96,7,0,0,0,0,0,0,40,0,0,0,0,0,0,0,32,0,0,0,0,0,0,0,232,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1,225,245,5,0,0,0,0,72,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,255,255,255,255,48,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,255,255,255,255,0,0,0,0,48,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,20,0,0,0,12,0,0,0,69,99,104,111,82,101,115,112,111,110,115,101,0,0,0,0,24,0,0,0,0,0,0,0,16,0,0,0,0,0,0,0,46,0,0,0,2,0,0,0,86,0,0,0,78,0,0,0,47,104,111,109,101,47,114,117,100,111,109,105,110,101,114,47,109,111,106,111,47,115,114,99,47,109,111,106,111,47,100,97,114,116,47,117,110,105,116,116,101,115,116,115,47,101,109,98,101,100,100,101,114,95,116,101,115,116,101,114,47,100,97,114,116,95,116,111,95,99,112,112,46,109,111,106,111,109,0,0,32,0,0,0,0,0,0,0,24,0,0,0,0,0,0,0,232,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
On 2016/03/02 17:15:35, zra wrote:
> I'd like to understand a little better what these are for. Right now, they're
> just being generated for tests in the Mojo tree, but is the plan that
generating
> these will be common? If so, I don't think it's a good idea to hardcode an
array
> of bytes into a Dart file like this. I'm not 100% sure, but I think it could
> hurt performance for snapshots and startup time. (The Dart code representation
> seemed much more compact.) There are lots of 0's in the data, so it seems like
> doing something simple like encoding an array of uint64 instead of uint8 could
> be a win, but I think it would also be good to consider gzipping this data
> before pasting it into the file. (I'm not sure we have the right decoder
hooked
> up on the Dart side, but we can certainly get that working if it isn't.)

Zach, I completely agree with you and had the same thoughts. This is way too
long. I have several ideas for making it (hopefully much) smaller. One is to
gzip which I guess should help a lot. The other is to remove some of the data
that is not really used. I plan to do that work next.

The only use case I know of for sure at the moment is Vanadium V23 proxy and
Syncbase. The current plan is that these bytes will not be in a generated Dart
file by default--the developer will have to explicitly set a flag in their build
configuration to say he wants it.

So are you comfortable with leaving it as is for a week or so until I can make
it smaller? I will need gzip support in Dart.

https://codereview.chromium.org/1753013002/diff/10002/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/1753013002/diff/10002/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/interface_definition.tmpl:12:
{%- if should_gen_mojom_types and interface.service_name %}
On 2016/03/02 05:27:12, alexfandrianto wrote:
> It may be worth noting explicitly in the CL description that, like Go, Dart
> interfaces will only receive the full service description if they also have a
> service name. Doing so helps explain the changes in some of the
> _mojo_for_test_only *.mojom.dart files, where their service description
> implementations are now returning null.

Good suggestion. Done.

https://codereview.chromium.org/1753013002/diff/10002/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl
(right):

https://codereview.chromium.org/1753013002/diff/10002/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/dart_templates/module_definition.tmpl:34:
return runtimeTypeInfo__;
On 2016/03/02 05:27:12, alexfandrianto wrote:
> Optionally, you can rewrite 30 to 35 as
> {{typepkg}}RuntimeTypeInfo getRuntimeTypeInfo() => runtimeTypeInfo ??
> _initRuntimeTypeInfo();
> 
> if you also add 'return runtimeTypeInfo__' to _initRuntimeTypeInfo()

Done.

[rudominer, 2016-03-03 01:27:51]

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
File mojo/dart/apptests/dart_apptests/lib/main.dart (right):

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
mojo/dart/apptests/dart_apptests/lib/main.dart:24: service_describer.tests,
One thing to note about this hack: It appears to be due to my change to the
*test* not my change to the code. In a clean client I can make the same change
to the test and reproduce the intermittent time-out. So while we should get to
the bottom of this, it does not appear to be caused by my changes. The change to
the test I made is to query PingPongService instead of EchoService. Also note
that I found other ways of eliminating the flakiness besides moving this line to
the top of the list. Commenting out either
- both connect_to_loader_apptests and echo
or
- file

Also seemed to work. So there is some race condition somewhere and some
complicated interaction between multiple apptests.

[alexfandrianto, 2016-03-03 02:26:23]

lgtm

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
File mojo/dart/apptests/dart_apptests/lib/main.dart (right):

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
mojo/dart/apptests/dart_apptests/lib/main.dart:24: service_describer.tests,
On 2016/03/03 01:27:51, rudominer wrote:
> One thing to note about this hack: It appears to be due to my change to the
> *test* not my change to the code. In a clean client I can make the same change
> to the test and reproduce the intermittent time-out. So while we should get to
> the bottom of this, it does not appear to be caused by my changes. The change
to
> the test I made is to query PingPongService instead of EchoService. Also note
> that I found other ways of eliminating the flakiness besides moving this line
to
> the top of the list. Commenting out either
> - both connect_to_loader_apptests and echo
> or
> - file
> 
> Also seemed to work. So there is some race condition somewhere and some
> complicated interaction between multiple apptests.

That is very troubling behavior. I wonder what's happening. Is there an issue to
track this?

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
File mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart
(right):

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:77: 
optional: remove

[zra, 2016-03-03 16:52:37]

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
File mojo/dart/apptests/dart_apptests/lib/main.dart (right):

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
mojo/dart/apptests/dart_apptests/lib/main.dart:24: service_describer.tests,
On 2016/03/03 01:27:51, rudominer wrote:
> One thing to note about this hack: It appears to be due to my change to the
> *test* not my change to the code. In a clean client I can make the same change
> to the test and reproduce the intermittent time-out. So while we should get to
> the bottom of this, it does not appear to be caused by my changes. The change
to
> the test I made is to query PingPongService instead of EchoService. Also note
> that I found other ways of eliminating the flakiness besides moving this line
to
> the top of the list. Commenting out either
> - both connect_to_loader_apptests and echo
> or
> - file
> 
> Also seemed to work. So there is some race condition somewhere and some
> complicated interaction between multiple apptests.

Application shutdown can race with a new client requesting a service from the
application that is shutting down. I bet that's what you're hitting here. I've
made a suggestion in the test to help investigate, but if that doesn't shed any
light on things, we can land this CL and file a bug assigned to me.

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
File mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart
(right):

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:32:
await serviceDescriberProxy.ptr
What is the value being dropped here?

This should probably be:

var something = await serviecDescriberProxy.responseOrError(
    serviceDescriberProxy.ptr.describeService(...));

This may give more information about the failure instead of hanging when the
service describer test is in a different place in the list.

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:40: var
sd = serviceDescriptionProxy.ptr;
Dart style is to use more descriptive variable names.

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:46:
mojom_types.MojomInterface a =
ditto

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:48:
mojom_types.MojomInterface b = ed.getTopLevelInterface(identity);
ditto

[rudominer, 2016-03-03 22:46:14]

zra@ ptal at patchset 3. I have responded to your comments.

The race condition in the tests still exists and the efforts at adding extra
diagnostics does not reveal any information.

In patchset 3 I have the troublesome test first in the list and in this order
all tests pass. In patchset 4 I have moved the troublesome test later in the
list to illustrate the trouble.

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
File mojo/dart/apptests/dart_apptests/lib/main.dart (right):

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
mojo/dart/apptests/dart_apptests/lib/main.dart:24: service_describer.tests,
On 2016/03/03 16:52:36, zra wrote:
> On 2016/03/03 01:27:51, rudominer wrote:
> > One thing to note about this hack: It appears to be due to my change to the
> > *test* not my change to the code. In a clean client I can make the same
change
> > to the test and reproduce the intermittent time-out. So while we should get
to
> > the bottom of this, it does not appear to be caused by my changes. The
change
> to
> > the test I made is to query PingPongService instead of EchoService. Also
note
> > that I found other ways of eliminating the flakiness besides moving this
line
> to
> > the top of the list. Commenting out either
> > - both connect_to_loader_apptests and echo
> > or
> > - file
> > 
> > Also seemed to work. So there is some race condition somewhere and some
> > complicated interaction between multiple apptests.
> 
> Application shutdown can race with a new client requesting a service from the
> application that is shutting down. I bet that's what you're hitting here. I've
> made a suggestion in the test to help investigate, but if that doesn't shed
any
> light on things, we can land this CL and file a bug assigned to me.

I have made the changes you suggested over hangouts but the behavior has not
changed and I don't see any more diagnostic info.

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
File mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart
(right):

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:32:
await serviceDescriberProxy.ptr
On 2016/03/03 16:52:36, zra wrote:
> What is the value being dropped here?
> 
> This should probably be:
> 
> var something = await serviecDescriberProxy.responseOrError(
>     serviceDescriberProxy.ptr.describeService(...));
> 
> This may give more information about the failure instead of hanging when the
> service describer test is in a different place in the list.

Done (as modified by our discussion over hangouts.)

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:40: var
sd = serviceDescriptionProxy.ptr;
On 2016/03/03 16:52:36, zra wrote:
> Dart style is to use more descriptive variable names.

Done.

https://codereview.chromium.org/1753013002/diff/90001/mojo/dart/apptests/dart...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:77: 
On 2016/03/03 02:26:23, alexfandrianto wrote:
> optional: remove

Done.

[zra, 2016-03-03 23:25:50]

On 2016/03/03 22:46:14, rudominer wrote:
> zra@ ptal at patchset 3. I have responded to your comments.
> 
> The race condition in the tests still exists and the efforts at adding extra
> diagnostics does not reveal any information.
> 
> In patchset 3 I have the troublesome test first in the list and in this order
> all tests pass. In patchset 4 I have moved the troublesome test later in the
> list to illustrate the trouble.

I patched in the changes to the tests. One more thing to try, and then I think
we'll just have to land this and I'll debug the flake. Could you comment out the
'Quit' test in //mojo/dart/apptests/dart_apptests/lib/src/pingpong_apptests.dart
? It's not actually necessary to shutdown the pingpong app there, and that
should address some of the app shutdown raceyness.

[rudominer, 2016-03-04 02:23:16]

Zach, ptal. As you suggested I commented out the 'Quit' test in
pingpong_apptests.dart and that appears to have eliminated the flakiness of
service_describer_apptests.dart.

[zra, 2016-03-04 15:56:28]

lgtm with nits

In the CL description, if you change 'BUG=#xxx' to 'fixes #xxx' github will
close the issue when you land. If you say 'related #xxx' github will mention the
commit in the comments for issue #xxx.

https://codereview.chromium.org/1753013002/diff/120001/mojo/dart/apptests/dar...
File mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart
(right):

https://codereview.chromium.org/1753013002/diff/120001/mojo/dart/apptests/dar...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:29:
print('There was an error $v'));
This should probably be a 'fail' instead of 'print' now.

https://codereview.chromium.org/1753013002/diff/120001/mojo/dart/apptests/dar...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:35: (v)
=> print('There was an error $v'));
ditto

https://codereview.chromium.org/1753013002/diff/120001/mojo/dart/apptests/dar...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:67:
(await serviceDescription.getTypeDefinition(typeKey)).type.
You can use serviceDescriptionProxy.responseOrError here, too.

https://codereview.chromium.org/1753013002/diff/120001/mojo/dart/apptests/dar...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:74: var
actualDescriptions = (await serviceDescription.
ditto

[rudominer, 2016-03-04 19:09:04]

Description was changed from

==========
Mojom runtime type info: New implementation for Dart.

This is a follow on to https://codereview.chromium.org/1719873003/
and does for the Dart bindings the same thing that that change did for the Go
bindings.

Our implementation of runtime type information is changing and this CL
implements the change for Dart.

In the older implementation the runtime type structures are computed and
constructed by the code generators (thus in Python and the Jinja 2 templates).
In this newer implementation the Mojom parser computes the information and
stores it in a mojom_types.RuntimeTypeInfo
structure. Then the code generators insert a literal array of bytes representing
a serialized RuntimeTypeInfo into the generated Dart code. At runtime all a Mojo
application needs to do
is deserialize the RuntimeTypeInfo struct and use it to answer queries.

One particular change to note is that like with Go, the new implementation
generates an implementation of getTopLevelInterface() that returns null for an
interface that is not a top-level interface (i.e. is not marked with the
"ServiceName" annotation.)

BUG=#448
==========

to

==========
Mojom runtime type info: New implementation for Dart.

This is a follow on to https://codereview.chromium.org/1719873003/
and does for the Dart bindings the same thing that that change did for the Go
bindings.

Our implementation of runtime type information is changing and this CL
implements the change for Dart.

In the older implementation the runtime type structures are computed and
constructed by the code generators (thus in Python and the Jinja 2 templates).
In this newer implementation the Mojom parser computes the information and
stores it in a mojom_types.RuntimeTypeInfo
structure. Then the code generators insert a literal array of bytes representing
a serialized RuntimeTypeInfo into the generated Dart code. At runtime all a Mojo
application needs to do
is deserialize the RuntimeTypeInfo struct and use it to answer queries.

One particular change to note is that like with Go, the new implementation
generates an implementation of getTopLevelInterface() that returns null for an
interface that is not a top-level interface (i.e. is not marked with the
"ServiceName" annotation.)

BUG=#448
R=alexfandrianto@google.com, azani@chromium.org, zra@google.com

Committed:
https://chromium.googlesource.com/external/mojo/+/04e96bfd15364f988b745d76f17...
==========

[rudominer, 2016-03-04 19:09:06]

Committed patchset #9 (id:160001) manually as
04e96bfd15364f988b745d76f17b59e214cb9cc6 (presubmit successful).

[rudominer, 2016-03-04 19:09:18]

https://codereview.chromium.org/1753013002/diff/120001/mojo/dart/apptests/dar...
File mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart
(right):

https://codereview.chromium.org/1753013002/diff/120001/mojo/dart/apptests/dar...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:29:
print('There was an error $v'));
On 2016/03/04 15:56:28, zra wrote:
> This should probably be a 'fail' instead of 'print' now.

Done.

https://codereview.chromium.org/1753013002/diff/120001/mojo/dart/apptests/dar...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:35: (v)
=> print('There was an error $v'));
On 2016/03/04 15:56:28, zra wrote:
> ditto

Done.

https://codereview.chromium.org/1753013002/diff/120001/mojo/dart/apptests/dar...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:67:
(await serviceDescription.getTypeDefinition(typeKey)).type.
On 2016/03/04 15:56:28, zra wrote:
> You can use serviceDescriptionProxy.responseOrError here, too.

Done.

https://codereview.chromium.org/1753013002/diff/120001/mojo/dart/apptests/dar...
mojo/dart/apptests/dart_apptests/lib/src/service_describer_apptests.dart:74: var
actualDescriptions = (await serviceDescription.
On 2016/03/04 15:56:28, zra wrote:
> ditto

Done.