Bug 98115 – PerfTests have incorrect standard deviation calculation

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Philip Rogers]

Our performance test numbers calculate standard deviation incorrectly:

(wrong)
RESULT PageLoad: svg: files: worldcup.svg= 2197.27248895 ms
median= 2157.99593925 ms, stdev= 693.92660072 ms, min= 2127.62403488 ms, max= 2865.42391777 ms

(right)
RESULT PageLoad: svg: files: worldcup.svg= 2177.03914642 ms
median= 2146.1250782 ms, stdev= 162.849482453 ms, min= 2105.28707504 ms, max= 2840.33107758 ms

[Philip Rogers]

Created attachment 166608 [details]
Fix PerfTest standard deviation calculation.

[Ryosuke Niwa]

Comment on attachment 166608 [details]
Fix PerfTest standard deviation calculation.

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

Could you also change the algorithm in runner.js? I've always wanted to fix that algorithm.

> Tools/Scripts/webkitpy/performance_tests/perftest.py:236
> +            'stdev': math.sqrt(squareSum / (len(sorted_test_times) - 1)),

So we're now computing the sample standard deviation as opposed to the sample deviation of the sample:
http://en.wikipedia.org/wiki/Standard_deviation#Estimation

You should explain that in the change log.

[Ryosuke Niwa]

Comment on attachment 166608 [details]
Fix PerfTest standard deviation calculation.

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

> Tools/Scripts/webkitpy/performance_tests/perftest.py:227
>              delta = time - mean
>              sweep = i + 1.0
>              mean += delta / sweep

We can rewrite this as
mean += (time - mean) / sweep
and eliminate delta now.

[Philip Rogers]

Created attachment 166621 [details]
Update per reviewer comments

[Philip Rogers]

(In reply to comment #2)
> (From update of attachment 166608 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166608&action=review
> 
> Could you also change the algorithm in runner.js? I've always wanted to fix that algorithm.

Done :)

> 
> > Tools/Scripts/webkitpy/performance_tests/perftest.py:236
> > +            'stdev': math.sqrt(squareSum / (len(sorted_test_times) - 1)),
> 
> So we're now computing the sample standard deviation as opposed to the sample deviation of the sample:
> http://en.wikipedia.org/wiki/Standard_deviation#Estimation
> 
> You should explain that in the change log.

Done

[Philip Rogers]

(In reply to comment #3)
> (From update of attachment 166608 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166608&action=review
> 
> > Tools/Scripts/webkitpy/performance_tests/perftest.py:227
> >              delta = time - mean
> >              sweep = i + 1.0
> >              mean += delta / sweep
> 
> We can rewrite this as
> mean += (time - mean) / sweep
> and eliminate delta now.

I'm not sure it's quite that simple (we need to store the mean before and after the update). This isn't performance-sensitive code and I think the readability (copied directly from Knuth) is useful here for future statistics hackers.

[Philip Rogers]

Comment on attachment 166621 [details]
Update per reviewer comments

Clearing flags since patch doesn't apply. Let me rebaseline after rniwa's change today and reupload.

[Philip Rogers]

Created attachment 166623 [details]
Rebaselined

[Ryosuke Niwa]

Comment on attachment 166623 [details]
Rebaselined

Please include the URL to the algorithm you cited.

[Ryosuke Niwa]

*** Bug 77401 has been marked as a duplicate of this bug. ***

[Philip Rogers]

Created attachment 166626 [details]
Added link to algorithm in the ChangeLog

[WebKit Review Bot]

Comment on attachment 166626 [details]
Added link to algorithm in the ChangeLog

Clearing flags on attachment: 166626

Committed r130135: <http://trac.webkit.org/changeset/130135>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #12)
> (From update of attachment 166626 [details])
> Clearing flags on attachment: 166626
> 
> Committed r130135: <http://trac.webkit.org/changeset/130135>

FYI: It broke the perf tests again. See the bots for details.

[Csaba Osztrogonác]

Fix landed in http://trac.webkit.org/changeset/130181

[Philip Rogers]

(In reply to comment #15)
> Fix landed in http://trac.webkit.org/changeset/130181

Should we also make this change in the python standard deviation method?

[Ryosuke Niwa]

(In reply to comment #16)
> (In reply to comment #15)
> > Fix landed in http://trac.webkit.org/changeset/130181
> 
> Should we also make this change in the python standard deviation method?

That's a good point. We should do that although PageLoadTest always gets 20 values.