Support displaying of types in the observatory (back-end only for now):

Support displaying of types in the observatory (back-end only for now):
- canonical type arguments cached in the global hash table
- subset of the above, but with cached instantiations
- canonical types cached by the type class

And of course, it should now be possible to display any type, e.g.
field types, function signature types, classes super types, interface types,
etc...

R=johnmccutchan@google.com

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

[regis, 2014-03-06 19:09:07]

John, Todd,

This is not ready for review or submission, but since you had questions about
support for types in the observatory, you can have a look and let me know what
you think.

Still missing:
- tests in service_test.cc
- support in front-end (maybe in a later cl)

I will also remove the duplicate checking code and submit it with a fix for type
ref hashes in a separate cl.

Thanks,
Regis

[Ivan Posva, 2014-03-11 08:02:31]

Regis,

What is the status of this change? Is it still the plan to go ahead with it like
this or have there been different ideas?

Thanks,
-Ivan

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

https://codereview.chromium.org/184653003/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:935: jsobj.AddPropertyF("canonicalTypeArguments",
"isolates/%" Pd "/typearguments",
Is this ("isolates/%" Pd "/...", main_port()) really the best way to express ids
within the current isolate()?

[Cutch, 2014-03-11 13:50:00]

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

https://codereview.chromium.org/184653003/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:935: jsobj.AddPropertyF("canonicalTypeArguments",
"isolates/%" Pd "/typearguments",
It's not necessary (and a bug) to include the isolate ID in an object's ID. 
Also, you should send this as a proper reference (which requires a type and an
id field). e.g:

{
  JSONObject obj(&jsobj, "canonicalTypeArguments");
  typeargRef.AddProperty("type", "@TypeList");
  typeargRef.AddProperty("id", "typearguments");
}

https://codereview.chromium.org/184653003/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:938:
jsobj.AddPropertyF("canonicalTypeArgumentsWithCachedInstantiations",
Same here.

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

https://codereview.chromium.org/184653003/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:13248: id = ring->GetIdForObject(raw());
How frequently does this path happen?

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

https://codereview.chromium.org/184653003/diff/1/runtime/vm/service.cc#newcod...
runtime/vm/service.cc:1222: "members with instantiations" : "members";
This should always be "members". Also, please do not use spaces in key names.

https://codereview.chromium.org/184653003/diff/1/runtime/vm/service.cc#newcod...
runtime/vm/service.cc:1283: { "typeargumentswithinstantiations",
HandleTypeArgumentsWithInstantiations },
Does it make sense to make the "withinstantiations" a sub-command of
typearguments?

[regis, 2014-03-14 23:52:07]

John,

Thanks for the comments. I addressed them today while taking a break from my
other cl.

The uploaded version contains canonicalization fixes that I will submit in a
different cl.

So this is still not ready for review, but feel free to have another look
anyway.

Thanks,
Regis

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

https://codereview.chromium.org/184653003/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:935: jsobj.AddPropertyF("canonicalTypeArguments",
"isolates/%" Pd "/typearguments",
On 2014/03/11 13:50:01, Cutch wrote:
> It's not necessary (and a bug) to include the isolate ID in an object's ID. 
> Also, you should send this as a proper reference (which requires a type and an
> id field). e.g:
> 
> {
>   JSONObject obj(&jsobj, "canonicalTypeArguments");
>   typeargRef.AddProperty("type", "@TypeList");
>   typeargRef.AddProperty("id", "typearguments");
> }

Done.

https://codereview.chromium.org/184653003/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:938:
jsobj.AddPropertyF("canonicalTypeArgumentsWithCachedInstantiations",
On 2014/03/11 13:50:01, Cutch wrote:
> Same here.

Done.

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

https://codereview.chromium.org/184653003/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:13248: id = ring->GetIdForObject(raw());
On 2014/03/11 13:50:01, Cutch wrote:
> How frequently does this path happen?

If I call PrintToJSON on all canonicalized type arguments on isolate shutdown
while running the tests, 58 out 11481 tests will take this path. So, no, this is
not frequent. However, this is a bug in canonicalization that I will fix in a
separate cl.

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

https://codereview.chromium.org/184653003/diff/1/runtime/vm/service.cc#newcod...
runtime/vm/service.cc:1222: "members with instantiations" : "members";
On 2014/03/11 13:50:01, Cutch wrote:
> This should always be "members". Also, please do not use spaces in key names.

Done.

https://codereview.chromium.org/184653003/diff/1/runtime/vm/service.cc#newcod...
runtime/vm/service.cc:1283: { "typeargumentswithinstantiations",
HandleTypeArgumentsWithInstantiations },
On 2014/03/11 13:50:01, Cutch wrote:
> Does it make sense to make the "withinstantiations" a sub-command of
> typearguments?

Good idea. Done.

[regis, 2014-03-19 21:16:47]

John, Todd,

This is now ready for review.

Thanks,
Regis

[Cutch, 2014-03-19 23:13:30]

lgtm after name and user_name are added (see my comments)

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

https://codereview.chromium.org/184653003/diff/40001/runtime/vm/object.cc#new...
runtime/vm/object.cc:3931: typesRef.AddPropertyF("id", "classes/%" Pd "/types",
id());
Need to add a name and user_name here.

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

https://codereview.chromium.org/184653003/diff/40001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1522: PrintError(js, "Must specify collection object id:
/typearguments/id");
Objects in the ring can go away when the ring wraps around. Consider sending
back some null object instead of an error here.

[regis, 2014-03-19 23:47:41]

Thanks. PTAL

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

https://codereview.chromium.org/184653003/diff/40001/runtime/vm/object.cc#new...
runtime/vm/object.cc:3931: typesRef.AddPropertyF("id", "classes/%" Pd "/types",
id());
On 2014/03/19 23:13:31, Cutch wrote:
> Need to add a name and user_name here.

Done here and also for @TypeArgumentsList in isolate.cc.

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

https://codereview.chromium.org/184653003/diff/40001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1522: PrintError(js, "Must specify collection object id:
/typearguments/id");
On 2014/03/19 23:13:31, Cutch wrote:
> Objects in the ring can go away when the ring wraps around. Consider sending
> back some null object instead of an error here.

The ring is not involved here.
The id is the index in the global table of type arguments.
There are 2 ways to request a canonicalized type argument vector:
objects/42
typearguments/42

The first one corresponds to the id printed when the type argument vector was
listed.

The second one is only to poke inside the global table. This is actually not an
id, but an index that may change over time.

This is the reason I added a comment on line 1520. I could remove this code.

[regis, 2014-03-20 16:42:05]

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