Closed Bug 1080911 Opened 5 years ago Closed 5 years ago

Sort frames by samples, not duration, in the call tree

Categories

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

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: fitzgen, Assigned: christian.pommranz)

Details

(Whiteboard: [good first bug][lang=js][mentor=vporof])

Attachments

(1 file, 2 obsolete files)

Samples are the real deal.

Duration is this thing we generated from the samples, but is funky because we don't deal with harmonics, apparently.
Whiteboard: [good first bug][lang=js][mentor=vporof]
Can we get a DXR link up in here?
Attached patch bug1080911.patch (obsolete) — Splinter Review
Comment on attachment 8509494 [details] [diff] [review]
bug1080911.patch

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

Nice! Did you check if all the tests still pass?
Attachment #8509494 - Flags: feedback+
Attached patch Fix tests (obsolete) — Splinter Review
Changed the failed tests to the expected values. All tests are now passing. However, since we're now sorting by samples and not by duration, we could change the test case to better reflect that (i.e. add another .A.B.D frame in http://dxr.mozilla.org/mozilla-central/source/browser/devtools/profiler/test/browser_profiler_tree-view-03.js#76 and update the expected results). Don't know if that's desired though.
Attachment #8509494 - Attachment is obsolete: true
Test improvements are always desired! :D
Christian, did you want this patch reviewed? Can you ask me for r? when you're ready?
Added another .A.B.D sample to the test case to make sure some tests are failing if the entries are sorted by duration rather than samples. The expected values are updated. I changed the test cases in all tree-view-0X files to stay consistent.
Nice secondary effect: Tests are now passing on l10n formats not using '.' as decimal point, too.
Attachment #8510274 - Attachment is obsolete: true
Attachment #8512616 - Flags: review?(vporof)
Attachment #8512616 - Flags: review?(vporof) → review+
Thanks Christian!
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/fx-team/rev/baaa389d2e23
Assignee: nobody → christian.pommranz
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js][mentor=vporof] → [good first bug][lang=js][mentor=vporof][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/baaa389d2e23
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][mentor=vporof][fixed-in-fx-team] → [good first bug][lang=js][mentor=vporof]
Target Milestone: --- → Firefox 36
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.