Closed Bug 1151973 Opened 9 years ago Closed 9 years ago

Inverted call tree should be ordered by 'self cost', not 'total cost'

Categories

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

x86_64
Windows 8.1
defect

Tracking

(firefox40 verified, firefox41 fixed)

VERIFIED FIXED
Firefox 41
Tracking Status
firefox40 --- verified
firefox41 --- fixed

People

(Reporter: djvj, Assigned: vporof)

References

Details

Attachments

(2 files, 2 obsolete files)

Currently in the profiler, inverted call trees are sorted by 'total cost'.  Given that the inverted tree tries to get at the 'leaf frames' where we spend time, it makes sense to count 'self cost' instead of 'total cost'.  Under the total cost model, we get issues like the following:

function foo() {
  bar(); // 999 samples containing foo() => bar()
  for (...) {
    something;  // 1 sample with 'foo()' as leaf, within loop.
  }
}

Here, 'foo()' becomes a leaf frame because of the 1 leaf sample at it, but because the inverted call tree sorts by total cost, it gets a far higher 'score' than otherwise warranted, and gets sorted up near the top of the frame list.  In reality, we only have 1 sample in 'foo()' directly and it should be sorted at the bottom.

Logical solution is to sort inverted call tree by self cost, which counts the amount of time spent directly in a given function.
Blocks: perf-tool-v2
Priority: -- → P1
Assignee: nobody → vporof
Status: NEW → ASSIGNED
No longer blocks: perf-tool-v2
Attached patch invert-cost.patch (obsolete) — Splinter Review
Attachment #8604672 - Flags: review?(jsantell)
Comment on attachment 8604672 [details] [diff] [review]
invert-cost.patch

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

::: browser/devtools/performance/test/browser.ini
@@ +128,5 @@
>  [browser_profiler_tree-view-05.js]
>  [browser_profiler_tree-view-06.js]
>  [browser_profiler_tree-view-07.js]
>  [browser_profiler_tree-view-08.js]
> +[browser_profiler_tree-view-09.js]

Missing test file?

::: browser/devtools/shared/profiler/tree-view.js
@@ +274,5 @@
>      // Sort the "callees" asc. by samples, before inserting them in the tree,
>      // if no other sorting predicate was specified on this on the root item.
> +    children.sort((a, b) => {
> +      return this.sortingPredicate(a, b, a._computedData, b._computedData, this.inverted);
> +    });

Seems weird to call a predicate function that calls another predicate fn

children.sort(this.sortingPredicate);

sortingPredicate: function (a, b) {
  return this.inverted ?
    (a._computedData.selfPercentage < b._computedData.selfPercentage ? 1 : -1) :
    (a.frame.samples < b.frame.samples ? 1 : -1);
}
Gah, hg add.

Yeah i didn't want to access private fields inside predicates, but a getter should work.
Attached patch v2 (obsolete) — Splinter Review
Attachment #8604672 - Attachment is obsolete: true
Attachment #8604672 - Flags: review?(jsantell)
Attachment #8604735 - Flags: review?(jsantell)
Comment on attachment 8604735 [details] [diff] [review]
v2

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

::: browser/devtools/performance/test/browser_profiler_tree-view-09.js
@@ +7,5 @@
> + */
> +
> +let { CATEGORY_MASK } = devtools.require("devtools/shared/profiler/global");
> +
> +let test = Task.async(function*() {

should use spawnTest but I don't care since these should all be xpcshell tests eventually
Attachment #8604735 - Flags: review?(jsantell) → review+
Attached patch v2Splinter Review
Welp I had to rewrite this thanks to shu's patches... which might have been a good thing because I cleaned up tree-model too.
Attachment #8605958 - Flags: review?(jsantell)
Comment on attachment 8605958 [details] [diff] [review]
v2

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

::: browser/devtools/shared/profiler/tree-view.js
@@ +275,5 @@
>      }
>  
>      return cell;
>    },
> +  _appendFunctionDetailsCells: function(cell, frameInfo) {

brief function comments here
Attachment #8605958 - Flags: review?(jsantell) → review+
There's a comment above for all these microfunctions.

/**
 * Functions creating each cell in this call view.
 * Invoked by `_displaySelf`.
 */
Darnit
Flags: needinfo?(vporof)
https://hg.mozilla.org/mozilla-central/rev/076cab9235a5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Flags: qe-verify+
Attachment #8604735 - Attachment is obsolete: true
Comment on attachment 8605958 [details] [diff] [review]
v2


Approval Request Comment
[Feature/regressing bug #]: 1167252, the new performance tool
[User impact if declined]: Won't ship the performance tool
[Describe test coverage new/current, TreeHerder]: There are try pushes in Bug 1167252 with all patches needing uplift
[Risks and why]: Requesting uplift for the accumulated changes in the performance tool since the 40 merge date, so these changes haven't had the full 6 weeks to bake.  Risks are generally contained within devtools, specifically within the performance panel.
[String/UUID change made/needed]: None
Attachment #8605958 - Flags: approval-mozilla-aurora?
Note: I had verbal confirmation for these uplifts from Sylvestre even before he's flagged them as a+.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1167252#c26
Comment on attachment 8605958 [details] [diff] [review]
v2

Change approved to skip one train as part of the spring campaign.
Attachment #8605958 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on Aurora 40.0a2 (2015-06-04), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.8.5.

If the Call Tree is inverted it'll get ordered by "Self Cost", otherwise it will be by default ordered according to "Total Cost".
Status: RESOLVED → VERIFIED
Flags: qe-verify+
I was just doing some profiling on a codebase using a recent m-c build, and we still seem to be sorting by total cost instead of self cost.

In this screenshot, |SourceMapConsumer_parseMappings| should be sorted above |doQuickSort|.
Flags: needinfo?(vporof)
This looks like it could be a recent regression. Not sure why this is happening, I'll take a quick look at some point.
Flags: needinfo?(vporof)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.