Bug 154673 – Update animation benchmark and tests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jon Lee]

Refine the ramp controller, and use bootstrapping to calculate confidence intervals.

[Jon Lee]

Created attachment 272183 [details]
1. Fix the cursor in the graph analysis when the min  complexity is not 0

[Jon Lee]

Created attachment 272184 [details]
2. Consolidate JS files, and move statistics out to a  separate JS.

[Jon Lee]

Created attachment 272185 [details]
3. Teach the harness to evaluate the samples and determine  the test score.

[Jon Lee]

Created attachment 272186 [details]
4. Allow dropping results JSON.

[Jon Lee]

Created attachment 272187 [details]
5. Fix graph drawing.

[Jon Lee]

Created attachment 272188 [details]
6. Use bootstrapping to get confidence interval in the  breakpoint.

[Jon Lee]

Created attachment 272189 [details]
7. Switch to a pseudo-random number generator.

[Jon Lee]

Created attachment 272190 [details]
8. Update test harness reporting.

[Jon Lee]

Created attachment 272191 [details]
9. Add meta charset so that encodings between harness and  test match.

[Jon Lee]

Created attachment 272192 [details]
10. Have the bootstrap calculation occur between tests.  Clean up harness.

[Jon Lee]

Created attachment 272193 [details]
11. Update the ramp controller.

[Jon Lee]

Created attachment 272194 [details]
12. Tests: update multiply test

[Jon Lee]

Created attachment 272195 [details]
13. Tests: Update distance metric so that fringe tiles get  more color.

[Jon Lee]

Created attachment 272196 [details]
14. Tests: update visuals.

[Jon Lee]

Created attachment 272197 [details]
Merged patch sans Tests changes

[Jon Lee]

Created attachment 272198 [details]
Merged patch of only Tests changes

[Dean Jackson]

Comment on attachment 272197 [details]
Merged patch sans Tests changes

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

Giving this a provisional r+ assuming Said will do an informal review. I'm not going to pretend I understood everything :)

> PerformanceTests/Animometer/resources/debug-runner/animometer.js:31
> +        button.textContent = Strings.text.graph + "...";

Typographic nerds would prefer "…" :)

> PerformanceTests/Animometer/resources/debug-runner/animometer.js:500
> +        if (dashboard.options["adjustment"] == "ramp") {
> +            Headers.details[3].disabled = true;
> +        } else {

Nit: Single line conditional

> PerformanceTests/Animometer/resources/debug-runner/graph.js:9
> +        element.testResult = testResult;
> +        element.testData = testData;

More of a general comment, but I wonder if we should prefix these custom attrs/properties with _

> PerformanceTests/Animometer/resources/debug-runner/graph.js:595
> +            document.getElementById("complexity-regression-aggregate-raw").textContent = complexityRegression.complexity.toFixed(2) + ", ±" + complexityRegression.stdev.toFixed(2) + "ms";

I guess this is just a review tool bug - supposed to be the +/- sign?

> PerformanceTests/Animometer/resources/debug-runner/graph.js:603
> +                "â",

Same here, but not sure what it is supposed to be.

> PerformanceTests/Animometer/resources/extensions.js:467
> +        for (var i = 0; i < this._size; ++i) {
> +            out += this._values[i];
> +            if (i < this._size - 1)
> +                out += ", ";
> +        }

Can you use this._values.join(", ") here?

$ jsc
>>> [1, 2, 3, 4, 5].join(", ")
1, 2, 3, 4, 5

[Jon Lee]

Comment on attachment 272197 [details]
Merged patch sans Tests changes

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

>> PerformanceTests/Animometer/resources/debug-runner/animometer.js:31
>> +        button.textContent = Strings.text.graph + "...";
> 
> Typographic nerds would prefer "…" :)

Done.

>> PerformanceTests/Animometer/resources/debug-runner/animometer.js:500
>> +        } else {
> 
> Nit: Single line conditional

Done.

>> PerformanceTests/Animometer/resources/debug-runner/graph.js:9
>> +        element.testData = testData;
> 
> More of a general comment, but I wonder if we should prefix these custom attrs/properties with _

We could. Done.

>> PerformanceTests/Animometer/resources/debug-runner/graph.js:595
>> +            document.getElementById("complexity-regression-aggregate-raw").textContent = complexityRegression.complexity.toFixed(2) + ", ±" + complexityRegression.stdev.toFixed(2) + "ms";
> 
> I guess this is just a review tool bug - supposed to be the +/- sign?

Yes, it is.

>> PerformanceTests/Animometer/resources/debug-runner/graph.js:603
>> +                "â",
> 
> Same here, but not sure what it is supposed to be.

en dash.

>> PerformanceTests/Animometer/resources/extensions.js:467
>> +        }
> 
> Can you use this._values.join(", ") here?
> 
> $ jsc

I didn't touch this code; I was just moving it, but yes we could do that.

[Jon Lee]

Committed r197229: <http://trac.webkit.org/changeset/197229>

[Jon Lee]

Committed r197230: <http://trac.webkit.org/changeset/197230>

[Jon Lee]

follow-up with Dean's comments
Committed r197232: <http://trac.webkit.org/changeset/197232>