Added page to Observatory to display native memory allocation information.

Added page to Observatory to display native memory allocation information.

BUG=
R=johnmccutchan@google.com

Committed: https://github.com/dart-lang/sdk/commit/f30f312b487f88ade32405bd6c9e2e943a5586d1

[bkonyi, 2017-03-16 19:50:56]

bkonyi@google.com changed reviewers:
+ johnmccutchan@google.com, turnidge@google.com, zra@google.com

[bkonyi, 2017-03-16 19:50:57]

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
File runtime/vm/profiler_service.cc (right):

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:1717: // TODO(bkonyi) deal with this
What needs to be done here again John?

[zra, 2017-03-16 20:36:37]

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

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:134: explicit AllocationInfo(uword address, intptr_t
allocation_size)
Don't need 'explicit' when there is more than one argument.

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:151: if (allocation_address != address_) {
Why might this happen?

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:161: Sample* sample_;
It might be worthwhile to make an explicit note here that the AllocationInfo
doesn't own the Sample object.

[Cutch, 2017-03-16 20:38:16]

lgtm

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

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:154: sample_->set_native_allocation_address(0);
what about the size too?

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
File runtime/vm/profiler_service.cc (right):

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:219: static void
AccumulateAllocationHelper(AllocationHashMap* map,
I don't see why this code is necessary?

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:1493:
current->IncrementAllocation(sample->native_allocation_size_bytes(),
Refactor this so it follows the pattern you set elsewhere:

ProfileCodeTrieNode::Tick takes the ProcessedSample and does the call to
IncrementaAllocation.

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:1522:
current->IncrementAllocation(sample->native_allocation_size_bytes(),
ditto

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:1542:
current->IncrementAllocation(sample->native_allocation_size_bytes(),
ditto

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:1569:
current->IncrementAllocation(sample->native_allocation_size_bytes(),
ditto

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:1717: // TODO(bkonyi) deal with this
On 2017/03/16 19:50:57, bkonyi wrote:
> What needs to be done here again John?

You should figure out why the assumption that the code is optimized is no longer
the case :)

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
File runtime/vm/profiler_service.h (right):

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.h:38: typedef intptr_t Key;
what is the key?

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.h:39: typedef intptr_t Value;
what is the value?

[Cutch, 2017-03-16 20:39:01]

Misclicked, NOT LGTM

[bkonyi, 2017-03-16 20:48:40]

Responded to some comments for clarification.

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

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:151: if (allocation_address != address_) {
On 2017/03/16 20:36:37, zra wrote:
> Why might this happen?

The samples are stored in a ring buffer and are eventually overwritten with new
samples. This is just to check if the sample has already been overwritten before
trying to mark it as a freed native allocation sample. Now that I think about it
this might be racy...

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
File runtime/vm/profiler_service.cc (right):

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:219: static void
AccumulateAllocationHelper(AllocationHashMap* map,
On 2017/03/16 20:38:16, Cutch wrote:
> I don't see why this code is necessary?

So, currently, I'm tracking the inclusive/exclusive allocation values in both
the trie and the Profile(Code|Function). The values in the trie are the ones
that are being displayed, but you had mentioned it might be worth keeping the
values in the Profile(Code|Function) objects as well. I meant to leave a comment
asking if we should leave this or not, but I think it might just make more sense
to take it out.

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
File runtime/vm/profiler_service.h (right):

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.h:38: typedef intptr_t Key;
On 2017/03/16 20:38:16, Cutch wrote:
> what is the key?

Size of the allocation. Will add comment/better name if we decide to keep memory
information in the Profile(Code|Function) objects.

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.h:39: typedef intptr_t Value;
On 2017/03/16 20:38:16, Cutch wrote:
> what is the value?

The number of allocations of the size represented by the key. Will add
comment/better name if we decide to keep memory information in the
Profile(Code|Function) objects.

[bkonyi, 2017-03-21 01:53:24]

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

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:134: explicit AllocationInfo(uword address, intptr_t
allocation_size)
On 2017/03/16 20:36:37, zra wrote:
> Don't need 'explicit' when there is more than one argument.

Done.

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:151: if (allocation_address != address_) {
On 2017/03/16 20:48:40, bkonyi wrote:
> On 2017/03/16 20:36:37, zra wrote:
> > Why might this happen?
> 
> The samples are stored in a ring buffer and are eventually overwritten with
new
> samples. This is just to check if the sample has already been overwritten
before
> trying to mark it as a freed native allocation sample. Now that I think about
it
> this might be racy...

I've changed how this is done so that we should be able to avoid races.

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:154: sample_->set_native_allocation_address(0);
On 2017/03/16 20:38:16, Cutch wrote:
> what about the size too?

I've changed how I check if a native allocation sample has been freed or not to
avoid races.

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/malloc_hooks...
runtime/vm/malloc_hooks.cc:161: Sample* sample_;
On 2017/03/16 20:36:37, zra wrote:
> It might be worthwhile to make an explicit note here that the AllocationInfo
> doesn't own the Sample object.

Done.

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
File runtime/vm/profiler_service.cc (right):

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:219: static void
AccumulateAllocationHelper(AllocationHashMap* map,
On 2017/03/16 20:48:40, bkonyi wrote:
> On 2017/03/16 20:38:16, Cutch wrote:
> > I don't see why this code is necessary?
> 
> So, currently, I'm tracking the inclusive/exclusive allocation values in both
> the trie and the Profile(Code|Function). The values in the trie are the ones
> that are being displayed, but you had mentioned it might be worth keeping the
> values in the Profile(Code|Function) objects as well. I meant to leave a
comment
> asking if we should leave this or not, but I think it might just make more
sense
> to take it out.

Removed.

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:1493:
current->IncrementAllocation(sample->native_allocation_size_bytes(),
On 2017/03/16 20:38:16, Cutch wrote:
> Refactor this so it follows the pattern you set elsewhere:
> 
> ProfileCodeTrieNode::Tick takes the ProcessedSample and does the call to
> IncrementaAllocation.

Done.

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:1522:
current->IncrementAllocation(sample->native_allocation_size_bytes(),
On 2017/03/16 20:38:16, Cutch wrote:
> ditto

Done.

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:1542:
current->IncrementAllocation(sample->native_allocation_size_bytes(),
On 2017/03/16 20:38:16, Cutch wrote:
> ditto

Done.

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:1569:
current->IncrementAllocation(sample->native_allocation_size_bytes(),
On 2017/03/16 20:38:16, Cutch wrote:
> ditto

Done.

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:1717: // TODO(bkonyi) deal with this
On 2017/03/16 20:38:16, Cutch wrote:
> On 2017/03/16 19:50:57, bkonyi wrote:
> > What needs to be done here again John?
> 
> You should figure out why the assumption that the code is optimized is no
longer
> the case :)

Acknowledged.

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
File runtime/vm/profiler_service.h (right):

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.h:38: typedef intptr_t Key;
On 2017/03/16 20:48:40, bkonyi wrote:
> On 2017/03/16 20:38:16, Cutch wrote:
> > what is the key?
> 
> Size of the allocation. Will add comment/better name if we decide to keep
memory
> information in the Profile(Code|Function) objects.

Removed.

https://codereview.chromium.org/2748403002/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.h:39: typedef intptr_t Value;
On 2017/03/16 20:48:40, bkonyi wrote:
> On 2017/03/16 20:38:16, Cutch wrote:
> > what is the value?
> 
> The number of allocations of the size represented by the key. Will add
> comment/better name if we decide to keep memory information in the
> Profile(Code|Function) objects.

Removed.

[Cutch, 2017-03-21 20:27:11]

A bunch of comments. Mostly minor.

It would be nice to have a C++ unit test of the profiler trie data.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
File runtime/observatory/lib/src/cpu_profile/cpu_profile.dart (right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:356: if
(root.inclusiveNativeAllocations != 0) {
it looks like inclusiveNativeAllocations be null which would evaluate to true
here.

to be safe you'll want to write:

(root.inclusiveNativeAllocations != null) && (root.inclusiveNativeAllocations !=
0)

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:366: if
(treeFilter.filtered.root.inclusiveNativeAllocations != 0) {
ditto

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:393: void
_setFunctionMemoryPercentage(FunctionCallTreeNode parent, FunctionCallTreeNode
node) {
long line, you probably want to run dartfmt on it.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart (right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart:106: } else {
} else if (type == ProfileTreeType.memory) {
} else {
  throw some error so we catch when we extend the number of types.
}

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart:115: update =
_updateMemoryFunctionRow;
ditto

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart:190:
element.children[3].text = _tree.isExpanded(item) ? '▼' : '►';
factor these strings into named constants and use the named constants below.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart:239: void
_updateMemoryCodeRow(HtmlElement element, M.CodeCallTreeNode item, int depth) {
long line..

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/native_memory_profiler.dart (right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/native_memory_profiler.dart:37:
Stream<RenderedEvent<NativeMemoryProfileElement>> get onRendered =>
_r.onRendered;
long line.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/native_memory_profiler.dart:52: //with
non-isolate version
space between // and text.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
File runtime/observatory/lib/src/repositories/sample_profile.dart (right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/repositories/sample_profile.dart:35: final Object
owner;
Use the type ServiceObjectOwner instead of Object (here and elsewhere)

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/repositories/sample_profile.dart:39: final bool
isCpuProfile;
isn't there an enum for the profile type? Use that instead of the bool here to
be more generic in the future.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
File runtime/observatory/lib/src/service/object.dart (right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/service/object.dart:792: if ((id != null) && (obj !=
null) && obj.canCache) {
we shouldn't put things in the VM objects' cache here.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/get_native_allocation_samples_test.dart
(right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/get_native_allocation_samples_test.dart:33:
rootMemory = root.inclusiveNativeAllocations - root.exclusiveNativeAllocations;
long line

https://codereview.chromium.org/2748403002/diff/40001/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/vm/profiler.h#n...
runtime/vm/profiler.h:591: native_allocation_sample_ = native_allocation_sample;
do you need this as a separate bool? Couldn't this be derived from _bytes_ != 0?

https://codereview.chromium.org/2748403002/diff/40001/runtime/vm/profiler_ser...
File runtime/vm/profiler_service.cc (right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:1894: ProcessedSample* sample) {
sample doesn't appear used here?

https://codereview.chromium.org/2748403002/diff/40001/runtime/vm/profiler_ser...
File runtime/vm/profiler_service.h (right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.h:94: return contains_native_allocation_;
is this used anywhere? why is it needed?

[bkonyi, 2017-03-22 21:25:21]

I think this covers pretty much everything except for the C++ test and that
assertion that was failing before due to what looks like an incorrect
assumption.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
File runtime/observatory/lib/src/cpu_profile/cpu_profile.dart (right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:356: if
(root.inclusiveNativeAllocations != 0) {
On 2017/03/21 20:27:10, Cutch wrote:
> it looks like inclusiveNativeAllocations be null which would evaluate to true
> here.
> 
> to be safe you'll want to write:
> 
> (root.inclusiveNativeAllocations != null) && (root.inclusiveNativeAllocations
!=
> 0)

Done.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:366: if
(treeFilter.filtered.root.inclusiveNativeAllocations != 0) {
On 2017/03/21 20:27:10, Cutch wrote:
> ditto

Done.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:393: void
_setFunctionMemoryPercentage(FunctionCallTreeNode parent, FunctionCallTreeNode
node) {
On 2017/03/21 20:27:10, Cutch wrote:
> long line, you probably want to run dartfmt on it.

Done.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart (right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart:106: } else {
On 2017/03/21 20:27:10, Cutch wrote:
> } else if (type == ProfileTreeType.memory) {
> } else {
>   throw some error so we catch when we extend the number of types.
> }

Done in the check above so we don't have to do it here and below.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart:115: update =
_updateMemoryFunctionRow;
On 2017/03/21 20:27:10, Cutch wrote:
> ditto

See above response.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart:239: void
_updateMemoryCodeRow(HtmlElement element, M.CodeCallTreeNode item, int depth) {
On 2017/03/21 20:27:10, Cutch wrote:
> long line..

Done.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/native_memory_profiler.dart (right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/native_memory_profiler.dart:37:
Stream<RenderedEvent<NativeMemoryProfileElement>> get onRendered =>
_r.onRendered;
On 2017/03/21 20:27:10, Cutch wrote:
> long line.

Done.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/native_memory_profiler.dart:52: //with
non-isolate version
On 2017/03/21 20:27:11, Cutch wrote:
> space between // and text.

Done.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
File runtime/observatory/lib/src/repositories/sample_profile.dart (right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/repositories/sample_profile.dart:35: final Object
owner;
On 2017/03/21 20:27:11, Cutch wrote:
> Use the type ServiceObjectOwner instead of Object (here and elsewhere)

Done.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/repositories/sample_profile.dart:39: final bool
isCpuProfile;
On 2017/03/21 20:27:11, Cutch wrote:
> isn't there an enum for the profile type? Use that instead of the bool here to
> be more generic in the future.

Done.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
File runtime/observatory/lib/src/service/object.dart (right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/lib...
runtime/observatory/lib/src/service/object.dart:792: if ((id != null) && (obj !=
null) && obj.canCache) {
On 2017/03/21 20:27:11, Cutch wrote:
> we shouldn't put things in the VM objects' cache here.

Done.

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/get_native_allocation_samples_test.dart
(right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/get_native_allocation_samples_test.dart:33:
rootMemory = root.inclusiveNativeAllocations - root.exclusiveNativeAllocations;
On 2017/03/21 20:27:11, Cutch wrote:
> long line

Done.

https://codereview.chromium.org/2748403002/diff/40001/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/vm/profiler.h#n...
runtime/vm/profiler.h:591: native_allocation_sample_ = native_allocation_sample;
On 2017/03/21 20:27:11, Cutch wrote:
> do you need this as a separate bool? Couldn't this be derived from _bytes_ !=
0?

Done.

https://codereview.chromium.org/2748403002/diff/40001/runtime/vm/profiler_ser...
File runtime/vm/profiler_service.cc (right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:1894: ProcessedSample* sample) {
On 2017/03/21 20:27:11, Cutch wrote:
> sample doesn't appear used here?

Removed.

https://codereview.chromium.org/2748403002/diff/40001/runtime/vm/profiler_ser...
File runtime/vm/profiler_service.h (right):

https://codereview.chromium.org/2748403002/diff/40001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.h:94: return contains_native_allocation_;
On 2017/03/21 20:27:11, Cutch wrote:
> is this used anywhere? why is it needed?

Apparently not. Removed.

[bkonyi, 2017-03-23 23:13:48]

Alright, I think this should be it if the new profiler test is sufficient.

[Cutch, 2017-03-23 23:39:45]

LGTM after addressing my comments.

I think you may have some bugs where you're using inclusive allocations where
you should be using exclusive allocations.

A bunch of style nits.

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/cpu_profile/cpu_profile.dart (right):

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:100: node._percentage =
(node.inclusiveNativeAllocations / parentMemory);
should this be exclusiveNativeAllocations?

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:361:
root.inclusiveNativeAllocations != null) {
test for null before testing against 0.

wrap each in parens.

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:371: if
(treeFilter.filtered.root.inclusiveNativeAllocations != 0 &&
ditto

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:412: node._percentage =
(node.inclusiveNativeAllocations / parentMemory);
should this be exclusiveNativeAllocations?

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:845: if (owner != null
&& owner is Isolate) {
here and everywhere: wrap in parens

((owner != null) && (owner is Isolate))

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/code_ref.dart (right):

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/code_ref.dart:57: href:
(M.isSyntheticCode(_code.kind) || _isolate == null)
wrap in parens.

... || (_isolate == null)

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart (right):

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart:248:
element.children[3].text = _tree.isExpanded(item) ? '▼' : '►';
make these arrows into const strings and reference the const strings instead of
duplicating them.

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/stack_trace_tree_config.dart (right):

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/stack_trace_tree_config.dart:12: enum
ProfileTreeType { cpu, memory, }
Why are there two enums that define the same keys?

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/models/repositories/sample_profile.dart
(right):

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/models/repositories/sample_profile.dart:11: enum
SampleProfileType { cpu, memory }
Why are there two enums that define the same keys?

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/repositories/sample_profile.dart (right):

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/repositories/sample_profile.dart:179: if (_last !=
null && !forceFetch) {
ditto

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/tes...
File runtime/observatory/tests/service/get_native_allocation_samples_test.dart
(right):

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/tes...
runtime/observatory/tests/service/get_native_allocation_samples_test.dart:22:
print('${root.profileFunction.function.name}');
remove commented out code

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

https://codereview.chromium.org/2748403002/diff/80001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:1469: sample->native_allocation_size_bytes());
why don't you forward the address to?

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

https://codereview.chromium.org/2748403002/diff/80001/runtime/vm/profiler.h#n...
runtime/vm/profiler.h:418: if (sample->is_native_allocation_sample()) {
I prefer:

if (!sample->is_native_allocation_sample()) {
  return false;
}

...

but feel free to leave this as is.

[bkonyi, 2017-03-24 00:53:39]

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/cpu_profile/cpu_profile.dart (right):

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:100: node._percentage =
(node.inclusiveNativeAllocations / parentMemory);
On 2017/03/23 23:39:44, Cutch wrote:
> should this be exclusiveNativeAllocations?

No, since the variable "inclusive" represents the direction of the tree, keeping
consistent with the flags used in Observatory for "Top Down" and "Bottom Up"

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:361:
root.inclusiveNativeAllocations != null) {
On 2017/03/23 23:39:44, Cutch wrote:
> test for null before testing against 0.
> 
> wrap each in parens.

Done.

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:371: if
(treeFilter.filtered.root.inclusiveNativeAllocations != 0 &&
On 2017/03/23 23:39:44, Cutch wrote:
> ditto

Done.

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:412: node._percentage =
(node.inclusiveNativeAllocations / parentMemory);
On 2017/03/23 23:39:44, Cutch wrote:
> should this be exclusiveNativeAllocations?

See reply above.

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:845: if (owner != null
&& owner is Isolate) {
On 2017/03/23 23:39:44, Cutch wrote:
> here and everywhere: wrap in parens
> 
> ((owner != null) && (owner is Isolate))

Done.

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/code_ref.dart (right):

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/code_ref.dart:57: href:
(M.isSyntheticCode(_code.kind) || _isolate == null)
On 2017/03/23 23:39:44, Cutch wrote:
> wrap in parens.
> 
> ... || (_isolate == null)

Done.

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart (right):

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/cpu_profile/virtual_tree.dart:248:
element.children[3].text = _tree.isExpanded(item) ? '▼' : '►';
On 2017/03/23 23:39:44, Cutch wrote:
> make these arrows into const strings and reference the const strings instead
of
> duplicating them.

Done.

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/stack_trace_tree_config.dart (right):

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/stack_trace_tree_config.dart:12: enum
ProfileTreeType { cpu, memory, }
On 2017/03/23 23:39:44, Cutch wrote:
> Why are there two enums that define the same keys?

Huh, somehow I completely forgot I had added them in sample_profile.dart...
Removed.

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/models/repositories/sample_profile.dart
(right):

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/models/repositories/sample_profile.dart:11: enum
SampleProfileType { cpu, memory }
On 2017/03/23 23:39:45, Cutch wrote:
> Why are there two enums that define the same keys?

Completely forgot about this definition. Just going to use this one.

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
File runtime/observatory/lib/src/repositories/sample_profile.dart (right):

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/lib...
runtime/observatory/lib/src/repositories/sample_profile.dart:179: if (_last !=
null && !forceFetch) {
On 2017/03/23 23:39:45, Cutch wrote:
> ditto

Done.

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/tes...
File runtime/observatory/tests/service/get_native_allocation_samples_test.dart
(right):

https://codereview.chromium.org/2748403002/diff/80001/runtime/observatory/tes...
runtime/observatory/tests/service/get_native_allocation_samples_test.dart:22:
print('${root.profileFunction.function.name}');
On 2017/03/23 23:39:45, Cutch wrote:
> remove commented out code

Done.

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

https://codereview.chromium.org/2748403002/diff/80001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:1469: sample->native_allocation_size_bytes());
On 2017/03/23 23:39:45, Cutch wrote:
> why don't you forward the address to?

It's not sent to the Observatory to be displayed since it's not really useful,
so there really isn't much of a point forwarding it, is there?

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

https://codereview.chromium.org/2748403002/diff/80001/runtime/vm/profiler.h#n...
runtime/vm/profiler.h:418: if (sample->is_native_allocation_sample()) {
On 2017/03/23 23:39:45, Cutch wrote:
> I prefer:
> 
> if (!sample->is_native_allocation_sample()) {
>   return false;
> }
> 
> ...
> 
> but feel free to leave this as is.

Done.

[bkonyi, 2017-03-24 16:20:33]

Description was changed from

==========
Added page to Observatory to display native memory allocation information.

BUG=
==========

to

==========
Added page to Observatory to display native memory allocation information.

BUG=
R=johnmccutchan@google.com

Committed:
https://github.com/dart-lang/sdk/commit/f30f312b487f88ade32405bd6c9e2e943a5586d1
==========

[bkonyi, 2017-03-24 16:20:35]

Committed patchset #6 (id:100001) manually as
f30f312b487f88ade32405bd6c9e2e943a5586d1 (presubmit successful).