Closed Bug 960776 Opened 10 years ago Closed 9 years ago

heap snapshot view

Categories

(DevTools :: Memory, defect)

Product:
DevTools
Component:
Memory
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox44 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: fitzgen, Assigned: jsantell)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

960776-heap-view.patch
69.43 KB, patch
jsantell
: review+
Details | Diff | Splinter Review 
We should be able to take a heap snapshot from the UI and get all of the data in bug 960765 but *also* we should be able to inspect individual object state and references to other objects as they were at the time of the snapshot.
Depends on: 961323
Nick, is this ticket for the full snapshot for the entire heap or is this a snapshot in time off the live memory tool?
Flags: needinfo?(nfitzgerald)
The full snapshot of the entire heap.
Flags: needinfo?(nfitzgerald)
Depends on: 1024774
No longer depends on: 961323
Depends on: 1213137
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
4 "preset" breakdowns for this view (may not implement them all this patch, but for reference):

[11:53pm] fitzgen: coarse type => objects and strings by allocation site, others by internal type
[11:53pm] fitzgen: allocation site => count
[11:53pm] fitzgen: object class => count
[11:54pm] fitzgen: if show-platform-data is enabled, internalType => count
Depends on: 1214872
Attached patch 960776-heap-view.patch (obsolete) — — Splinter Review 
First pass at the tree. Needs a lot of styling follow ups.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cd9dc4e670c
Attachment #8677161 - Flags: review?(nfitzgerald)
Comment on attachment 8677161 [details] [diff] [review]
960776-heap-view.patch

Review of attachment 8677161 [details] [diff] [review]:
-----------------------------------------------------------------

Wasn't as bad as you made it seem like it would be over IRC.

The tree widget really needs tests. It is the core of this tool's UI. Especially if this is being groomed as the One True Tree Widget. Really don't want to end up in a situation where everything is really solid all the way from the platform up through the RDP and the workers and analyses except things break down in the very last step when displaying the data. Only as strong as the weakest link mumble mumble.

Also, stripDynamicProps is pretty smelly to me. I think that unless the IDs are nondeterministic due to ordering of tests (which I think is not the case given the new test here) then we should just modify the EXPECTEDs to include the IDs.

::: devtools/client/memory/components/tree.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

This file should not be in devtools/client/memory, more like devtools/client/shared if we have such a directory.

This tree view is the foundation of our tool's UI: it really deserves some unit tests.

@@ +396,5 @@
> +    });
> +  },
> +
> +  /**
> +   * TODO FITZGEN

All this stuff needs doc/usage comments.

::: devtools/shared/heapsnapshot/tests/unit/head_heapsnapshot.js
@@ +161,5 @@
> +  delete obj.parent;
> +  if (obj.children) {
> +    obj.children.forEach(stripDynamicProps);
> +  }
> +  return obj;

Does the order that the tests run affect the id each node gets because the ids are global rather than per-census? Or are you just trying to avoid adding an id to each test's EXPECTED? If the latter, I'd prefer to add the id to each test's EXPECTED.

Regardless, a good rule of thumb: either modify the object and don't return anything, or do not modify the object and return a new copy. Don't modify the in-param and return it (implies that it is a new object, when it isn't; sets people up for footgun'ing).

::: devtools/shared/heapsnapshot/tests/unit/test_census-tree-node-04.js
@@ +12,5 @@
> +  let func = children.find(x => x.id === 5);
> +  let array = children.find(x => x.id === 6);
> +
> +  ok(id === 1, "real root has an id");
> +  ok(other.parent === id, "root nodes use the real root as parent ID");

"children of root use the root's id and their parent id"
Attachment #8677161 - Flags: review?(nfitzgerald)
Comment on attachment 8677161 [details] [diff] [review]
960776-heap-view.patch

Review of attachment 8677161 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/memory/components/tree.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

Until it's used by something else, I'd rather this not be in shared, as this isn't the shared tree widget yet

@@ +396,5 @@
> +    });
> +  },
> +
> +  /**
> +   * TODO FITZGEN

Do any of these TODO mean anything?

::: devtools/shared/heapsnapshot/tests/unit/head_heapsnapshot.js
@@ +161,5 @@
> +  delete obj.parent;
> +  if (obj.children) {
> +    obj.children.forEach(stripDynamicProps);
> +  }
> +  return obj;

Yeah mostly a quick hack around. I think IDs are fresh for each test (xpcshell anyway) so the IDs are most likely deterministic surprisingly
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #6)
> Comment on attachment 8677161 [details] [diff] [review]
> 960776-heap-view.patch
> 
> Review of attachment 8677161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/memory/components/tree.js
> @@ +1,3 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> Until it's used by something else, I'd rather this not be in shared, as this
> isn't the shared tree widget yet
> 
> @@ +396,5 @@
> > +    });
> > +  },
> > +
> > +  /**
> > +   * TODO FITZGEN
> 
> Do any of these TODO mean anything?

They are placeholders for doc/usage comments.
Comment on attachment 8678269 [details] [diff] [review]
960776-heap-view.patch

Review of attachment 8678269 [details] [diff] [review]:
-----------------------------------------------------------------

I think you will need to rebase on top latest m-c.

Thanks for the tests, those are really nice.
Attachment #8678269 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8678269 [details] [diff] [review]
960776-heap-view.patch

Review of attachment 8678269 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/shared/heapsnapshot/census-tree-node.js
@@ -207,5 @@
>    }
>  
> -  // Loop through each frame in the stack and get or create a CensusTreeNode for
> -  // the frame.
> -

Why delete this comment?

@@ +390,5 @@
>    this.totalBytes = 0;
>    this.count = 0;
>    this.totalCount = 0;
>    this.children = undefined;
> +  this.id = INC++;

Set `this.parent = undefined;` here too.
Depends on: 1217979
Backed out for XPCShell failures:
https://hg.mozilla.org/integration/fx-team/rev/9e60fa0fc1e3

Failed checkin: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=335ea7a95492

19:25:09     INFO -  TEST-START | devtools/shared/heapsnapshot/tests/unit/test_census-tree-node-01.js
19:25:09  WARNING -  TEST-UNEXPECTED-FAIL | devtools/shared/heapsnapshot/tests/unit/test_census-tree-node-01.js | xpcshell return code: 0
19:25:09     INFO -  TEST-INFO took 550ms
19:25:09     INFO -  >>>>>>>
19:25:09     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
19:25:09     INFO -  PROCESS | 11760 | HEAPSNAPSHOT-TEST: Generating CensusTreeNode from report:
19:25:09     INFO -  PROCESS | 11760 | HEAPSNAPSHOT-TEST: breakdown: {
19:25:09     INFO -  PROCESS | 11760 |     "by": "internalType",
19:25:09     INFO -  PROCESS | 11760 |     "then": {
19:25:09     INFO -  PROCESS | 11760 |         "by": "count",
19:25:09     INFO -  PROCESS | 11760 |         "count": true,
19:25:09     INFO -  PROCESS | 11760 |         "bytes": true
19:25:09     INFO -  PROCESS | 11760 |     }
19:25:09     INFO -  PROCESS | 11760 | }
19:25:09     INFO -  PROCESS | 11760 | HEAPSNAPSHOT-TEST: report: {
19:25:09     INFO -  PROCESS | 11760 |     "JSObject": {
19:25:09     INFO -  PROCESS | 11760 |         "bytes": 100,
19:25:09     INFO -  PROCESS | 11760 |         "count": 10
19:25:09     INFO -  PROCESS | 11760 |     },
19:25:09     INFO -  PROCESS | 11760 |     "js::Shape": {
19:25:09     INFO -  PROCESS | 11760 |         "bytes": 500,
19:25:09     INFO -  PROCESS | 11760 |         "count": 50
19:25:09     INFO -  PROCESS | 11760 |     },
19:25:09     INFO -  PROCESS | 11760 |     "JSString": {
19:25:09     INFO -  PROCESS | 11760 |         "bytes": 0,
19:25:09     INFO -  PROCESS | 11760 |         "count": 0
19:25:09     INFO -  PROCESS | 11760 |     }
19:25:09     INFO -  PROCESS | 11760 | }
19:25:09     INFO -  PROCESS | 11760 | HEAPSNAPSHOT-TEST: expected: {
19:25:09     INFO -  PROCESS | 11760 |     "name": null,
19:25:09     INFO -  PROCESS | 11760 |     "bytes": 0,
19:25:09     INFO -  PROCESS | 11760 |     "totalBytes": 600,
19:25:09     INFO -  PROCESS | 11760 |     "count": 0,
19:25:09     INFO -  PROCESS | 11760 |     "totalCount": 60,
19:25:09     INFO -  PROCESS | 11760 |     "children": [
19:25:09     INFO -  PROCESS | 11760 |         {
19:25:09     INFO -  PROCESS | 11760 |             "name": "js::Shape",
19:25:09     INFO -  PROCESS | 11760 |             "bytes": 500,
19:25:09     INFO -  PROCESS | 11760 |             "totalBytes": 500,
19:25:09     INFO -  PROCESS | 11760 |             "count": 50,
19:25:09     INFO -  PROCESS | 11760 |             "totalCount": 50,
19:25:09     INFO -  PROCESS | 11760 |             "id": 2,
19:25:09     INFO -  PROCESS | 11760 |             "parent": 1
19:25:09     INFO -  PROCESS | 11760 |         },
19:25:09     INFO -  PROCESS | 11760 |         {
19:25:09     INFO -  PROCESS | 11760 |             "name": "JSObject",
19:25:09     INFO -  PROCESS | 11760 |             "bytes": 100,
19:25:09     INFO -  PROCESS | 11760 |             "totalBytes": 100,
19:25:09     INFO -  PROCESS | 11760 |             "count": 10,
19:25:09     INFO -  PROCESS | 11760 |             "totalCount": 10,
19:25:09     INFO -  PROCESS | 11760 |             "id": 3,
19:25:09     INFO -  PROCESS | 11760 |             "parent": 1
19:25:09     INFO -  PROCESS | 11760 |         },
19:25:09     INFO -  PROCESS | 11760 |         {
19:25:09     INFO -  PROCESS | 11760 |             "name": "JSString",
19:25:09     INFO -  PROCESS | 11760 |             "bytes": 0,
19:25:09     INFO -  PROCESS | 11760 |             "totalBytes": 0,
19:25:09     INFO -  PROCESS | 11760 |             "count": 0,
19:25:09     INFO -  PROCESS | 11760 |             "totalCount": 0,
19:25:09     INFO -  PROCESS | 11760 |             "id": 4,
19:25:09     INFO -  PROCESS | 11760 |             "parent": 1
19:25:09     INFO -  PROCESS | 11760 |         }
19:25:09     INFO -  PROCESS | 11760 |     ],
19:25:09     INFO -  PROCESS | 11760 |     "id": 1
19:25:09     INFO -  PROCESS | 11760 | }
19:25:09     INFO -  PROCESS | 11760 | HEAPSNAPSHOT-TEST: actual: {
19:25:09     INFO -  PROCESS | 11760 |     "name": null,
19:25:09     INFO -  PROCESS | 11760 |     "bytes": 0,
19:25:09     INFO -  PROCESS | 11760 |     "totalBytes": 600,
19:25:09     INFO -  PROCESS | 11760 |     "count": 0,
19:25:09     INFO -  PROCESS | 11760 |     "totalCount": 60,
19:25:09     INFO -  PROCESS | 11760 |     "children": [
19:25:09     INFO -  PROCESS | 11760 |         {
19:25:09     INFO -  PROCESS | 11760 |             "name": "js::Shape",
19:25:09     INFO -  PROCESS | 11760 |             "bytes": 500,
19:25:09     INFO -  PROCESS | 11760 |             "totalBytes": 500,
19:25:09     INFO -  PROCESS | 11760 |             "count": 50,
19:25:09     INFO -  PROCESS | 11760 |             "totalCount": 50,
19:25:09     INFO -  PROCESS | 11760 |             "id": 2,
19:25:09     INFO -  PROCESS | 11760 |             "parent": 0
19:25:09     INFO -  PROCESS | 11760 |         },
19:25:09     INFO -  PROCESS | 11760 |         {
19:25:09     INFO -  PROCESS | 11760 |             "name": "JSObject",
19:25:09     INFO -  PROCESS | 11760 |             "bytes": 100,
19:25:09     INFO -  PROCESS | 11760 |             "totalBytes": 100,
19:25:09     INFO -  PROCESS | 11760 |             "count": 10,
19:25:09     INFO -  PROCESS | 11760 |             "totalCount": 10,
19:25:09     INFO -  PROCESS | 11760 |             "id": 1,
19:25:09     INFO -  PROCESS | 11760 |             "parent": 0
19:25:09     INFO -  PROCESS | 11760 |         },
19:25:09     INFO -  PROCESS | 11760 |         {
19:25:09     INFO -  PROCESS | 11760 |             "name": "JSString",
19:25:09     INFO -  PROCESS | 11760 |             "bytes": 0,
19:25:09     INFO -  PROCESS | 11760 |             "totalBytes": 0,
19:25:09     INFO -  PROCESS | 11760 |             "count": 0,
19:25:09     INFO -  PROCESS | 11760 |             "totalCount": 0,
19:25:09     INFO -  PROCESS | 11760 |             "id": 3,
19:25:09     INFO -  PROCESS | 11760 |             "parent": 0
19:25:09     INFO -  PROCESS | 11760 |         }
19:25:09     INFO -  PROCESS | 11760 |     ],
19:25:09     INFO -  PROCESS | 11760 |     "id": 0
19:25:09     INFO -  PROCESS | 11760 | }
Looks like a bad rebased -- added parent/id props to census tree nodes after all of Nick's patches landed from end of last week, working good now
Attachment #8678269 - Attachment is obsolete: true
Attachment #8678946 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/18427d3815fc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: