Bug 97960 - Perf. results page should have heuristics to detect bimodal distributions
Summary: Perf. results page should have heuristics to detect bimodal distributions
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 77037
  Show dependency treegraph
 
Reported: 2012-09-28 18:35 PDT by Ryosuke Niwa
Modified: 2017-07-18 08:27 PDT (History)
5 users (show)

See Also:


Attachments
Adds the feature (6.75 KB, patch)
2012-09-28 18:42 PDT, Ryosuke Niwa
benjamin: review-
Details | Formatted Diff | Diff
Sample output (97.37 KB, text/html)
2012-09-28 18:43 PDT, Ryosuke Niwa
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-09-28 18:35:44 PDT
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).
Comment 1 Eric Seidel (no email) 2012-09-28 18:36:59 PDT
Sounds cool.  I didn't realize we already did "time dependent distribution" detection.
Comment 2 Ryosuke Niwa 2012-09-28 18:42:43 PDT
Created attachment 166345 [details]
Adds the feature
Comment 3 Ryosuke Niwa 2012-09-28 18:43:35 PDT
Created attachment 166346 [details]
Sample output
Comment 4 Ryosuke Niwa 2012-09-28 18:50:22 PDT
(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.
Comment 5 Ryosuke Niwa 2012-10-01 10:09:26 PDT
Any reviewer?
Comment 6 Zoltan Horvath 2012-10-04 14:38:05 PDT
Comment on attachment 166345 [details]
Adds the feature

I reviewed the change and the sample and it looks okay for me.
Comment 7 Benjamin Poulain 2013-02-26 14:52:52 PST
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?