Bug 110842 - Use perf.webkit.org JSON format in results page
Summary: Use perf.webkit.org JSON format in results page
Status: RESOLVED FIXED
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: 111145
  Show dependency treegraph
 
Reported: 2013-02-25 21:25 PST by Ryosuke Niwa
Modified: 2013-02-28 23:36 PST (History)
8 users (show)

See Also:


Attachments
Updated the results page template (23.49 KB, patch)
2013-02-25 21:43 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Also change the default JSON format (41.19 KB, patch)
2013-02-25 23:49 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed per comments (41.33 KB, patch)
2013-02-26 19:11 PST, Ryosuke Niwa
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-02-25 21:25:17 PST
Now that we're uploading results to perf.webkit.org using new JSON format, we should use the same format in results page.
Comment 1 Ryosuke Niwa 2013-02-25 21:43:09 PST
Created attachment 190200 [details]
Updated the results page template
Comment 2 Ryosuke Niwa 2013-02-25 23:49:48 PST
Created attachment 190217 [details]
Also change the default JSON format
Comment 3 Ryosuke Niwa 2013-02-25 23:52:32 PST
(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.
Comment 4 Dirk Pranke 2013-02-26 15:38:24 PST
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?
Comment 5 Benjamin Poulain 2013-02-26 15:40:53 PST
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?
Comment 6 Ryosuke Niwa 2013-02-26 15:42:34 PST
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
Comment 7 Ryosuke Niwa 2013-02-26 15:44:48 PST
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?
Comment 8 Dirk Pranke 2013-02-26 15:47:00 PST
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 :).
Comment 9 Ryosuke Niwa 2013-02-26 15:50:56 PST
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.
Comment 10 Benjamin Poulain 2013-02-26 15:52:09 PST
> 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);
Comment 11 Ryosuke Niwa 2013-02-26 15:59:50 PST
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.
Comment 12 Ryosuke Niwa 2013-02-26 19:11:54 PST
Created attachment 190419 [details]
Fixed per comments
Comment 13 Benjamin Poulain 2013-02-26 20:28:59 PST
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?
Comment 14 Benjamin Poulain 2013-02-26 20:29:42 PST
If you continue like that, I'll need to have someone send me my statistics textbooks :)
Comment 15 Ryosuke Niwa 2013-02-26 21:03:58 PST
Committed r144141: <http://trac.webkit.org/changeset/144141>