stylo: Rearrange PropertyDeclaration to save a word

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Assigned: Bobby Holley (parental leave - send mail for anything urgent))

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Right now PropertyDeclaration embeds DeclaredValue, which means that we have two discriminants padding each actual specified value. This means that PropertyDeclaration is 48 bytes even though the larged specified value is 32 bytes.

I've determined that reducing the size of PropertyDeclaration boosts parsing performance, persumably because we're memmoving less. We can certainly consider boxing more properties and shrinking the maximum specified value size more, but I wanted to see if I could make this work.

It ended up being a bit more involved than I was hoping, but still doable. I'll upload a patch.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd5b0fcee89e160fb60d0d14748b82defba308be
I’m not sure if you’re planning to try this, or have done it and meant to attach a patch.

I’m guessing that the way to do it is tripling the number of variants of PropertyDeclaration, to cover all combinations of its current variants and DeclaredValue variants. (I’ve done something similar in ParsedDeclaration.)


This made me think of the PropertyDeclaration::id method. It effectively re-implements std::intrinsics::discriminant_value with a large match expression. I was hoping it would optimize to almost a no-op, but apparently that’s not the case. I don’t completely understand the assembly code, but I think it’s based on a lookup table and takes O(1) time. I think your change would make the table larger, but id() should still take O(1) time.
(In reply to Simon Sapin (:SimonSapin) from comment #2)
> I’m not sure if you’re planning to try this, or have done it and meant to
> attach a patch.

I have a patch that mostly works, but it's orange on try - going to try to look into that today.

> 
> I’m guessing that the way to do it is tripling the number of variants of
> PropertyDeclaration, to cover all combinations of its current variants and
> DeclaredValue variants. (I’ve done something similar in ParsedDeclaration.)

Not quite - I just added two variants, each of which takes a (LonghandId, _) tuple.

> This made me think of the PropertyDeclaration::id method. It effectively
> re-implements std::intrinsics::discriminant_value with a large match
> expression. I was hoping it would optimize to almost a no-op, but apparently
> that’s not the case. I don’t completely understand the assembly code, but I
> think it’s based on a lookup table and takes O(1) time. I think your change
> would make the table larger, but id() should still take O(1) time.

Yeah - in the with statements, we just pull the LonghandId directly out of the tuple.

I have seen some calls to id() showing up in profiles though, which makes me suspicious that it's really O(1).
I *think* id() compiles to a computed jump, and https://github.com/servo/servo/pull/15992 makes it a lookup table in 1.17. Both should be O(1) instructions, but maybe the former has bad cache locality when fetching code?
Green try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4323fcaf6a11e36a1da37fc06f4b127c08d83801
Created attachment 8848210 [details] [diff] [review]
Rearrange PropertyDeclaration to avoid embedding DeclaredValue. v1

This effectively combines the discriminants of the two enums and reduces the
size of PropertyDeclaration by one word.

MozReview-Commit-ID: 9rCRiSVZTQT
Attachment #8848210 - Flags: review?(simon.sapin)
Comment on attachment 8848210 [details] [diff] [review]
Rearrange PropertyDeclaration to avoid embedding DeclaredValue. v1

Simon says he's swamped, forwarding to matt. Feel free to bounce again if you don't have time.
Attachment #8848210 - Flags: review?(simon.sapin) → review?(mbrubeck)
This improves parsing performance on the myspace testcase by about 5%. \o/
Comment on attachment 8848210 [details] [diff] [review]
Rearrange PropertyDeclaration to avoid embedding DeclaredValue. v1

Review of attachment 8848210 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!
Attachment #8848210 - Flags: review?(mbrubeck) → review+
https://github.com/servo/servo/pull/15997
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.