Add 'address' vm service message that finds heap objects.

Add 'address' vm service message that finds heap objects.

Will be used to navigate from heap map to individual objects.

R=turnidge@google.com

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

[turnidge, 2014-03-20 17:01:19]

lgtm w/ comments.

nice change.

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/heap.cc#newco...
runtime/vm/heap.cc:196: }
This code does get tested by service_test.cc, but you might consider adding a
more direct unit test in heap_test.cc.  Up to you.

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/scavenger.cc
File runtime/vm/scavenger.cc (right):

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/scavenger.cc#...
runtime/vm/scavenger.cc:689: }
Ditto testing comment from heap.cc.  Up to you.

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

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1460: virtual uword filter_addr() const { return addr_; }
Is this accessor used?

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1486: object.PrintToJSONStream(js, true);
Printing the null object isn't the same as printing the null instance (perhaps
it should be).  Consider checking for null and using Object::null_instance()
like we do at the end of HandleClassesEval.

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/service_test.cc
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/service_test....
runtime/vm/service_test.cc:1048: EXPECT_SUBSTRING("\"type\":\"null\"",
handler.msg());
This result will look a bit different once you use the null_instance() in
service.cc.

[turnidge, 2014-03-20 17:08:25]

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

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1486: object.PrintToJSONStream(js, true);
On 2014/03/20 17:01:19, turnidge wrote:
> Printing the null object isn't the same as printing the null instance (perhaps
> it should be).  Consider checking for null and using Object::null_instance()
> like we do at the end of HandleClassesEval.

Actually returning the null instance here would be wrong, wouldn't it?  What's
really going on is that the object has been moved or collected if we can't find
it.

Look for PrintPseudoNull in this file -- we use it to tag objects that are
expired (handle out of date) or collected.  Not sure what to mark these -- maybe
expired.

[koda, 2014-03-20 18:36:26]

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

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1486: object.PrintToJSONStream(js, true);
On 2014/03/20 17:08:25, turnidge wrote:
> On 2014/03/20 17:01:19, turnidge wrote:
> > Printing the null object isn't the same as printing the null instance
(perhaps
> > it should be).  Consider checking for null and using Object::null_instance()
> > like we do at the end of HandleClassesEval.
> 
> Actually returning the null instance here would be wrong, wouldn't it?  What's
> really going on is that the object has been moved or collected if we can't
find
> it.
> 
> Look for PrintPseudoNull in this file -- we use it to tag objects that are
> expired (handle out of date) or collected.  Not sure what to mark these --
maybe
> expired.

Note that we might find *any* heap object, not just subclasses of Instance (for
example, we could find RawInstructions or even a FreeListElement).

Also, note that the queried address might *never* have been occupied by any heap
object, so neither "expired" nor "collected" feels entirely appropriate.

Therefore, I chose Object::null(). Let me know what you think.

[koda, 2014-03-20 20:25:37]

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/heap.cc
File runtime/vm/heap.cc (right):

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/heap.cc#newco...
runtime/vm/heap.cc:196: }
On 2014/03/20 17:01:19, turnidge wrote:
> This code does get tested by service_test.cc, but you might consider adding a
> more direct unit test in heap_test.cc.  Up to you.

Done.

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/scavenger.cc
File runtime/vm/scavenger.cc (right):

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/scavenger.cc#...
runtime/vm/scavenger.cc:689: }
On 2014/03/20 17:01:19, turnidge wrote:
> Ditto testing comment from heap.cc.  Up to you.

I think it's sufficient to test it via heap_test. We actually don't have a
scavenger_test yet...

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

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1460: virtual uword filter_addr() const { return addr_; }
On 2014/03/20 17:01:19, turnidge wrote:
> Is this accessor used?

Yes, it's called by the super class (FindObjectVisitor::VisitRange).

[turnidge, 2014-03-20 20:34:54]

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

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1486: object.PrintToJSONStream(js, true);
On 2014/03/20 18:36:26, koda wrote:
> On 2014/03/20 17:08:25, turnidge wrote:
> > On 2014/03/20 17:01:19, turnidge wrote:
> > > Printing the null object isn't the same as printing the null instance
> (perhaps
> > > it should be).  Consider checking for null and using
Object::null_instance()
> > > like we do at the end of HandleClassesEval.
> > 
> > Actually returning the null instance here would be wrong, wouldn't it? 
What's
> > really going on is that the object has been moved or collected if we can't
> find
> > it.
> > 
> > Look for PrintPseudoNull in this file -- we use it to tag objects that are
> > expired (handle out of date) or collected.  Not sure what to mark these --
> maybe
> > expired.
> 
> Note that we might find *any* heap object, not just subclasses of Instance
(for
> example, we could find RawInstructions or even a FreeListElement).
> 
> Also, note that the queried address might *never* have been occupied by any
heap
> object, so neither "expired" nor "collected" feels entirely appropriate.
> 
> Therefore, I chose Object::null(). Let me know what you think.

I guess it depends on what the UI expects.  We have a general convention that
all json responses from the vmservice are tagged with a type -- I think this
would be the only exception, which makes me pause.

One option is to make a new PseudoNull type -- something like "notfound".

If you want to leave it as "null" for now, go ahead.  We can change it later if
we need to.

[koda, 2014-03-20 20:44:19]

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

https://codereview.chromium.org/206583003/diff/30001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1486: object.PrintToJSONStream(js, true);
On 2014/03/20 20:34:54, turnidge wrote:
> On 2014/03/20 18:36:26, koda wrote:
> > On 2014/03/20 17:08:25, turnidge wrote:
> > > On 2014/03/20 17:01:19, turnidge wrote:
> > > > Printing the null object isn't the same as printing the null instance
> > (perhaps
> > > > it should be).  Consider checking for null and using
> Object::null_instance()
> > > > like we do at the end of HandleClassesEval.
> > > 
> > > Actually returning the null instance here would be wrong, wouldn't it? 
> What's
> > > really going on is that the object has been moved or collected if we can't
> > find
> > > it.
> > > 
> > > Look for PrintPseudoNull in this file -- we use it to tag objects that are
> > > expired (handle out of date) or collected.  Not sure what to mark these --
> > maybe
> > > expired.
> > 
> > Note that we might find *any* heap object, not just subclasses of Instance
> (for
> > example, we could find RawInstructions or even a FreeListElement).
> > 
> > Also, note that the queried address might *never* have been occupied by any
> heap
> > object, so neither "expired" nor "collected" feels entirely appropriate.
> > 
> > Therefore, I chose Object::null(). Let me know what you think.
> 
> I guess it depends on what the UI expects.  We have a general convention that
> all json responses from the vmservice are tagged with a type -- I think this
> would be the only exception, which makes me pause.
> 
> One option is to make a new PseudoNull type -- something like "notfound".
> 
> If you want to leave it as "null" for now, go ahead.  We can change it later
if
> we need to.

For most objects, obj.PrintToJSONStream does tag the response with a type (my
unit test checks for '@String' and 'null').

It's true that some internal VM classes (e.g., Stackmap) just add an empty
object to the stream. We should probably fix that, and make it a requirement
that any override of PrintToJSONStream at least populates type and size.

[koda, 2014-03-20 21:08:28]

Committed patchset #5 manually as r34206 (presubmit successful).