Bug 199050 - [perf.webkit.org] Update summary page calculations to use mean instead of median
Summary: [perf.webkit.org] Update summary page calculations to use mean instead of median
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Johnson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-19 18:25 PDT by Dean Johnson
Modified: 2019-06-24 11:21 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.69 KB, patch)
2019-06-19 23:10 PDT, Dean Johnson
no flags Details | Formatted Diff | Diff
Patch (3.45 KB, patch)
2019-06-20 16:15 PDT, Dean Johnson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Johnson 2019-06-19 18:25:49 PDT
Median of a set of data points can see higher variation in each view of the summary page than when using mean. This means a benchmark + platform's status may see major shifts as new data comes in, or when a benchmark produces bi-modal results. The downside to using mean is that any major outliers will impact the score a benchmark sees for a longer period of time.

When doing comparisons between both median and mean-based calculations locally, results did not appear to shift wildly (i.e. from regressed => progressed or progressed => regressed). That said, I did see the resolution of a major regression cause incremental decreases to a single calculation as more data points came in, which seems desirable over an abrupt shift.
Comment 1 Dean Johnson 2019-06-19 18:28:05 PDT
<rdar://problem/51763007>
Comment 2 Dean Johnson 2019-06-19 23:10:16 PDT
Created attachment 372539 [details]
Patch
Comment 3 Ryosuke Niwa 2019-06-20 14:46:57 PDT
Comment on attachment 372539 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372539&action=review

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:378
> +    static _medianForTimeRange(timeSeries, timeRange)
> +    {

Why do we need this function given that we're no longer using median?
Comment 4 dewei_zhu 2019-06-20 14:49:26 PDT
Comment on attachment 372539 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372539&action=review

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:379
> +        var [startPoint, endPoint] = SummaryPageConfigurationGroup._startAndEndPointForTimeRange(timeSeries, timeRange);

We favor 'const'/'let' over 'var'.

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:385
> +        var [startPoint, endPoint] = SummaryPageConfigurationGroup._startAndEndPointForTimeRange(timeSeries, timeRange);

ditto
Comment 5 Ryosuke Niwa 2019-06-20 15:11:22 PDT
Comment on attachment 372539 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372539&action=review

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:350
> +            var baselineMean = SummaryPageConfigurationGroup._meanForTimeRange(baselineTimeSeries, timeRange);
> +            var currentMean = SummaryPageConfigurationGroup._meanForTimeRange(currentTimeSeries, timeRange);

We should use const here.
Comment 6 Dean Johnson 2019-06-20 16:06:11 PDT
(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 372539 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372539&action=review
> 
> > Websites/perf.webkit.org/public/v3/pages/summary-page.js:378
> > +    static _medianForTimeRange(timeSeries, timeRange)
> > +    {
> 
> Why do we need this function given that we're no longer using median?

I figured it was worth keeping in-case we wanted to revert or test what using median would look like instead, but in hindsight that was a poor decision since we'd just roll back this patch anyways.

I'll remove it.

(In reply to dewei_zhu from comment #4)
> Comment on attachment 372539 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372539&action=review
> 
> > Websites/perf.webkit.org/public/v3/pages/summary-page.js:379
> > +        var [startPoint, endPoint] = SummaryPageConfigurationGroup._startAndEndPointForTimeRange(timeSeries, timeRange);
> 
> We favor 'const'/'let' over 'var'.
> 
> > Websites/perf.webkit.org/public/v3/pages/summary-page.js:385
> > +        var [startPoint, endPoint] = SummaryPageConfigurationGroup._startAndEndPointForTimeRange(timeSeries, timeRange);
> 
> ditto
The existing code used var, but I can change it to const.

(In reply to Ryosuke Niwa from comment #5)
> Comment on attachment 372539 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372539&action=review
> 
> > Websites/perf.webkit.org/public/v3/pages/summary-page.js:350
> > +            var baselineMean = SummaryPageConfigurationGroup._meanForTimeRange(baselineTimeSeries, timeRange);
> > +            var currentMean = SummaryPageConfigurationGroup._meanForTimeRange(currentTimeSeries, timeRange);
> 
> We should use const here.
Will fix.
Comment 7 Dean Johnson 2019-06-20 16:15:14 PDT
Created attachment 372599 [details]
Patch
Comment 8 Dean Johnson 2019-06-20 16:24:32 PDT
New patch should address comments around 'var' usage.
Comment 9 dewei_zhu 2019-06-21 12:16:28 PDT
Comment on attachment 372599 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372599&action=review

> Websites/perf.webkit.org/ChangeLog:12
> +        (SummaryPageConfigurationGroup.set _meanForTimeRange): New. Returns the mean of a timeSeries across a

Where is the '.set' coming from?
Comment 10 Dean Johnson 2019-06-21 12:17:36 PDT
Comment on attachment 372599 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372599&action=review

>> Websites/perf.webkit.org/ChangeLog:12
>> +        (SummaryPageConfigurationGroup.set _meanForTimeRange): New. Returns the mean of a timeSeries across a
> 
> Where is the '.set' coming from?

I'm not sure - it was auto-generated. I can upload a new patch without it if you'd like.
Comment 11 WebKit Commit Bot 2019-06-24 11:20:51 PDT
Comment on attachment 372599 [details]
Patch

Clearing flags on attachment: 372599

Committed r246743: <https://trac.webkit.org/changeset/246743>
Comment 12 WebKit Commit Bot 2019-06-24 11:20:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-06-24 11:21:21 PDT
<rdar://problem/52061931>