Bug 110842 – Use perf.webkit.org JSON format in results page

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Now that we're uploading results to perf.webkit.org using new JSON format, we should use the same format in results page.

[Ryosuke Niwa]

Created attachment 190200 [details]
Updated the results page template

[Ryosuke Niwa]

Created attachment 190217 [details]
Also change the default JSON format

[Ryosuke Niwa]

(In reply to comment #2)
> Created an attachment (id=190217) [details]
> Also change the default JSON format

Turns out that I had to make more changes at once in order to not break various features.

[Dirk Pranke]

Comment on attachment 190217 [details]
Also change the default JSON format

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

python code looks fine. A couple of minor JS comments, but you probably want someone more familiar w/ JS and/or this code to review that.

> PerformanceTests/resources/results-template.html:574
> +        addTests(entry.tests, '');

maybe I'm missing something, but why make this a function to only call it once?

> PerformanceTests/resources/statistics.js:84
> +    var tDistributionQuantiles = {

Can you add a comment about what the values in this table are and where they come from, for those of us who are less informed in such things?

[Benjamin Poulain]

Comment on attachment 190217 [details]
Also change the default JSON format

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

> PerformanceTests/resources/results-template.html:226
> +    var unit = {'FrameRate': 'fps',
> +        'Runs': 'runs/s',
> +        'Time': 'ms',
> +        'Malloc': 'bytes',
> +        'JSHeap': 'bytes'}[metric];

I'd cut this in two lines.

> PerformanceTests/resources/results-template.html:370
> -        min: minIsZero ? 0 : Math.min.apply(Math, results.map(function (result, index) { return result.min(); })) * 0.98,
> -        max: Math.max.apply(Math, results.map(function (result, index) { return result.max(); })) * (minIsZero ? 1.1 : 1.01)}});
> +        min: minIsZero ? 0 : overallMin - (overallMax - overallMin) * 0.1,
> +        max: minIsZero ? overallMax * 1.1 : overallMax + (overallMax - overallMin) * 0.1}});

For clarity?: var margin = (overallMax - overallMin) * 0.1;

> PerformanceTests/resources/statistics.js:37
> +        return values.length ? values.reduce(function (a, b) { return a + b; }) : 0;

return values.reduce(function (a, b) { return a + b; }, 0)?

> PerformanceTests/resources/statistics.js:41
> +        return values.length ? values.reduce(function (sum, value) { return sum + value * value;}, 0) : 0;

ditto

> PerformanceTests/resources/statistics.js:45
> +    // See https://rniwa.com/2012-11-10/sample-standard-deviation-in-terms-of-sum-and-square-sum-of-samples/

hehe

> PerformanceTests/resources/statistics.js:49
> +        return Math.sqrt(squareSum / (numberOfSamples - 1) - sum * sum / (numberOfSamples - 1) / numberOfSamples);

I'd split this on multiple lines.
And do (sum * sum) * numberOfSamples / (numberOfSamples - 1)

> PerformanceTests/resources/statistics.js:86
> +    var tDistributionQuantiles = {
> +        0.9: [
> +            3.077684, 1.885618, 1.637744, 1.533206, 1.475884, 1.439756, 1.414924, 1.396815, 1.383029, 1.372184,

Aren't those the one-tail numbers?

[Ryosuke Niwa]

Comment on attachment 190217 [details]
Also change the default JSON format

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

>> PerformanceTests/resources/results-template.html:574
>> +        addTests(entry.tests, '');
> 
> maybe I'm missing something, but why make this a function to only call it once?

It's recursive. See lines 569-570.

>> PerformanceTests/resources/statistics.js:84
>> +    var tDistributionQuantiles = {
> 
> Can you add a comment about what the values in this table are and where they come from, for those of us who are less informed in such things?

It's these http://www.math.uni-magdeburg.de/~rooff/statistics2/tabstud.pdf

[Ryosuke Niwa]

Comment on attachment 190217 [details]
Also change the default JSON format

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

>> PerformanceTests/resources/statistics.js:86
>> +            3.077684, 1.885618, 1.637744, 1.533206, 1.475884, 1.439756, 1.414924, 1.396815, 1.383029, 1.372184,
> 
> Aren't those the one-tail numbers?

That's a good point. tDistributionCDFTail?

[Dirk Pranke]

Comment on attachment 190217 [details]
Also change the default JSON format

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

>>> PerformanceTests/resources/results-template.html:574
>>> +        addTests(entry.tests, '');
>> 
>> maybe I'm missing something, but why make this a function to only call it once?
> 
> It's recursive. See lines 569-570.

Ah, right :).

>>> PerformanceTests/resources/statistics.js:84
>>> +    var tDistributionQuantiles = {
>> 
>> Can you add a comment about what the values in this table are and where they come from, for those of us who are less informed in such things?
> 
> It's these http://www.math.uni-magdeburg.de/~rooff/statistics2/tabstud.pdf

I meant add a comment to the code :).

[Ryosuke Niwa]

Comment on attachment 190217 [details]
Also change the default JSON format

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

>>>> PerformanceTests/resources/statistics.js:84
>>>> +    var tDistributionQuantiles = {
>>> 
>>> Can you add a comment about what the values in this table are and where they come from, for those of us who are less informed in such things?
>> 
>> It's these http://www.math.uni-magdeburg.de/~rooff/statistics2/tabstud.pdf
> 
> I meant add a comment to the code :).

I can't think of any useful comment.

[Benjamin Poulain]

> I'd split this on multiple lines.
> And do (sum * sum) * numberOfSamples / (numberOfSamples - 1)
> 
> > PerformanceTests/resources/statistics.js:86
> > +    var tDistributionQuantiles = {
> > +        0.9: [
> > +            3.077684, 1.885618, 1.637744, 1.533206, 1.475884, 1.439756, 1.414924, 1.396815, 1.383029, 1.372184,
> 
> Aren't those the one-tail numbers?

Okay, Ryosuke just showed me. It is the one tail numbers, but he does the computation for a two-tails interval:
    var quantile = (1 - (1 - probability) / 2);

[Ryosuke Niwa]

Comment on attachment 190217 [details]
Also change the default JSON format

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

>>> PerformanceTests/resources/statistics.js:86
>>> +            3.077684, 1.885618, 1.637744, 1.533206, 1.475884, 1.439756, 1.414924, 1.396815, 1.383029, 1.372184,
>> 
>> Aren't those the one-tail numbers?
> 
> That's a good point. tDistributionCDFTail?

Ugh.. what the heck I'm talking about. It should be called tDistributionInverseCDF.

[Ryosuke Niwa]

Created attachment 190419 [details]
Fixed per comments

[Benjamin Poulain]

Comment on attachment 190419 [details]
Fixed per comments

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

I don't like sum as an attribute for sum(values) because it is confusing with Statistics.sum. "sumValues" or "accumulatedValues" maybe?

> PerformanceTests/resources/statistics.js:46
> +    this.sampleStandardDeviation = function (numberOfSamples, sum, squareSum) {

I would have this private (_sampleStandardDeviation?) for now.

> PerformanceTests/resources/statistics.js:61
> +    this.confidenceIntervalDelta = function (confidenceLevel, numberOfSamples, sum, squareSum) {

I would have this private for now (starting with a _?), and have an other function taking values and returning the confidence interval.

> PerformanceTests/resources/statistics.js:75
> +        // d = c * S/sqrt(numberOfSamples) where c ~ t-distribution(degreesOfFreedom) and S is the sample standard deviation.

For clarity? // confidenceInterval = t-distribution@confidenceLevel * standardDeviation / sqrt(numberOfSamples)

> PerformanceTests/resources/statistics.js:77
> +        return deltas[degreesOfFreedom - 1] * this.sampleStandardDeviation(numberOfSamples, sum, squareSum) / Math.sqrt(numberOfSamples);

I think you meant to use delta here instead of deltas[degreesOfFreedom - 1].

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:264
> +        if description:
> +            contents_for_perf_webkit['description'] = description
> +
> +        for key, value in {'buildTime': self._datetime_in_ES5_compatible_iso_format(self._utc_timestamp),
> +            'platform': platform, 'revisions': revisions_for_perf_webkit,
> +            'builderName': builder_name, 'buildNumber': int(build_number) if build_number else None}.items():
> +            if value:
> +                contents_for_perf_webkit[key] = value

Please move the dictionary out of the loop.

Why is description handled differently from the other keys?
Can they really all evaluate to None? In which case would "buildTime" or "platform" be none?

[Benjamin Poulain]

If you continue like that, I'll need to have someone send me my statistics textbooks :)

[Ryosuke Niwa]

Committed r144141: <http://trac.webkit.org/changeset/144141>