Add additional tables to Memory view for drilling down #14
Conversation
|
Yup, the UI looks great - let me look through the code. |
lib/memory/memory.dart
Outdated
| @@ -293,6 +362,15 @@ class MemoryColumnInstanceCount extends Column<ClassHeapStats> { | |||
| String render(dynamic value) => Column.fastIntl(value); | |||
| } | |||
|
|
|||
| class MemoryColumnSimple<T> extends Column<T> { | |||
| String Function(T) getter; | |||
| MemoryColumnSimple(String name, this.getter, {bool wide = false}) | |||
There was a problem hiding this comment.
Don't need to change now, but Flutter style is that constructors come first. I see that we're not consistent with that in this codebase, so in some future PR we should update the class definitions.
We can look at what the Observatory does here (https://github.com/dart-lang/sdk/blob/03d68355d6ff699bfc9df5b86f8b1f175bd4a88c/runtime/observatory/lib/src/service/object.dart#L1577); I would guess a snapshot in time is what we want. We can always wire that up separately from the UI. For the UI, this is very similar to what I was thinking of. The deltas are that instead of a hard-coded list of tables expanding out to the right, we want the selection in the n-1 table to inform what kind of table to render to its right. So, selecting in the initial table would show a table of all the instances of a class. Selecting in that table would show a table with outbound references (the fields of that instance). Selecting in that one (an object) would show its outbound references to the right. You can then progressively explore the heap. In that setup, I'd imagine the tables are mostly a fixed width, and that they're in a panel that where there are enough contents, will scroll horizontally. |
|
Ok, I changed the tables over to a stack so we can push new tables based on the selected data:
I also added a spinner for async work, though it's possible it doesn't make sense anywhere except waiting for the snapshot (there may not be much async work elsewhere, and things like getObject are very fast). I might look at the data stuff on another branch that targets this one to simplify the PRs/changesets a little. |
There was a problem hiding this comment.
lgtm
A possible future change is to have a ViewManager class - it would handle the stack of Views that extend out to the right. A View subclass would know how in render itself (as a table), and would be provided with an input object - a class type, or an instance. It would listen to its own selection, and then tell the parent ViewManager to create a new child to its right - another View subclass.
In any case, not something for this PR, but more useful when we want to display more kinds of data.
lib/memory/memory.dart
Outdated
| tableContainer.element.scrollTo(<String, dynamic>{ | ||
| 'left': tableContainer.element.scrollWidth, | ||
| 'top': 0, | ||
| 'behavior': 'smooth' |
lib/memory/memory.dart
Outdated
| @@ -102,8 +130,9 @@ class MemoryScreen extends Screen { | |||
| return stats.instancesCurrent > 0; //|| stats.instancesAccumulated > 0; | |||
| }).toList(); | |||
|
|
|||
| memoryTable.setRows(heapStats); | |||
| tableStack.first..setRows(heapStats); | |||
There was a problem hiding this comment.
too many periods (first..setRows())
lib/memory/memory.dart
Outdated
| }); | ||
|
|
||
| // Kick off population of data for the table. | ||
| // TODO: If it turns out not to be async work, remove the spinner. |
lib/memory/memory.dart
Outdated
| @@ -25,7 +26,10 @@ class MemoryScreen extends Screen { | |||
| StatusItem objectCountStatus; | |||
|
|
|||
| PButton loadSnapshotButton; | |||
| Table<ClassHeapStats> memoryTable; | |||
|
|
|||
| // TODO(dantup): Is it reasonable to put dynamic here? | |||
There was a problem hiding this comment.
Object may be more correct here - the common super class of all the things we could display.
DanTup
force-pushed
the
multiple-tables-for-memory
branch
from
5c1a975 to
a7d90b0
Compare
Sounds like a good idea. I think it may be easier to do with a second use of this (it'll be clearer which bits of the behaviour need to be controlled from the outside) so will tackle it then. I've addressed the other comments, and moved constructors to the top for all the classes in this file (I didn't do the whole repo for fear of making lots of conflicts on outstanding PRs/unpushed changes!). Also rebased on master. LMK if you're happy with this being merged while it still has fake data. |
Yes, sounds good. I think we'd still in the prototyping stage. |
@devoncarew This adds multiple tables to the Memory page that appear/disappear based on selection (hopefully this is what you were describing - see below).
Currently it has hard-coded data because I'm not sure how to get the data. I did wire it up to
_getInstancesin the VM then realised that it was supposed to be a snapshot (most things returnedSentinelfetching them live!).I can see there's a
_loadHeapSnapshotfunction but it's commented out, and it only seems to result in a byte array. Is there any info on how this works I can look at? And do we need to be concerned about it being fetched separately to_getAllocationProfile(su the results might not match perfectly)?