Summary: | Update benchmark test suite | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jon Lee <jonlee> | ||||||||||||||
Component: | Animations | Assignee: | Jon Lee <jonlee> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, dino, rniwa, sabouhallawa, simon.fraser, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Jon Lee
2016-01-03 18:13:58 PST
Created attachment 268232 [details]
1. Fix tests for other browsers.
Created attachment 268233 [details]
2. Move algorithm.js and sampler.js to tests/ and benchmark-runner.js to runner/.
Created attachment 268234 [details]
3. Add a new test.
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"? 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. All looks good to me. Unofficial r=me. 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 (In reply to comment #4) > Created attachment 268234 [details] > 3. Add a new test. I will check in a version with the corrected ChangeLog. Created attachment 268524 [details]
3. Add new test (Take 2)
Created attachment 268526 [details]
1. Fix tests for other browsers (Take 2)
Created attachment 268527 [details]
2. Move algorithm.js and sampler.js (Take 2)
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? 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? 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? (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. (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. (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. 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> Committed r194753: <http://trac.webkit.org/changeset/194753> (In reply to comment #20) > Committed r194753: <http://trac.webkit.org/changeset/194753> This is patch #2. Committed r194755: <http://trac.webkit.org/changeset/194755> fix in r194756 |