Bug 110842

Created attachment 190200 [details] Updated the results page template

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

(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 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 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 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 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 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 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.

> 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 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.

Created attachment 190419 [details] Fixed per comments

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?

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

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