StyleBench is a benchmark for testing style resolution performance.
Created attachment 327854 [details] patch
<rdar://problem/35366401>
Comment on attachment 327854 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=327854&action=review > PerformanceTests/StyleBench/InteractiveRunner.html:21 > +function formatTestName(suiteName, testName) { Opening function parens on new line please (everywhere) > PerformanceTests/StyleBench/InteractiveRunner.html:42 > + label.appendChild(document.createTextNode(formatTestName(suite.name))); or just set textContent? > PerformanceTests/StyleBench/InteractiveRunner.html:53 > + anchor.appendChild(document.createTextNode(formatTestName(suite.name, test.name))); Set textContent? > PerformanceTests/StyleBench/resources/style-bench.js:26 > +function nextAnimationFrame() { return new Promise((resolve) => requestAnimationFrame(resolve)); } No reason to have this all on one line.
Comment on attachment 327854 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=327854&action=review Neat! I skipped the test runner stuff but the rest looked good to me. > PerformanceTests/StyleBench/resources/style-bench.js:32 > + const configuration = { Style: You could just `return` instead of using the local which you immediately return. > PerformanceTests/StyleBench/resources/style-bench.js:79 > + let configuration = this.defaultConfiguration(); > + configuration.name = 'Sibling combinators'; > + configuration.combinators = [ > + ' ', > + ' ', > + '>', > + '>', > + '~', > + '+', > + ]; > + return configuration; Another way to write these configuration functions would be to use `Object.assign` to add/override additional properties on the default configuration. Like so: return Object.assign(this.defaultConfiguration(), { name: 'Sibling combinators', combinators: [' ', ' ', '>', '>', '~', '+'], }); But use the approach you find easiest to read! Also there is an inconsistent mix of single quoted and double quoted strings. We normally prefer to be consistent within a single file. > PerformanceTests/StyleBench/resources/style-bench.js:116 > + ]; I'd be interested to see one that includes `:matches()`, `:all()`, `:not()` pseudo classeses in the future! We make use of that a lot in Web Inspector. If there was one for `var()` where custom properties have different values after mutation would that spend most of its time in style resolution as well, or not to the same degree? > PerformanceTests/StyleBench/resources/style-bench.js:299 > + Style: Unnecessary blank line. > PerformanceTests/StyleBench/resources/style-bench.js:348 > + const classList = element.classList; > + classList.add(this.randomClassName()); > + > + const canRemove = classList.length; > + if (!canRemove) > + continue; Because line 344 just added a class name it seems `classList.length` would never be 0, right? So I think you can drop lines 346-348 and even make this a straight for loop by moving the increment into the for loop header. Or perhaps line 344 is an accident of a copy/paste from addClasses? > PerformanceTests/StyleBench/resources/tests.js:22 > + const suite = { Style: Same as before, you can drop the temporary local and just `return`.
> Also there is an inconsistent mix of single quoted and double quoted > strings. We normally prefer to be consistent within a single file. Do we have a preference? > > PerformanceTests/StyleBench/resources/style-bench.js:116 > > + ]; > > I'd be interested to see one that includes `:matches()`, `:all()`, `:not()` > pseudo classeses in the future! We make use of that a lot in Web Inspector. Yeah. It will be easy to add more cases. Thanks for the comments!
Created attachment 327967 [details] patch
> > PerformanceTests/StyleBench/InteractiveRunner.html:21 I didn't do any fixes to this copy-code from Speedometer.
Comment on attachment 327967 [details] patch Clearing flags on attachment: 327967 Committed r225324: <https://trac.webkit.org/changeset/225324>
All reviewed patches have been landed. Closing bug.
StyleBench is failing on the perf bot: Running StyleBench/InteractiveRunner.html (184 of 184) ERROR: layer at (0,0) size 785x630 FAILED Finished: 0.873694 s https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20%28Perf%29/builds/1742
Skipped InteractiveRunner.html in https://trac.webkit.org/r225392