[webkit-reviews] review denied: [Bug 97960] Perf. results page should have heuristics to detect bimodal distributions : [Attachment 166345] Adds the feature

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Ryosuke Niwa
<rniwa at webkit.org>'s request for review:
Bug 97960: Perf. results page should have heuristics to detect bimodal
distributions
https://bugs.webkit.org/show_bug.cgi?id=97960

Attachment 166345: Adds the feature
https://bugs.webkit.org/attachment.cgi?id=166345&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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?