Perf. results page should have heuristics to detect bimodal distributions

[Ryosuke Niwa]

We should auto-detect tests with bi-modal distributions just like we detect tests with time-dependent distributions.
We can implement a simple heuristic based on binning (separate values into bins and look for two modes separated by reasonably big space).

[Eric Seidel (no email)]

Sounds cool.  I didn't realize we already did "time dependent distribution" detection.

[Ryosuke Niwa]

Created attachment 166345 [details]
Adds the feature

[Ryosuke Niwa]

Created attachment 166346 [details]
Sample output

[Ryosuke Niwa]

(In reply to comment #1)
> Sounds cool.  I didn't realize we already did "time dependent distribution" detection.

Yeah, we just added that thing a couple of days ago.

[Ryosuke Niwa]

Any reviewer?

[Zoltan Horvath]

Comment on attachment 166345 [details]
Adds the feature

I reviewed the change and the sample and it looks okay for me.

[Benjamin Poulain]

Comment on attachment 166345 [details]
Adds the feature

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

r- because of the typo :-D

> PerformanceTests/resources/results-template.html:420
> +function isHeuristicallyBiModal(values, expecetedNumberOfPointsInBin) {

expecetedNumberOfPointsInBin ->expected.
I am not sure why this is an argument here.

> PerformanceTests/resources/results-template.html:426
> +        var bins = [];

var bins = new Array(numberOfBins) ?

> PerformanceTests/resources/results-template.html:433
> +            bins[Math.floor((values[i] - min) / binSize)] += 1 / values.length;

Why "1 / values.length"?

Wouldn't it be easier to follow by doing comparison with the sample size? E.g.
    bins[i] >= values.length / 4

> PerformanceTests/resources/results-template.html:452
> +                    if (firstMode + Math.max(1, numberOfBins / 5) >= i)
> +                        continue; // Too close.
> +                    var thereAreEnoughValuesBetweenModes = false;
> +                    for (var j = firstMode + 1; j < i; j++) {
> +                        if (bins[j] >= 0.1) {
> +                            thereAreEnoughValuesBetweenModes = true;
> +                            break;
> +                        }
> +                    }
> +                    if (thereAreEnoughValuesBetweenModes)
> +                        continue;
> +                    return true;

I am not sure I like this, it makes impossible to interpret what is being computed.

Could this be achieved by having a bigger initial expecetedNumberOfPointsInBin, and only checking (i - firstMode) > threshold?