1167899 - Call Tree data should be based only on sample count

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

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

Couple questions:

* Should we consolidate, or remove, some columns? These take up a lot of room, and we can probably use tooltips or something for any info that is less useful, but sometimes desirable.
* Should samples only be displayed for when the frame is a leaf? This is how OSX instruments does it: http://i.imgur.com/muO5tqA.png
* Should we use samples as the defacto "self cost" of a frame? Again, like OSX instruments above; they do not have a self cost/time


Things that need to be done for sure:

* if we stick with self cost/time, total cost/time, we should atleast be in the same order as Chrome.
* Change tooltip for inverted call tree "total" cost, whatever it winds up being -- it's from the perspective of a non-inverted tree.
* Everything should be based on sample counts -- right now some values are based on samples, others on duration. We should just use sample count for everything, because right now, some things are kinda off from rounding/statistical errors

Comment 1

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #0)
> Couple questions:
> 
> * Should we consolidate, or remove, some columns? These take up a lot of
> room, and we can probably use tooltips or something for any info that is
> less useful, but sometimes desirable.

I think we could do something like Chrome does where Self and Total have percent and ms within them. But I don't think it matters that much.

> * Should samples only be displayed for when the frame is a leaf? This is how
> OSX instruments does it: http://i.imgur.com/muO5tqA.png

Yes. That is how we get the data, but then we walk the stack and start incrementing sample count for all parent frames. Not sure why we do (did?) that, but it is not intuitive for me as a user.

Of course, when the set of samples is aggregated into a tree, you can still get samples for things that aren't a leaf in the tree, but were a leaf in a particular sample, eg:

Samples:
A->B
A->B->C
A->B->C

Tree:
+---------+----------+
| Samples | Function |
+---------+----------+
|       0 | A        |
|       1 |   B      |
|       2 |     C    |
+---------+----------+

> * Should we use samples as the defacto "self cost" of a frame? Again, like
> OSX instruments above; they do not have a self cost/time

Nope. Self cost/time is the most important measurement most of the time, because that is where time is actually being spent. I don't want to have to calculate the numbers in my head when comparing two functions and looking at their sample counts.

> Things that need to be done for sure:
> 
> * if we stick with self cost/time, total cost/time, we should atleast be in
> the same order as Chrome.

I guess. I don't feel strongly about this, but it might help new users, or people who are more used to Crhome.

> * Everything should be based on sample counts -- right now some values are
> based on samples, others on duration. We should just use sample count for
> everything, because right now, some things are kinda off from
> rounding/statistical errors

+1

Comment 2

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

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #1)
> (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #0)
> > * Should samples only be displayed for when the frame is a leaf? This is how
> > OSX instruments does it: http://i.imgur.com/muO5tqA.png
> 
> Yes. That is how we get the data, but then we walk the stack and start
> incrementing sample count for all parent frames. Not sure why we do (did?)
> that, but it is not intuitive for me as a user.
> 
> Of course, when the set of samples is aggregated into a tree, you can still
> get samples for things that aren't a leaf in the tree, but were a leaf in a
> particular sample, eg:
> 
> Samples:
> A->B
> A->B->C
> A->B->C
> 
> Tree:
> +---------+----------+
> | Samples | Function |
> +---------+----------+
> |       0 | A        |
> |       1 |   B      |
> |       2 |     C    |
> +---------+----------+
> 

Yes -- samples would still be counted if it's not /visually/ a leaf, but a leaf in some sample (as this is what our self cost/times are from, giving B some weight here) -- I think we're in agreement here


> > * Should we use samples as the defacto "self cost" of a frame? Again, like
> > OSX instruments above; they do not have a self cost/time
> 
> Nope. Self cost/time is the most important measurement most of the time,
> because that is where time is actually being spent. I don't want to have to
> calculate the numbers in my head when comparing two functions and looking at
> their sample counts.

I think I didn't explain well: instead of having self cost/time as a percentage and time, what if we just had the sample count, like here is just total cost/time and sample count: http://i.imgur.com/muO5tqA.png

Comment 3

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #2)
> Yes -- samples would still be counted if it's not /visually/ a leaf, but a
> leaf in some sample (as this is what our self cost/times are from, giving B
> some weight here) -- I think we're in agreement here

Yup!

Upon reflection, I think we should probably use the term "youngest frame" here (as we do elsewhere, like in the debugger and in SavedStacks) and reserve the word "leaf" for the actual tree.

> I think I didn't explain well: instead of having self cost/time as a
> percentage and time, what if we just had the sample count, like here is just
> total cost/time and sample count: http://i.imgur.com/muO5tqA.png

But then I still have to do the arithmetic in my head to figure out how long this particular function (and not its callees) is actually taking or what percentage of the profile its taking up. I'd have to compare two (potentially "large") numbers with out any "out of a total of N" context, even. That's exactly what I shouldn't have to do, and exactly what the tools should do for me.

Comment 5

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

Attachment: 1167899-samples.patch

All sample times are based off of sample count, rather than the time stamp found on samples.

Also, due to this, a start and end time is now required for computing the tree models -- we could use sample time of the last sample or so, but this should be in sync with other views, and not be slightly off if we use something else. This was a 100% root node's time equals the current selection exactly.

Removed OVERVIEW_RANGE_CLEARED event, as it's not really necessary.

Views now always received a time range in the render function to support this as well.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8617044 [details] [diff] [review]
1167899-samples.patch

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

Great simplification.

::: browser/devtools/performance/modules/logic/tree-model.js
@@ +165,5 @@
>        let prevCalls = this.calls;
>        let prevFrameKey;
>        let isLeaf = mutableFrameKeyOptions.isLeaf = true;
>        let skipRoot = options.invertTree;
> +      let framesInStack = Object.create(null);

Dead variable leftover from a previous version? Please remove.

@@ -237,5 @@
> -        } else {
> -          // An empty frame key means this frame should be skipped.
> -          if (frameKey === "") {
> -            continue;
> -          }

Nice, very happy to see this removed.

@@ +335,5 @@
> + *  A()->B()
> + *  Q()->B()
> + *
> + * In inverted tree, a->b->c would have one frame node, and a->b and q->b would share a frame node.
> + * In an uninverted tree, a->b->c and a->b would share a frame node, with q->b having its own.

Nit: Please make the stack representations consistent, like same capitalization, no parentheses.

@@ +388,5 @@
> +    if (isLeaf) {
> +      this.leafSamples++;
> +    }
> +    this.samples++;
> +  },

Since you split out _addOptimizations, this function is now very simple. I would just inline it into its caller.

::: browser/devtools/performance/modules/widgets/tree-view.js
@@ +374,5 @@
> +      data.totalDuration = this.frame.samples / totalSamples * totalDuration;
> +    }
> +    if (this.visibleCells.percentage) {
> +      data.totalPercentage = this.frame.samples / totalSamples * 100;
> +    }

Oh neat, because you track frame.samples separately from frame.leafSamples, you can always do a root-relative computation and still get parent-relative percentage numbers.

Could you add a comment here explaining this briefly?

Comment 7

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

comments addressed,  https://hg.mozilla.org/integration/fx-team/rev/1ed548d6c222

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1ed548d6c222