Bug 230810 – Summary page should support calculating summary using weighted mean.

• Component: New Bugs
• Status: RESOLVED FIXED

Summary page should support calculating summary using weighted mean.

Created attachment 439301 [details]
Patch

rdar://83554204

[Ryosuke Niwa]

Comment on attachment 439301 [details]
Patch

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

> Websites/perf.webkit.org/public/shared/statistics.js:19
> +    this.weightedMean = function (values) {

It's confusing to call this argument "values".
Maybe valuesWithWeights?

> Websites/perf.webkit.org/public/shared/statistics.js:27
> +            if (!Array.isArray(item)) {
> +                totalWeight += 1;
> +                sum += item;
> +                continue;
> +            }

I don't think we should do polymorphism like this.

> Websites/perf.webkit.org/public/shared/statistics.js:31
> +            const [value, weight] = item;
> +            totalWeight += weight;
> +            sum += value * weight;

Why not just use a dictionary / object?
e.g.
totalWeight += item.weight;
sum += item.weight * item.value;

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:17
> +        this._customWeightConfigurations = summarySettings.customWeightConfigurations;

It doesn't seem useful to say this is custom weight?
How about just weightedConfigurations?

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:352
> +        if (typeof weightForPlatform === 'number')

There is no point in using === here. The value of typeof is always a string.
But it seems problematic to assume anything that's not a number is an object?
e.g. string and null.

Created attachment 439318 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 439318 [details]
Patch

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

We really need tests for summary page...

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:17
> +        this._weightConfigurations = summarySettings.weightConfigurations;

This sounds like the configuration of weights. So I think it should be weightedConfigurations.
Like excluded configurations, we're listing a list of configurations whose weight isn't 1 along with its weight.

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:354
> +        const isNumber = (object) => ['number', 'string'].includes(typeof object) && !isNaN(+object);
> +        if (isNumber(weightForPlatform))
> +            return +weightForPlatform;

This isn't quite right. This will throw an exception if weightForPlatform is null or true/false as well.
I think what we want to check instead is simply isNaN(+object). i.e.
if (!isNaN(weightForPlatform))
    return weightForPlatform;

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:358
> +        const weight = weightForPlatform[metricId]
> +        if (isNumber(weight))
> +            return +weight;

Here, we probably want to do instead:
if (typeof weightForPlatform != 'object' || isNaN(+weightForPlatform[metricId]))
    return 1;

Landed in r283153.