Bug 199050 – [perf.webkit.org] Update summary page calculations to use mean instead of median

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dean Johnson]

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.

[Dean Johnson]

<rdar://problem/51763007>

[Dean Johnson]

Created attachment 372539 [details]
Patch

[Ryosuke Niwa]

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 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

[Ryosuke Niwa]

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.

[Dean Johnson]

(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.

[Dean Johnson]

Created attachment 372599 [details]
Patch

[Dean Johnson]

New patch should address comments around 'var' usage.

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?

[Dean Johnson]

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.

[WebKit Commit Bot]

Comment on attachment 372599 [details]
Patch

Clearing flags on attachment: 372599

Committed r246743: <https://trac.webkit.org/changeset/246743>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52061931>