Bug 152679 – Update benchmark test suite

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Jon Lee]

Update tests.

[Radar WebKit Bug Importer]

<rdar://problem/24034507>

[Jon Lee]

Created attachment 268232 [details]
1. Fix tests for other browsers.

[Jon Lee]

Created attachment 268233 [details]
2. Move algorithm.js and sampler.js to tests/ and benchmark-runner.js to runner/.

[Jon Lee]

Created attachment 268234 [details]
3. Add a new test.

[Said Abou-Hallawa]

Comment on attachment 268233 [details]
2. Move algorithm.js and sampler.js to tests/ and benchmark-runner.js to runner/.

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

> PerformanceTests/ChangeLog:16
> +        into utilities.js.

Did you mean "into extensions.js"?

[Said Abou-Hallawa]

Comment on attachment 268234 [details]
3. Add a new test.

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

> PerformanceTests/Animometer/resources/extensions.js:96
> +            return new Point(this.x + other, this.y + other);

I am not sure if adding a scalar value to a point/size is something meaningful.

[Said Abou-Hallawa]

All looks good to me. Unofficial r=me.

[Jon Lee]

Comment on attachment 268233 [details]
2. Move algorithm.js and sampler.js to tests/ and benchmark-runner.js to runner/.

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

>> PerformanceTests/ChangeLog:16
>> +        into utilities.js.
> 
> Did you mean "into extensions.js"?

Yes

[Jon Lee]

(In reply to comment #4)
> Created attachment 268234 [details]
> 3. Add a new test.

I will check in a version with the corrected ChangeLog.

[Jon Lee]

Created attachment 268524 [details]
3. Add new test (Take 2)

[Jon Lee]

Created attachment 268526 [details]
1. Fix tests for other browsers (Take 2)

[Jon Lee]

Created attachment 268527 [details]
2. Move algorithm.js and sampler.js (Take 2)

[Simon Fraser (smfr)]

Comment on attachment 268526 [details]
1. Fix tests for other browsers (Take 2)

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

> PerformanceTests/ChangeLog:14
> +        (CanvasElectron.prototype._draw): Some browsers don't support ellipse.

Which ones?

[Simon Fraser (smfr)]

Comment on attachment 268526 [details]
1. Fix tests for other browsers (Take 2)

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

> PerformanceTests/Animometer/resources/extensions.js:70
> +    var rect = element.getBoundingClientRect();
> +    return new Point(rect.width, rect.height);

Are you sure this doesn't cause layout thrashing?

[Simon Fraser (smfr)]

Comment on attachment 268524 [details]
3. Add new test (Take 2)

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

> PerformanceTests/Animometer/resources/extensions.js:96
> +        if(isNaN(other.x))
> +            return new Point(this.x + other, this.y + other);

Weird but ok!

> PerformanceTests/Animometer/tests/master/resources/dom-particles.js:25
> +        this.element.style.backgroundColor = "hsl(" + (((Date.now()/2000)%1)*360) + ", 70%, 45%)";

Are the outer parens around Date.now()... necessary?

[Jon Lee]

(In reply to comment #13)
> Comment on attachment 268526 [details]
> 1. Fix tests for other browsers (Take 2)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268526&action=review
> 
> > PerformanceTests/ChangeLog:14
> > +        (CanvasElectron.prototype._draw): Some browsers don't support ellipse.
> 
> Which ones?

Firefox.

[Jon Lee]

(In reply to comment #14)
> Comment on attachment 268526 [details]
> 1. Fix tests for other browsers (Take 2)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268526&action=review
> 
> > PerformanceTests/Animometer/resources/extensions.js:70
> > +    var rect = element.getBoundingClientRect();
> > +    return new Point(rect.width, rect.height);
> 
> Are you sure this doesn't cause layout thrashing?

This function is only called when the stage is initialized, and for graphing. It is not called during the benchmark run.

I wouldn't expect any benchmark to make a call to this because it should be caching its size.

[Jon Lee]

(In reply to comment #15)
> Comment on attachment 268524 [details]
> 3. Add new test (Take 2)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268524&action=review
> 
> > PerformanceTests/Animometer/resources/extensions.js:96
> > +        if(isNaN(other.x))
> > +            return new Point(this.x + other, this.y + other);
> 
> Weird but ok!
> 
> > PerformanceTests/Animometer/tests/master/resources/dom-particles.js:25
> > +        this.element.style.backgroundColor = "hsl(" + (((Date.now()/2000)%1)*360) + ", 70%, 45%)";
> 
> Are the outer parens around Date.now()... necessary?

No. This calculation is done enough that I'll probably factor it out anyway in a future patch.

[WebKit Commit Bot]

Comment on attachment 268526 [details]
1. Fix tests for other browsers (Take 2)

Clearing flags on attachment: 268526

Committed r194752: <http://trac.webkit.org/changeset/194752>

[Jon Lee]

Committed r194753: <http://trac.webkit.org/changeset/194753>

[Jon Lee]

(In reply to comment #20)
> Committed r194753: <http://trac.webkit.org/changeset/194753>

This is patch #2.

[Jon Lee]

Committed r194755: <http://trac.webkit.org/changeset/194755>

[Jon Lee]

fix in r194756