search

catapult-project / catapult

Deprecated Catapult GitHub. Please instead use http://crbug.com "Speed>Benchmarks" component for bugs and https://chromium.googlesource.com/catapult for downloading and editing source code..
https://chromium.googlesource.com/catapult
BSD 3-Clause "New" or "Revised" License
1.91k stars 563 forks source link

Pinpoint: Make regression amounts clearer #3871

Open anniesullie opened 6 years ago

anniesullie commented 6 years ago

I'm going through and triaging old perf bugs, and one of the biggest things I need to understand is the magnitude of the regression found by bisect:

  1. Regression amount. For each regression, I currently find myself looking at the bisect numbers to see the regression amount. For example, in this bug, the bisect is clear and reproducible, but since it's only reproing 592 bytes of memory change it's not finding something useful. It would be really great to be able to see the approximate regression amount for each culprit easily.

  2. Human-readable units. In this example pinpoint bisect, we see units like 607500 for memory. But in this example perf bug we see units like 646.0KiB. The latter is much more readable and makes for faster triage.

@dave-2 @simonhatch

dave-2 commented 6 years ago

Are you interested in the mean, median, or interquartile mean?

(The IQM is when you chop off the bottom and top 25% and take the mean of the rest. Since the median is when you chop off the bottom and top 50% and take the mean of the rest, the IQM is always between the mean and median.)

dave-2 commented 6 years ago

For point #2, we can read the raw units from the ChartJSON, but to do the conversion from 607500 to 593.3 KiB, we maybe need to integrate Histogram Units.

@eakuefner @benshayden

benshayden commented 6 years ago

You should be able to call Unit.format() similar to how the alerts-table does it. If you only have legacy units, you can use LEGACY_UNIT_INFO to map them to tr.b.Units. Feel free to schedule a VC if you have more specific questions.

dave-2 commented 6 years ago

We'll also want to update the docs at https://chromium.googlesource.com/chromium/src/+/master/docs/speed/addressing_performance_regressions.md when the units are available in Pinpoint.

perezju commented 6 years ago

+1! Also would be great if it were possible, on the job UI, to sort the culprits found by size of the regression. In the case of "lots of regressions found" maybe only cc people from the 2 or 3 largest regressions?

As to what statistic to use, I would probably just go for mean since that's the easier to explain, and what people are used to. I expect in most cases it wont make much of a difference which statistic we report. (Most of the times I'm just looking for an order of magnitude estimate.)