Bug 180140 – Add StyleBench

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

[Antti Koivisto]

StyleBench is a benchmark for testing style resolution performance.

[Antti Koivisto]

Created attachment 327854 [details]
patch

[Antti Koivisto]

<rdar://problem/35366401>

[Simon Fraser (smfr)]

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.

[Joseph Pecoraro]

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`.

[Antti Koivisto]

> 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!

[Antti Koivisto]

Created attachment 327967 [details]
patch

[Antti Koivisto]

> > PerformanceTests/StyleBench/InteractiveRunner.html:21

I didn't do any fixes to this copy-code from Speedometer.

[WebKit Commit Bot]

Comment on attachment 327967 [details]
patch

Clearing flags on attachment: 327967

Committed r225324: <https://trac.webkit.org/changeset/225324>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[Antti Koivisto]

Skipped InteractiveRunner.html in https://trac.webkit.org/r225392