Closed Bug 1353239 Opened 7 years ago Closed 7 years ago

stylo: Use a SmallVec to accumulate properties when parsing declaration blocks

Categories

(Core :: CSS Parsing and Computation, enhancement, P4)

enhancement

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: bholley, Assigned: SimonSapin)

References

(Blocks 1 open bug)

Details

This should allow us to eliminate realloc overhead, which is ~40ms on the 100x myspace testcase. See bug 1331843 comment 34.

We should of course measure that this does what we want, and that the added memmov traffic to copy the entries off the stack doesn't cancel out the win.
Note that I did this for selector parsing in bug 1331843, and it was a modest win. We should still investigate for PDB parsing, which is what this bug is about.
https://github.com/servo/servo/pull/16954 uses a stack-allocated ArrayVec during property parsing, then calls Vec::reserve before pushing to the Vec inside PropertyDeclarationBlock. This is all per source `name: value;` declaration though, so it only helps with shorthands that expand to many longhands. (`all` being the extreme case.)

The number of longhands a shorthand expands to is known based on only its name, so instead of pushing to PropertyDeclarationBlock at the end of each `name: value` source declaration we could do so at the *start*, only if the ArrayVec doesn’t have enough available capacity. (Plus of course at the end of the block.) Though keeping all the corner cases working correctly may be tricky. (For example more invalid tokens causing the source declaration to be dropped after we’ve already pushed to ArrayVec.)

(By the way, it’d be nice to have better terminology for "a declaration" before v.s. after shorthand expansion.)
Priority: P1 → P4
So here's a profile on 100x myspace from today:

https://perfht.ml/2g2em4H

So stylo spends about 100ms in functions matching |alloc|, which is a bit better than gecko (which spends 117). Moreover, most of the work seems to be coming from reserve(), with only 17ms coming from |double|. And this is a pretty synthetic testcase. So I doubt there are significant wins remaining here.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.