Diff heap statistics. #4501
Diff heap statistics. #4501
Conversation
packages/devtools_app/lib/src/screens/memory/panes/diff/controller/diff_pane_controller.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/memory/panes/diff/controller/heap_diff.dart
Outdated
Show resolved
Hide resolved
| if (other.runtimeType != runtimeType) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
this should be captured by the other is _HeapCouple check
There was a problem hiding this comment.
Copied it from https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#common-boilerplates-for-operator--and-hashcode.
I would stay with the standard.
Maybe is is more expensive and we want to exit early?
packages/devtools_app/lib/src/screens/memory/panes/diff/controller/heap_diff.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/memory/panes/diff/widgets/class_details.dart
Show resolved
Hide resolved
| if (!isProcessing && current.heap != null) ...[ | ||
| _DiffDropdown( | ||
| current: current, | ||
| list: controller.snapshots, |
There was a problem hiding this comment.
a nice polish could be to remove current from the diff drop down list, so we only show things you're interested in diffing against (no point in diffing current against current)
There was a problem hiding this comment.
I use 'current' as place holder for the option to not diff the snapshot at all. And I replace the name of snapshot wit '-':
I think it creates a nice visual mapping between list at the left and list in dropdown:
| } else { | ||
| final heap1 = item.heap!; | ||
| final heap2 = item.diffWith.value!.heap!; | ||
| stats = controller.diffStore.compare(heap1, heap2).stats; |
There was a problem hiding this comment.
if this can be expensive, we shouldn't do this in build. If it is expensive, we could do this in the controller before we set isProcessing to false (that way we still show a loading spinner while we are calculating this).
There was a problem hiding this comment.
It is ok for small applications.
And it may be ok for performance, but heavy for memory.
I need to experiment with Gallery.
Added to the issue so that we do not forget: #3949 (comment)
There was a problem hiding this comment.
If this has the potential to slow down the UI at all, we should definitely ship it out to a future and have the UI with a future builder and a spinner or something :)
There was a problem hiding this comment.
ok
Added TODO and item to #3949 (comment)
packages/devtools_app/lib/src/screens/memory/panes/diff/widgets/stats_table.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/memory/panes/diff/widgets/stats_table.dart
Outdated
Show resolved
Hide resolved
| abstract class DiffListItem extends DisposableController { | ||
| /// Number, that, if shown in name, should be unique in the list. | ||
| /// | ||
| /// If the number is not shown, it should be 0. |
There was a problem hiding this comment.
This part of the comment is a bit confusing.
There was a problem hiding this comment.
updated. does it help?
packages/devtools_app/lib/src/screens/memory/panes/diff/controller/Item_controller.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/memory/panes/diff/widgets/class_details.dart
Outdated
Show resolved
Hide resolved
| } else { | ||
| final heap1 = item.heap!; | ||
| final heap2 = item.diffWith.value!.heap!; | ||
| stats = controller.diffStore.compare(heap1, heap2).stats; |
There was a problem hiding this comment.
If this has the potential to slow down the UI at all, we should definitely ship it out to a future and have the UI with a future builder and a spinner or something :)
packages/devtools_app/lib/src/screens/memory/panes/diff/controller/Item_controller.dart
Outdated
Show resolved
Hide resolved
| final _diffWith = ValueNotifier<SnapshotListItem?>(null); | ||
| ValueListenable<SnapshotListItem?> get diffWith => _diffWith; | ||
| void setDiffWith(SnapshotListItem? value) { |
There was a problem hiding this comment.
same comment around ordering. Also, any reason we aren't using a formal setter here?
There was a problem hiding this comment.
fixed ordering
formal setter wants to have the same type as the formal getter with the same name
packages/devtools_app/lib/src/screens/memory/panes/diff/controller/Item_controller.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/memory/panes/diff/controller/Item_controller.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/memory/panes/diff/controller/Item_controller.dart
Outdated
Show resolved
Hide resolved
| @@ -61,6 +62,9 @@ class DiffPaneController { | |||
| } | |||
|
|
|||
| Future<void> clearSnapshots() async { | |||
| for (var i = 0; i < snapshots.value.length; i++) { | |||
There was a problem hiding this comment.
nit: use for (final snapshot in snapshots) to make this less verbose
There was a problem hiding this comment.
The index should start with 1 here. Fixed.
No description provided.