Cleanup native, collected, and stub code handling in profiler

Cleanup native, collected, and stub code handling in profiler

R=iposva@google.com, turnidge@google.com

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

[turnidge, 2014-02-28 19:23:36]

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

https://codereview.chromium.org/182703003/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4861: printf("Function given null name.\n");
Debug printf?

https://codereview.chromium.org/182703003/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4864: printf("Function name is not a symbol.\n");
Debug printf?

https://codereview.chromium.org/182703003/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:10349: if (OwnerIsNotNullFunction()) {
I don't care for negations in predicate names unless it is a performance issue. 
Can we get away with something like this:

if (OwnerIsFunction()) {
  ...
  if (func.IsNull() {
    // Regular stub. 
  } else {
  }
}

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

https://codereview.chromium.org/182703003/diff/1/runtime/vm/object.h#newcode3301
runtime/vm/object.h:3301: void set_owner_function(const Function& function)
const {
Consider camel-case for this function and the one below, as they are less like
straight setters.  Your call.

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

https://codereview.chromium.org/182703003/diff/1/runtime/vm/raw_object.h#newc...
runtime/vm/raw_object.h:819: RawObject* owner_;  // Function, Null, or a smi
(class id).
I think we need a comment which explains the three cases and when they occur.

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

https://codereview.chromium.org/182703003/diff/1/runtime/vm/service.cc#newcod...
runtime/vm/service.cc:1062: jsobj.AddProperty("type", "NativeCode");
The references to native and collected code have type "@Code" not
"@NativeCode/@CollectedCode"  so we should return "Code" as the type here for
consistency.

Actually, we usually have the case that the object reference (@Foo) returns a
subset of the response for the full object (Foo).  These cases don't follow this
pattern, as the NativeCode and CollectedCode responses contain less info than
the original references.  (The exception are null-like references like
objects/being-initilized which have null responses).

Is it possible to make these return the same info as the references?  Or is that
info not available?  Will these references be links in the Observatory UI?  If
not, it might make sense to handle one or both of them like we handle null-like
references like "objects/being-initialized" and return a null object.

Happy to discuss.

https://codereview.chromium.org/182703003/diff/1/runtime/vm/service.cc#newcod...
runtime/vm/service.cc:1101: }
Can you extend the unit test to cover some of these new cases?

[Cutch, 2014-02-28 19:37:08]

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

https://codereview.chromium.org/182703003/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4861: printf("Function given null name.\n");
On 2014/02/28 19:23:37, turnidge wrote:
> Debug printf?

Done.

https://codereview.chromium.org/182703003/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4864: printf("Function name is not a symbol.\n");
On 2014/02/28 19:23:37, turnidge wrote:
> Debug printf?

Done.

https://codereview.chromium.org/182703003/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:10349: if (OwnerIsNotNullFunction()) {
On 2014/02/28 19:23:37, turnidge wrote:
> I don't care for negations in predicate names unless it is a performance
issue. 
> Can we get away with something like this:
> 
> if (OwnerIsFunction()) {
>   ...
>   if (func.IsNull() {
>     // Regular stub. 
>   } else {
>   }
> }

Done.

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

https://codereview.chromium.org/182703003/diff/1/runtime/vm/raw_object.h#newc...
runtime/vm/raw_object.h:819: RawObject* owner_;  // Function, Null, or a smi
(class id).
On 2014/02/28 19:23:37, turnidge wrote:
> I think we need a comment which explains the three cases and when they occur.

Done.

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

https://codereview.chromium.org/182703003/diff/1/runtime/vm/service.cc#newcod...
runtime/vm/service.cc:1062: jsobj.AddProperty("type", "NativeCode");
On 2014/02/28 19:23:37, turnidge wrote:
> The references to native and collected code have type "@Code" not
> "@NativeCode/@CollectedCode"  so we should return "Code" as the type here for
> consistency.
> 
> Actually, we usually have the case that the object reference (@Foo) returns a
> subset of the response for the full object (Foo).  These cases don't follow
this
> pattern, as the NativeCode and CollectedCode responses contain less info than
> the original references.  (The exception are null-like references like
> objects/being-initilized which have null responses).
> 
> Is it possible to make these return the same info as the references?  Or is
that
> info not available?  Will these references be links in the Observatory UI?  If
> not, it might make sense to handle one or both of them like we handle
null-like
> references like "objects/being-initialized" and return a null object.
> 
> Happy to discuss.

Done.

https://codereview.chromium.org/182703003/diff/1/runtime/vm/service.cc#newcod...
runtime/vm/service.cc:1101: }
On 2014/02/28 19:23:37, turnidge wrote:
> Can you extend the unit test to cover some of these new cases?

Done.

[turnidge, 2014-02-28 19:45:43]

lgtm w/ minor comments.

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

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:822: // Else, owner_ is a regular Dart Function.
Can we put the comment above the declaration and get rid of the same-line
comment?

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

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1091: }
Since this is simpler now, you could drop the helper functions (HandleBlahCode)
and combine the two cases.

[Cutch, 2014-02-28 19:48:08]

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

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:822: // Else, owner_ is a regular Dart Function.
On 2014/02/28 19:45:43, turnidge wrote:
> Can we put the comment above the declaration and get rid of the same-line
> comment?

Done.

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

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1091: }
On 2014/02/28 19:45:43, turnidge wrote:
> Since this is simpler now, you could drop the helper functions
(HandleBlahCode)
> and combine the two cases.

Done.

[Ivan Posva, 2014-02-28 21:46:32]

LGTM with comments.

-Ivan

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

https://codereview.chromium.org/182703003/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4531: value.set_owner_function(*this);
How about just value.set_owner(*this)? The set_owner function knows what to do
with this anyway.

https://codereview.chromium.org/182703003/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4860: if (value.IsNull()) {
Debugging code...

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

https://codereview.chromium.org/182703003/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:10343: if (OwnerIsNotNullFunction()) {
How about just adding Code::Name() and Code::UserName()?

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

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:4531: value.set_owner_function(*this);
I still think this should be called set_owner.

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:10335: RawString* Code::Name() const {
If we stored the Class object instead of the cid as part of the owner field,
then we could just dispatch on the obejct's type here.

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/object.h#newc...
runtime/vm/object.h:3278: bool OwnerIsFunction() const {
IsFunction, IsStub, IsAllocationStub

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:579: code ^= Code::LookupCodeInVmIsolate(pc);
Why don't you move this up before the CodeContains call?

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

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/service.cc#ne...
runtime/vm/service.cc:1081: if (!strcmp("collected", command)) {
strcmp does not return a bool.

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/stub_code.cc
File runtime/vm/stub_code.cc (right):

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/stub_code.cc#...
runtime/vm/stub_code.cc:108:
stub.set_owner_allocation_stub_class_id(Smi::New(cls.id()));
If we switch to owner_ being a Class object or Function object, then we can just
have set_owner.

[Cutch, 2014-02-28 22:47:04]

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

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:4531: value.set_owner_function(*this);
On 2014/02/28 21:46:35, Ivan Posva wrote:
> I still think this should be called set_owner.

Done.

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:10335: RawString* Code::Name() const {
On 2014/02/28 21:46:35, Ivan Posva wrote:
> If we stored the Class object instead of the cid as part of the owner field,
> then we could just dispatch on the obejct's type here.

Done.

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/object.h#newc...
runtime/vm/object.h:3278: bool OwnerIsFunction() const {
On 2014/02/28 21:46:35, Ivan Posva wrote:
> IsFunction, IsStub, IsAllocationStub

Done.

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/182703003/diff/60001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:579: code ^= Code::LookupCodeInVmIsolate(pc);
On 2014/02/28 21:46:35, Ivan Posva wrote:
> Why don't you move this up before the CodeContains call?

Done.

[Cutch, 2014-02-28 22:56:18]

Committed patchset #6 manually as r33177 (presubmit successful).