1364148 - stylo: shrink selector components

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

I just had a look and found that these are definitely bigger than they need to be. Shrinking these should speed up both parsing and selector matching.

Looking at [1], it looks like AttrEqual is probably the long pole. We have:
* the discriminant
* The name
* the lowername
* The NamespaceConstraint discriminant (since we can't use the NonZero optimization in stable rust on GeckoAtom).
* The namespace word
* The attrvalue
* The CaseSensitivity

So that's 7 words if I count right. I think we can shave off at least 2: The CaseSensitivity, and the NamespaceConstraint discriminant. We could use extra Component variants if we need to (though that risks exponential blowup), or we could pack low bits somewhere. This would definitely work for Gecko atoms (where the points have 2 low bits we can steal), though I'm not sure if it would work for Servo atoms (which IIRC are packed more tightly).

Simon, WDYT? Do you have cycles to look at this?

[1] http://searchfox.org/mozilla-central/rev/d66b9f27d5630a90b2fce4d70d4e9050f43df9b4/servo/components/selectors/parser.rs#433

Comment 1

[Bobby Holley (:bholley)]

Also, we should add a size_of test to lock down the size once we get it where we want.

Comment 2

[Simon Sapin (:SimonSapin)]

Looking into this now. Servo’s existing size_of() tests assume a 64-bit platform, so I’ll do the same here.

I’m adding size unit tests to the selectors crate. To make them representative of Stylo, I’m making dummy PseudoClass, PseudoElement and Atom types that have similar size and alignment as the real types in style/gecko/selector_parser.rs.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Note that the pseudo-element representation is going to change in bug 1364412, though.

Comment 4

[Bobby Holley (:bholley)]

Thanks Simon! Optimizing for 64-bit is fine.

Comment 5

[Simon Sapin (:SimonSapin)]

(It’s not just optimizing, these unit tests would fail on 32-bit. Though I support I can add a #[cfg] attribute to disable them there.)


The idea I’m trying now is using *two* entries in Vec<Component> for attribute selectors in some (hopefully less common) cases: with a namespace prefix, with a namespace wildcard, or with an explicit `i` case-insensitivity marker. I think this can bring size_of::<Component>() down to 4 words, at which point Stylo’s PseudoClass also becomes a limiting factor since it’s 3 words: the PseudoClass discriminant, the pointer of Box<[u16]>, and the length of Box<[u16]>.

This means that "components" would no longer be independent, matching cannot use .iter().any(…) anymore. To make consumers less likely to get this wrong I’d prefer to make Component private to the selectors crate, but that seems difficult since it has multiple outside uses, with even a dedicated visit_simple_selector method.

Comment 6

[Simon Sapin (:SimonSapin)]

This morning’s realizations:

* Servo’s CI, including rust-selectors size_of unit tests, run on rust nightly (currently 1.19.0). Since 1.18 Rust reorders fields to minimize padding, which of course affects size_of. 1.18 should reach the stable channel in mid-June. In the mean-time, consider using beta or nigthly for benchmarks that could be affected by the size of Rust types. Gecko’s rustc_min_version is currently 1.15.1.

* A simplified `enum Component { Foo, Bar(usize, u8) }` can be two words instead of three: the u8 field can be moved into the same word as the discriminant, only the usize field needs to be word-aligned. I can manually reorder fields to get this size reduction in order versions, but I don’t know if it’s worth the bother: most people/machines will be on 1.18 soon enough. However this doesn’t work with `enum Component { Foo, Bar(struct Baz(u8, usize)) }` on any rust version, since Baz is an independent type whose size is rounded up to a multiple of its alignment.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Simon Sapin (:SimonSapin) from comment #6)
> This morning’s realizations:
> 
> * Servo’s CI, including rust-selectors size_of unit tests, run on rust
> nightly (currently 1.19.0). Since 1.18 Rust reorders fields to minimize
> padding, which of course affects size_of. 1.18 should reach the stable
> channel in mid-June. In the mean-time, consider using beta or nigthly for
> benchmarks that could be affected by the size of Rust types. Gecko’s
> rustc_min_version is currently 1.15.1.
> 
> * A simplified `enum Component { Foo, Bar(usize, u8) }` can be two words
> instead of three: the u8 field can be moved into the same word as the
> discriminant, only the usize field needs to be word-aligned.

Oh great! So the discriminant type for an enum with <=256 variants will be a u8? I always thought it was an unconditional usize. That's great news in general.

> I can manually
> reorder fields to get this size reduction in order versions, but I don’t
> know if it’s worth the bother: most people/machines will be on 1.18 soon
> enough. However this doesn’t work with `enum Component { Foo, Bar(struct
> Baz(u8, usize)) }` on any rust version, since Baz is an independent type
> whose size is rounded up to a multiple of its alignment.

Ok. Does it make sense to optimize the size now for 1.18, and just run the tests only on servo (and not on stylo) until Gecko gets in 1.18?

Comment 8

[Simon Sapin (:SimonSapin)]

> Oh great! So the discriminant type for an enum with <=256 variants will be a u8? I always thought it was an unconditional usize.

Yes, by default (without #[repr(…)] on an enum type) the discriminant is the smallest of u* type it can be. But for example in Option<usize> there’s enough padding next to the discriminant that it’s indistinguishable from #[repr(usize)].

Regarding testing 1.18+, I agree. I mentioned this also to keep in mind when taking benchmarks manually. I’ve just run `rustup override set nightly` in my gecko trees. (1.18 beta 2 is broken: bug 1365300, nightly since ~last week has a new warning that we #[deny] and that https://github.com/servo/servo/pull/16897 fixes.)


By the way, I’ve already landed a couple of preliminary PRs:

* https://github.com/servo/servo/pull/16870
* https://github.com/servo/servo/pull/16890

And opened one to fix a spec bug:

* https://github.com/w3c/csswg-drafts/pull/1387

Comment 9

[Simon Sapin (:SimonSapin)]

https://github.com/servo/servo/pull/16915 brings the size of Component from 64 bytes to 32. It uses a Box for attribute selector whose name have a namespace wildcard `*|foo` or prefix `pfx|foo`.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Simon Sapin (:SimonSapin) from comment #9)
> https://github.com/servo/servo/pull/16915 brings the size of Component from
> 64 bytes to 32. It uses a Box for attribute selector whose name have a
> namespace wildcard `*|foo` or prefix `pfx|foo`.

Nice work! :-)

Comment 11

[Simon Sapin (:SimonSapin)]

https://github.com/servo/servo/pull/16915 has landed.