1347719 - stylo: Rearrange PropertyDeclaration to save a word

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd5b0fcee89e160fb60d0d14748b82defba308be

Comment 2

[Simon Sapin (:SimonSapin)]

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.

Comment 3

[Bobby Holley (:bholley)]

(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).

Comment 4

[Simon Sapin (:SimonSapin)]

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?

Comment 5

[Bobby Holley (:bholley)]

Green try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4323fcaf6a11e36a1da37fc06f4b127c08d83801

Comment 6

[Bobby Holley (:bholley)]

Attachment: 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

Comment 7

[Bobby Holley (:bholley)]

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.

Comment 8

[Bobby Holley (:bholley)]

This improves parsing performance on the myspace testcase by about 5%. \o/

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8848210 [details] [diff] [review]
Rearrange PropertyDeclaration to avoid embedding DeclaredValue. v1

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

Nice!

Comment 10

[Bobby Holley (:bholley)]

https://github.com/servo/servo/pull/15997