1201542 - Implement `allocationSite` breakdown for CensusTreeNode

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Sending your way

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Implement `allocationSite` breakdown for CensusTreeNode

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dda832df3346

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8675976 [details] [diff] [review]
Implement `allocationSite` breakdown for CensusTreeNode

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

some comments but lgtm!

::: devtools/shared/heapsnapshot/census-tree-node.js
@@ +61,5 @@
> + * @returns {String}
> + *          The unique string that can be used as a key in a FrameCache.
> + */
> +FrameCache.hash = function (frame) {
> +  return `${frame.functionDisplayName},${frame.source},${frame.line},${frame.column},${frame.asyncCause}`;

Do we always have a column? What about when functions don't have names? Not sure if SavedFrames handles this better than the profiler (I think it does, and tbf it's not hard to handle this better than profiler);

A line like: `p.then(function(){},function(){})` where two anon functions are used

::: devtools/shared/heapsnapshot/tests/unit/head_heapsnapshot.js
@@ +155,5 @@
> +}
> +
> +function savedFrameReplacer(key, val) {
> +  if (isSavedFrame(val)) {
> +    return "<SavedFrame '" + val.toString().split(/\n/g).shift() + "'>";

template strings might be nicer

@@ +188,5 @@
>  }
>  
>  // Deep structural equivalence that can handle Map objects in addition to plain
>  // objects.
>  function assertStructurallyEquivalent(actual, expected, path="root") {

Wonder if we should roll this into the `breakdownEquals` function I made for breakdowns, if we add map support, and just have it a general structural equiv function?

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4)
> Comment on attachment 8675976 [details] [diff] [review]
> Implement `allocationSite` breakdown for CensusTreeNode
> 
> Review of attachment 8675976 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> some comments but lgtm!
> 
> ::: devtools/shared/heapsnapshot/census-tree-node.js
> @@ +61,5 @@
> > + * @returns {String}
> > + *          The unique string that can be used as a key in a FrameCache.
> > + */
> > +FrameCache.hash = function (frame) {
> > +  return `${frame.functionDisplayName},${frame.source},${frame.line},${frame.column},${frame.asyncCause}`;
> 
> Do we always have a column? What about when functions don't have names? Not
> sure if SavedFrames handles this better than the profiler (I think it does,
> and tbf it's not hard to handle this better than profiler);
> 
> A line like: `p.then(function(){},function(){})` where two anon functions
> are used

Yes, we always have a column. SpiderMonkey will infer display names in most cases, and worst case scenario it will be undefined, but that's ok we still want that to contribute to the hash as it is part of each frame's identity.

> @@ +188,5 @@
> >  }
> >  
> >  // Deep structural equivalence that can handle Map objects in addition to plain
> >  // objects.
> >  function assertStructurallyEquivalent(actual, expected, path="root") {
> 
> Wonder if we should roll this into the `breakdownEquals` function I made for
> breakdowns, if we add map support, and just have it a general structural
> equiv function?

What's nice about this function now is that it keeps track of where you are in the structure via the path parameter and asserts along the way rather than returning true/false which gives better failure messages. Both of these things you would not want in an equality checker because (a) you want to be fast and not carry around long strings, and (b) you don't want to assert but return true or false. Additionally, it does not attempt to handle cyclic values at all, which a robust equality checker handling untrusted data should. I think we should keep this as a test utility.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3a73b2d41ed5

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for m(oth) failures: https://treeherder.mozilla.org/logviewer.html#?job_id=5219453&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/101384a5c8c9

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Looks like it also broke an xpcshell test: https://treeherder.mozilla.org/logviewer.html#?job_id=5219176&repo=fx-team

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I think this was a bad rebase.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Implement `allocationSite` breakdown for CensusTreeNode

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb080004c158

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New try push, apparently that last one was affected by the S3 migration.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ae2f6266fdf

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3e148d8f5e05

Comment 14

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/integration/fx-team/rev/b408336f4b90226a9617d022c1c768008c6861e2
Backed out changeset 3e148d8f5e05 (bug 1201542) for XPCShell bustage

Comment 15

[Nigel Babu [:nigelb]]

Busted XPCSHell tests like this -> https://treeherder.mozilla.org/logviewer.html#?job_id=5260235&repo=fx-team I had to back out bug 1217239 as it's dependent on this one.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e5d4bd62d234

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e5d4bd62d234