Add Service ID zones to service protocol

Add Service ID zones to service protocol

- Add ServiceIdZone interface for getting temporary ids.
- Add RingServiceIdZone which uses the isolate's object id ring.
- Add GrowableServiceIdZone which uses a growable array.
- Unit tests for ServiceIdZones.
- JSONStream has a service id zone.
- Default service id zone is ring with eager id allocation.
- All service RPCs can include a _serviceIdZone parameter.
- Value of _serviceIdZone can be Ring.NewId or Ring.ExistingId.
- All ObjectIdRing usage in object.cc has been replaced with `jsobj.AddServiceId("id", *this);`
- Add a policy for id reuse to ObjectIdRing::GetIdForObject
- ObjectIdRing can dump its contents to JSON.
- Add _dumpRingRequests RPC which dumps the isolate's ring.
- _getCrashDump includes complete object id ring.

R=turnidge@google.com

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

[Cutch, 2015-05-11 14:16:18]

johnmccutchan@google.com changed reviewers:
+ turnidge@google.com

[turnidge, 2015-05-11 19:48:45]

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

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/json_stream....
runtime/vm/json_stream.h:139: RingServiceIdZone ring_service_id_zone_;
Call this default_service_id_zone?  Also, stray space before semicolon.

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

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:1599: jsobj.AddServiceId("id", *this);
This is nicer.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:6803: static void SetFunctionServiceId(const JSONObject&
jsobj,
AddFunctionServiceId or AddFunctionServiceIdProperty?

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
File runtime/vm/object_id_ring.cc (right):

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.cc:81: void
ObjectIdRing::PrintGetObjectRequestsToJSON(JSONStream* js) {
Maybe PrintContentsToJSON or DumpToJSON?

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.cc:87: jsobj.AddProperty("type",
"_ObjectIdRingRequests");
I don't like exposing the name "Ring" in the response.  Too implementation
specific.  What is this?  The contents of a zone?  So how about "_ZoneContents"
or "_IdZoneContents" or "_ServiceIdZoneContents"?

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.cc:88: const char* request_format =
"getObject?objectId=%s";
Currently this returns a map from (request) => (response).  I would like to see
this instead be an rpc that returns the contents of a zone, so it feels like
returning a map from (id) => (response) would be better.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.cc:90: JSONObject ring_requests(&jsobj,
"ringRequests");
In almost everyplace else in our protocol, the property names are fixed strings.
 In this case, we are using the request string as the property name.  I would
like this data to be structured differently so that it is easier to describe the
response in the IDL.

You currently return:

  ringRequests: { "getObject?objectId=objects/0" : "BLAH",
"getObject?objectId=objects/1" : "BLAH2" }

Instead I'm suggesting...

Either two parallel arrays, one of ids and one of responses

   {ids: ["objects/0", "objects/1"] responses: {"BLAH", "BLAH2"]}

Or one array of json objects,

   contents: [ { id: "objects/0", response: "BLAH" }, { id: "objects/1",
response: "BLAH2" } ]

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
File runtime/vm/object_id_ring.h (right):

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.h:31: kNewId,             // Always allocate a new
object id.
kAllocateId?

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.h:32: kExistingOrNewId,   // If the object is already
in the ring, reuse id.
kReuseId?

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.h:65: int32_t* ids_;
I think we may be able to drop the ids_ array.  Let's discuss offline.

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

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:70: GrowableServiceIdZone::GrowableServiceIdZone(const
char* format_str,
format_str is perhaps too general, given that we will eventually need to
parse/dispatch these ids.  Directory format, as discussed offline?

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:523:
existing_ring_service_id_zone(ObjectIdRing::kExistingOrNewId);
Having this on the stack feels wrong, somehow.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:534: if (strcmp("Ring.NewId", service_id_zone_param) == 0)
{
Name this ring "default"?

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:537: } else if (strcmp("Ring.ExistingId",
service_id_zone_param) == 0) {
Name this ring "default.reuse"?  Not sure.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:818: new BoolParameter("_full", false),
Update the idl if necessary.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:890: static bool _DumpRingRequests(Isolate* isolate,
JSONStream* js) {
Is using a leading underscore a thing in our C++?  We should probably drop these
underscores.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:2657: { "_dumpRingRequests", _DumpRingRequests, NULL },
I would instead like this request to be "_dumpZone" or "_dumpZoneContents".  It
should dump the contents of whatever zone is provided as an argument.  That way
people can eventually dump their custom zones with the same rpc.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service/vmse...
File runtime/vm/service/vmservice.dart (right):

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service/vmse...
runtime/vm/service/vmservice.dart:188: new
Message.forIsolate(Uri.parse('_dumpRingRequests'), isolate);
We shouldn't leak the word "Ring" here.

Maybe _dumpRingRequests -> _dumpZone?

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service/vmse...
runtime/vm/service/vmservice.dart:191:
response['result']['ringRequests'].forEach((k, v) {
We shouldn't use the word "ring" here in "ringRequests".  It leaks
implementation details that aren't needed by the protocol user.

[turnidge, 2015-05-11 20:01:04]

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

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/json_stream....
runtime/vm/json_stream.h:139: RingServiceIdZone ring_service_id_zone_;
On 2015/05/11 19:48:45, turnidge wrote:
> Call this default_service_id_zone?  Also, stray space before semicolon.

Or default_id_zone.

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

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:544: PrintInvalidParamError(&js, "_serviceIdZone");
_idZone instead?  service is implied.  Here are elsewhere.

[Cutch, 2015-05-12 14:59:58]

PTAL

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

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/json_stream....
runtime/vm/json_stream.h:139: RingServiceIdZone ring_service_id_zone_;
On 2015/05/11 20:01:04, turnidge wrote:
> On 2015/05/11 19:48:45, turnidge wrote:
> > Call this default_service_id_zone?  Also, stray space before semicolon.
> 
> Or default_id_zone.

Done.

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

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:6803: static void SetFunctionServiceId(const JSONObject&
jsobj,
On 2015/05/11 19:48:45, turnidge wrote:
> AddFunctionServiceId or AddFunctionServiceIdProperty?

Done.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
File runtime/vm/object_id_ring.cc (right):

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.cc:81: void
ObjectIdRing::PrintGetObjectRequestsToJSON(JSONStream* js) {
On 2015/05/11 19:48:45, turnidge wrote:
> Maybe PrintContentsToJSON or DumpToJSON?

::PrintJSON

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.cc:87: jsobj.AddProperty("type",
"_ObjectIdRingRequests");
On 2015/05/11 19:48:45, turnidge wrote:
> I don't like exposing the name "Ring" in the response.  Too implementation
> specific.  What is this?  The contents of a zone?  So how about
"_ZoneContents"
> or "_IdZoneContents" or "_ServiceIdZoneContents"?

Acknowledged.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.cc:88: const char* request_format =
"getObject?objectId=%s";
On 2015/05/11 19:48:45, turnidge wrote:
> Currently this returns a map from (request) => (response).  I would like to
see
> this instead be an rpc that returns the contents of a zone, so it feels like
> returning a map from (id) => (response) would be better.

Acknowledged.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.cc:90: JSONObject ring_requests(&jsobj,
"ringRequests");
On 2015/05/11 19:48:45, turnidge wrote:
> In almost everyplace else in our protocol, the property names are fixed
strings.
>  In this case, we are using the request string as the property name.  I would
> like this data to be structured differently so that it is easier to describe
the
> response in the IDL.
> 
> You currently return:
> 
>   ringRequests: { "getObject?objectId=objects/0" : "BLAH",
> "getObject?objectId=objects/1" : "BLAH2" }
> 
> Instead I'm suggesting...
> 
> Either two parallel arrays, one of ids and one of responses
> 
>    {ids: ["objects/0", "objects/1"] responses: {"BLAH", "BLAH2"]}
> 
> Or one array of json objects,
> 
>    contents: [ { id: "objects/0", response: "BLAH" }, { id: "objects/1",
> response: "BLAH2" } ]
> 

Acknowledged.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
File runtime/vm/object_id_ring.h (right):

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.h:31: kNewId,             // Always allocate a new
object id.
On 2015/05/11 19:48:45, turnidge wrote:
> kAllocateId?

Done.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.h:32: kExistingOrNewId,   // If the object is already
in the ring, reuse id.
On 2015/05/11 19:48:45, turnidge wrote:
> kReuseId?

Done.

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

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:70: GrowableServiceIdZone::GrowableServiceIdZone(const
char* format_str,
On 2015/05/11 19:48:45, turnidge wrote:
> format_str is perhaps too general, given that we will eventually need to
> parse/dispatch these ids.  Directory format, as discussed offline?

Acknowledged.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:523:
existing_ring_service_id_zone(ObjectIdRing::kExistingOrNewId);
On 2015/05/11 19:48:45, turnidge wrote:
> Having this on the stack feels wrong, somehow.

Done.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:534: if (strcmp("Ring.NewId", service_id_zone_param) == 0)
{
On 2015/05/11 19:48:45, turnidge wrote:
> Name this ring "default"?

Done.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:537: } else if (strcmp("Ring.ExistingId",
service_id_zone_param) == 0) {
On 2015/05/11 19:48:45, turnidge wrote:
> Name this ring "default.reuse"?  Not sure.

Done.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:544: PrintInvalidParamError(&js, "_serviceIdZone");
On 2015/05/11 20:01:04, turnidge wrote:
> _idZone instead?  service is implied.  Here are elsewhere.

Done.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:818: new BoolParameter("_full", false),
On 2015/05/11 19:48:45, turnidge wrote:
> Update the idl if necessary.

Done.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:890: static bool _DumpRingRequests(Isolate* isolate,
JSONStream* js) {
On 2015/05/11 19:48:45, turnidge wrote:
> Is using a leading underscore a thing in our C++?  We should probably drop
these
> underscores.

Done.

https://codereview.chromium.org/1132323002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:2657: { "_dumpRingRequests", _DumpRingRequests, NULL },
On 2015/05/11 19:48:45, turnidge wrote:
> I would instead like this request to be "_dumpZone" or "_dumpZoneContents". 
It
> should dump the contents of whatever zone is provided as an argument.  That
way
> people can eventually dump their custom zones with the same rpc.

Done.

[turnidge, 2015-05-12 20:08:01]

https://codereview.chromium.org/1132323002/diff/80001/runtime/vm/json_stream.h
File runtime/vm/json_stream.h (right):

https://codereview.chromium.org/1132323002/diff/80001/runtime/vm/json_stream....
runtime/vm/json_stream.h:24: class RingIdZone;
Is this forward declaration right?  I see "RingServiceIdZone" below.

https://codereview.chromium.org/1132323002/diff/80001/runtime/vm/object_id_ri...
File runtime/vm/object_id_ring.cc (right):

https://codereview.chromium.org/1132323002/diff/80001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.cc:86: jsobj.AddProperty("name", "ObjectIdRing");
Should the name be "default"?

https://codereview.chromium.org/1132323002/diff/80001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.cc:97: }
How does the user figure out what the id's of the objects in the dump are?  I
would think that we need to emit the id along with each object...

https://codereview.chromium.org/1132323002/diff/80001/runtime/vm/service.h
File runtime/vm/service.h (right):

https://codereview.chromium.org/1132323002/diff/80001/runtime/vm/service.h#ne...
runtime/vm/service.h:43: explicit RingServiceIdZone(ObjectIdRing::IdPolicy
policy);
Why not pass the object id ring into the constructor?

[turnidge, 2015-05-12 20:13:53]

lgtm

[turnidge, 2015-05-12 20:13:55]

lgtm

[Cutch, 2015-05-12 23:32:59]

https://codereview.chromium.org/1132323002/diff/80001/runtime/vm/json_stream.h
File runtime/vm/json_stream.h (right):

https://codereview.chromium.org/1132323002/diff/80001/runtime/vm/json_stream....
runtime/vm/json_stream.h:24: class RingIdZone;
On 2015/05/12 20:08:01, turnidge wrote:
> Is this forward declaration right?  I see "RingServiceIdZone" below.

Done.

https://codereview.chromium.org/1132323002/diff/80001/runtime/vm/object_id_ri...
File runtime/vm/object_id_ring.cc (right):

https://codereview.chromium.org/1132323002/diff/80001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.cc:86: jsobj.AddProperty("name", "ObjectIdRing");
On 2015/05/12 20:08:01, turnidge wrote:
> Should the name be "default"?

Done.

https://codereview.chromium.org/1132323002/diff/80001/runtime/vm/object_id_ri...
runtime/vm/object_id_ring.cc:97: }
On 2015/05/12 20:08:01, turnidge wrote:
> How does the user figure out what the id's of the objects in the dump are?  I
> would think that we need to emit the id along with each object...

Each object printed to the objects array includes an "id" property. No need to
include it twice.

https://codereview.chromium.org/1132323002/diff/80001/runtime/vm/service.h
File runtime/vm/service.h (right):

https://codereview.chromium.org/1132323002/diff/80001/runtime/vm/service.h#ne...
runtime/vm/service.h:43: explicit RingServiceIdZone(ObjectIdRing::IdPolicy
policy);
On 2015/05/12 20:08:01, turnidge wrote:
> Why not pass the object id ring into the constructor?

Done.

[Cutch, 2015-05-12 23:48:33]

Committed patchset #7 (id:120001) manually as 45754 (presubmit successful).