Converted Observatory heap-profile element

Converted Observatory heap-profile element

R=johnmccutchan@google.com

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

[cbernaschina, 2016-08-16 23:34:44]

cbernaschina@google.com changed reviewers:
+ johnmccutchan@google.com, rmacnak@google.com, turnidge@google.com

[Cutch, 2016-08-17 14:28:44]

LGTM with some comments and I'd like to see the sorting aspect of the table
tested.

Nice UI cleanup too!

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/lib...
File runtime/observatory/lib/src/app/page.dart (right):

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/app/page.dart:7: AllocationProfileRepository
_heapProfileRepository
_allocationProfileRepository

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/allocation_profile.dart (right):

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/allocation_profile.dart:109: if
(_autoRefresh && e.isolate.id == _isolate.id) {
(e.isolate.id == _isolate.id)

but I think you can just do (e.isolate == _isolate) because we should be
canonicalizing isolate instances.

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/allocation_profile.dart:272: getter =
(M.ClassHeapStats s) => s.clazz.name;
missing final break

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/containers/virtual_collection.dart
(right):

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:145: if
(_top == null || (top - _top).abs() >= tail_length) {
Our style is to always wrap multiple if expressions in ( )s:

((_top == null) || ((top - _top).abs() >= tail_length))

I know it's not semantically necessary but it helps avoid silly mistakes.

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/tes...
File
runtime/observatory/tests/observatory_ui/allocation_profile/element_test.dart
(right):

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/tes...
runtime/observatory/tests/observatory_ui/allocation_profile/element_test.dart:88:
test('to gc', () async {
what about a test that changes the sort?

[cbernaschina, 2016-08-17 17:01:27]

Fixed layout problem with VirtualCollection header in CpuProfiler

Added sorting tests

Avoid to download CSV during test

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/lib...
File runtime/observatory/lib/src/app/page.dart (right):

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/app/page.dart:7: AllocationProfileRepository
_heapProfileRepository
On 2016/08/17 14:28:44, Cutch wrote:
> _allocationProfileRepository

Done.

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/allocation_profile.dart (right):

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/allocation_profile.dart:109: if
(_autoRefresh && e.isolate.id == _isolate.id) {
On 2016/08/17 14:28:44, Cutch wrote:
> (e.isolate.id == _isolate.id)
> 
> but I think you can just do (e.isolate == _isolate) because we should be
> canonicalizing isolate instances.

Done.
Checking id is meanly for test-ability, in this way during tests I can create
IsolateRefMock and IsolateMock keeping this working.

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/allocation_profile.dart:272: getter =
(M.ClassHeapStats s) => s.clazz.name;
On 2016/08/17 14:28:44, Cutch wrote:
> missing final break

Done.

Should the analyzer signal this?

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/containers/virtual_collection.dart
(right):

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/containers/virtual_collection.dart:145: if
(_top == null || (top - _top).abs() >= tail_length) {
On 2016/08/17 14:28:44, Cutch wrote:
> Our style is to always wrap multiple if expressions in ( )s:
> 
> ((_top == null) || ((top - _top).abs() >= tail_length))
> 
> I know it's not semantically necessary but it helps avoid silly mistakes.

Done.

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/tes...
File
runtime/observatory/tests/observatory_ui/allocation_profile/element_test.dart
(right):

https://codereview.chromium.org/2255613002/diff/60001/runtime/observatory/tes...
runtime/observatory/tests/observatory_ui/allocation_profile/element_test.dart:88:
test('to gc', () async {
On 2016/08/17 14:28:44, Cutch wrote:
> what about a test that changes the sort?

Done.

[Cutch, 2016-08-17 17:35:10]

C++ changes LGTM with my comment addressed.

https://codereview.chromium.org/2255613002/diff/100001/runtime/vm/class_table.cc
File runtime/vm/class_table.cc (right):

https://codereview.chromium.org/2255613002/diff/100001/runtime/vm/class_table...
runtime/vm/class_table.cc:524: if
(isolate->last_allocationprofile_accumulator_reset_timestamp()) {
the type of

isolate->last_allocationprofile_accumulator_reset_timestamp()

is int64_t

Please change this so that the expression is a bool:

isolate->last_allocationprofile_accumulator_reset_timestamp() != 0

https://codereview.chromium.org/2255613002/diff/100001/runtime/vm/class_table...
runtime/vm/class_table.cc:530: if
(isolate->last_allocationprofile_gc_timestamp()) {
ditto

[cbernaschina, 2016-08-17 17:55:52]

https://codereview.chromium.org/2255613002/diff/100001/runtime/vm/class_table.cc
File runtime/vm/class_table.cc (right):

https://codereview.chromium.org/2255613002/diff/100001/runtime/vm/class_table...
runtime/vm/class_table.cc:524: if
(isolate->last_allocationprofile_accumulator_reset_timestamp()) {
On 2016/08/17 17:35:10, Cutch wrote:
> the type of
> 
> isolate->last_allocationprofile_accumulator_reset_timestamp()
> 
> is int64_t
> 
> Please change this so that the expression is a bool:
> 
> isolate->last_allocationprofile_accumulator_reset_timestamp() != 0

Done.

https://codereview.chromium.org/2255613002/diff/100001/runtime/vm/class_table...
runtime/vm/class_table.cc:530: if
(isolate->last_allocationprofile_gc_timestamp()) {
On 2016/08/17 17:35:10, Cutch wrote:
> ditto

Done.

[cbernaschina, 2016-08-17 22:51:48]

Description was changed from

==========
Converted Observatory heap-profile element
==========

to

==========
Converted Observatory heap-profile element

R=johnmccutchan@google.com

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

[cbernaschina, 2016-08-17 22:51:49]

Committed patchset #9 (id:160001) manually as
b0582b3bb7ac5a35726aa2e86fe6a480c8e6db7d (presubmit successful).