[webkit-reviews] review granted: [Bug 110842] Use perf.webkit.org JSON format in results page : [Attachment 190419] Fixed per comments

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Ryosuke Niwa
<rniwa at webkit.org>'s request for review:
Bug 110842: Use perf.webkit.org JSON format in results page
https://bugs.webkit.org/show_bug.cgi?id=110842

Attachment 190419: Fixed per comments
https://bugs.webkit.org/attachment.cgi?id=190419&action=review

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