Treat null like the object it is in the Observatory and Service.

Treat null like the object it is in the Observatory and Service.

BUG=http://dartbug.com/20649
R=turnidge@google.com

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

[rmacnak, 2014-08-26 21:36:51]

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

[rmacnak, 2014-08-26 21:36:51]

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

https://codereview.chromium.org/501583007/diff/80001/runtime/vm/object_id_rin...
runtime/vm/object_id_ring.h:22: enum LookupResult {
Not thrilled about this name.

[turnidge, 2014-08-27 16:13:56]

https://codereview.chromium.org/501583007/diff/80001/runtime/bin/vmservice/ob...
File runtime/bin/vmservice/observatory/lib/src/elements/instance_ref.html
(right):

https://codereview.chromium.org/501583007/diff/80001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/src/elements/instance_ref.html:29:
<template if="{{ isPseduoNull(ref) }}">
Pseduo is tpyed worng.

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

https://codereview.chromium.org/501583007/diff/80001/runtime/vm/object_id_rin...
runtime/vm/object_id_ring.cc:31: ASSERT(!object->IsNullOrPseudoNull());
Technically we could allow null to be inserted now that we have LookupResult...
but it's still better not to let it in.

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

https://codereview.chromium.org/501583007/diff/80001/runtime/vm/object_id_rin...
runtime/vm/object_id_ring.h:22: enum LookupResult {
On 2014/08/26 21:36:50, rmacnak wrote:
> Not thrilled about this name.

Sure.  I'm having trouble coming up with an improvement though.  Maybe if
GetObjectForId were called "Lookup" then the type name would feel better.  Not
necessarily suggesting that.

Maybe add one-line comments for kInvalid, kCollected, and kExpired.

https://codereview.chromium.org/501583007/diff/80001/runtime/vm/object_id_rin...
runtime/vm/object_id_ring.h:39: // Returns Object::sentinel() when the id is not
valid.
This comment is now out of date with the code... but I'm wondering if we should
change the code to return Object::sentinel() whenever the LookupResult is not
kValid?  Or something like that -- always the same result in those three cases.

https://codereview.chromium.org/501583007/diff/80001/runtime/vm/object_id_rin...
File runtime/vm/object_id_ring_test.cc (right):

https://codereview.chromium.org/501583007/diff/80001/runtime/vm/object_id_rin...
runtime/vm/object_id_ring_test.cc:54:
ObjectIdRingTestHelper::ExpectString(ring->GetObjectForId(id, &kind), "1");
Can you add checks that kind is the right value?

EXPECT_EQ(kWhatever, kind);

It would also be good to have at least one test which returns kInvalid,
kExpired, and kCollected.

Instead of forcing an object to be *really* gc'ed, you could add a test-only
function on the id ring which fakes an object being collected.

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

https://codereview.chromium.org/501583007/diff/80001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:382: }
I missed where this is used... can you tell me?

[rmacnak, 2014-08-27 18:09:59]

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

https://codereview.chromium.org/501583007/diff/80001/runtime/vm/object_id_rin...
runtime/vm/object_id_ring.cc:31: ASSERT(!object->IsNullOrPseudoNull());
On 2014/08/27 16:13:56, turnidge wrote:
> Technically we could allow null to be inserted now that we have
LookupResult...
> but it's still better not to let it in.

No, the regular null is how we know an entry was collected because the GC places
a regular null.

Narrowed this to disallowing only regular null based on returning regular null
for !kValid lookups.

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

https://codereview.chromium.org/501583007/diff/80001/runtime/vm/object_id_rin...
runtime/vm/object_id_ring.h:22: enum LookupResult {
On 2014/08/27 16:13:56, turnidge wrote:
> On 2014/08/26 21:36:50, rmacnak wrote:
> > Not thrilled about this name.
> 
> Sure.  I'm having trouble coming up with an improvement though.  Maybe if
> GetObjectForId were called "Lookup" then the type name would feel better.  Not
> necessarily suggesting that.
> 
> Maybe add one-line comments for kInvalid, kCollected, and kExpired.

Added one-liners.

https://codereview.chromium.org/501583007/diff/80001/runtime/vm/object_id_rin...
runtime/vm/object_id_ring.h:39: // Returns Object::sentinel() when the id is not
valid.
On 2014/08/27 16:13:56, turnidge wrote:
> This comment is now out of date with the code... but I'm wondering if we
should
> change the code to return Object::sentinel() whenever the LookupResult is not
> kValid?  Or something like that -- always the same result in those three
cases.

Updated to say we return Object::null() when !kValid.

https://codereview.chromium.org/501583007/diff/80001/runtime/vm/object_id_rin...
File runtime/vm/object_id_ring_test.cc (right):

https://codereview.chromium.org/501583007/diff/80001/runtime/vm/object_id_rin...
runtime/vm/object_id_ring_test.cc:54:
ObjectIdRingTestHelper::ExpectString(ring->GetObjectForId(id, &kind), "1");
On 2014/08/27 16:13:56, turnidge wrote:
> Can you add checks that kind is the right value?

Added

> EXPECT_EQ(kWhatever, kind);
> 
> It would also be good to have at least one test which returns kInvalid,
> kExpired, and kCollected.
> 
> Instead of forcing an object to be *really* gc'ed, you could add a test-only
> function on the id ring which fakes an object being collected.

One of the existing test already checks the GC'd case, so only added one for the
expired case. (kInvalid happens in service.cc)

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

https://codereview.chromium.org/501583007/diff/80001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:382: }
On 2014/08/27 16:13:56, turnidge wrote:
> I missed where this is used... can you tell me?

Was used in guard for values not to insert in the ring. Removed based on
narrowed restriction..

[turnidge, 2014-08-27 18:24:07]

lgtm

[rmacnak, 2014-08-27 19:36:42]

Committed patchset #9 (id:150001) manually as 39616 (presubmit successful).