1424022 - Prefer ids and classes to tag names when collecting ancestor hashes for a selector.

Status: NEW

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

Description

[Emilio Cobos Álvarez (:emilio)]

Props to Antti in trac.webkit.org/projects/webkit/changeset/225596. Let's see if this helps with stylo-chrome perf.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1424022: Prefer ids and classes to tags when collecting ancestor hashes.

(Will run it through talos, just in case it's not an improvement for some
reason.)

Review commit: https://reviewboard.mozilla.org/r/206396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206396/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

(This actually doesn't improve StyleBench that much for us... Let's wait for talos though)

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8935498 [details]
Bug 1424022: Prefer ids and classes to tags when collecting ancestor hashes.

https://reviewboard.mozilla.org/r/206396/#review212000

Looks reasonable. r=me assuming it does improves talos.

(If it doesn't, maybe we should hold back until we find it really improves anything.)

::: servo/components/selectors/parser.rs:243
(Diff revision 1)
> +        for hash in &self.ids {
> +            if slots.push(*hash).is_some() {
> +                return;
> +            }
> +        }
> +
> +        for hash in &self.classes {
> +            if slots.push(*hash).is_some() {
> +                return;
> +            }
> +        }
> +
> +        for hash in &self.tags {
> +            if slots.push(*hash).is_some() {
> +                return;
> +            }
> +        }
> +
> +        for hash in &self.ns {
> +            if slots.push(*hash).is_some() {
> +                return;
> +            }
> +        }

Maybe the code can be more compact with
```rust
let mut iter = self.ids.iter()
    .chain(self.classes.iter())
    .chain(self.tags.iter())
    .chain(self.ns.iter());
for hash in iter {
    if slots.push(*hash).is_some() {
        return;
    }
}
```
?

Not sure whether it would have suboptimal codegen, though.

::: servo/components/selectors/parser.rs:295
(Diff revision 1)
> +            let mut hash_iter = hashes.iter();
> -        for i in 0..4 {
> +            for i in 0..4 {
> -            hashes[i] = match hash_iter.next() {
> +                unpacked_hashes[i] = match hash_iter.next() {
> -                Some(x) => x & BLOOM_HASH_MASK,
> +                    Some(x) => x & BLOOM_HASH_MASK,
> -                None => break,
> +                    None => break,
> -            }
> +                }
> -        }
> +            }

Would something like
```rust
for (unpacked_hash, hash) in unpacked_hash.mut_iter().zip(hashes.iter()) {
    *unpacked_hash = hash & BLOOM_HASH_MASK;
}
```
be better?

If there is any particular reason that using `0..4` is better, that reason should be put in some comment, I guess.

::: servo/components/selectors/parser.rs:673
(Diff revision 1)
>  
>          self.0.next()
>      }
>  }
>  
> +

This seems to be an unrelated change.

Comment 4

[Cameron McCormack (:heycam)]

Did we find out if this helped Talos (and whether it helped stylo-chrome specifically)?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Yeah, I don't think it helped, but worth double-checking the talos results:

  https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=04a885652b63afbfe44358a4e92e68f8b3519b18&newProject=try&newRevision=fb3f2261dc3bad168a744a993025d3fd2b7a6bc1&framework=1