1358375 - stylo: Precompute the existence of declarations

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

Right now we do a bunch of dumb pointer chasing to figure out if the declaration has any rules of the given cascade level (and thus whether we can avoid matching selectors), which turns out to be much more expensive than the selector matching itself (at least it is now, after all the other selector matching optimizations I've landed).

Storing these bools inline gives us a 5x speedup single-threaded non-stylesharing performance on the testcase in bug 1348935, causing us to spend only 77ms in get_all_matching_rules, which is about 2.5x better than gecko on the same testcase.

Comment 1

[Bobby Holley (:bholley)]

(I still need to write a microbenchmark for this to check into Talos).

Comment 2

[Bobby Holley (:bholley)]

Attachment: Avoid pointer-chasing to check whether there are any declarations. v1

Simon, can you confirm that we definitely flush the stylist if somebody
modifies the declarations over CSSOM? Otherwise these bools could get stale.

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

specificity actually only uses 30bit for specificity, which means there are still 2 bits available there, and thus there makes little sense to add another 32bit size to the struct just for two bools, I guess.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Huh.  So servo does selector matching twice for a rule that has both !important and non-!important declarations?

Fwiw, the way Gecko handles this is that it does matching only once, when doing the non-!important pass.  Then when it needs the !important bits it walks the relevant ruletree bit and asks the rules for their !important pieces.  Hard to say which is better in practice.

Comment 6

[Simon Sapin (:SimonSapin)]

> Simon, can you confirm that we definitely flush the stylist if somebody
> modifies the declarations over CSSOM? Otherwise these bools could get stale.

As far as I know we do not. So yes, it does sound like these bools would need to be updated.

I think this can be tested easily: use getComputedStyle to do a style flush, then use CSSOM to add an !important declaration to a style rule that didn’t have any before, then use getComputedStyle again and assert that some computed value changed.


> Huh.  So servo does selector matching twice for a rule that has both !important and non-!important declarations?

Yes, because that seemed like the only way to do it without allocating a Vec of matching rules when we didn’t have a rule tree. Now that we do, we can probably do something like what you describe.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

If wins are so huge as bug 1348935 comment 12 claims, perhaps we could consider just flushing stylesheets when those booleans change, which I expect it to be an uncommon subset of all declaration mutations.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Just a side note, since there seemed to be some confusion over a discussion I read on the IRC backlog: These changes do _not_ affect style attributes at all. Those are pulled from the element in push_applicable_declarations, and are checked there in-place for normal/important rules.

When a style attribute changes, only the relevant part of the rule tree is rebuilt, without re-running selector-matching on the element. If a new important property is added, we'll add a new rule node.

Since the rule objects are only for actual style rules, we don't need to invalidate the stylist when the style attribute changes, only when a stylesheet rule does (and only when the any_important/any_normal status changes, which is what I suggest in comment 7).

Comment 9

[Bobby Holley (:bholley)]

I added a microbenchmark for this in https://github.com/heycam/style-perf-tests/pull/2

Comment 10

[Bobby Holley (:bholley)]

So, it looks like we will in fact call Servo_StyleSet_NoteStyleSheetsChanged when the rule is modified. I'll add some reftests to make sure this doesn't break.

Comment 11

[Bobby Holley (:bholley)]

Attachment: Reftest for refreshing the cached booleans. v1

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8860515 [details] [diff] [review]
Reftest for refreshing the cached booleans. v1

Might be good to have a third test, where we go from empty rule to !important.

r=me either way.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #4)
> specificity actually only uses 30bit for specificity, which means there are
> still 2 bits available there, and thus there makes little sense to add
> another 32bit size to the struct just for two bools, I guess.

Nice idea. Switching to packed bits make the microbenchmark ~3% slower (presumably because testing a bit is more expensive than testing a byte), but I think that's acceptable given the memory impact. I'll switch to that.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Avoid pointer-chasing to check whether there are any declarations. v2

Comment 15

[Bobby Holley (:bholley)]

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bcac887a39da
Reftest for refreshing the cached booleans. r=bz

Comment 17

[Bobby Holley (:bholley)]

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

Comment 18

[Simon Sapin (:SimonSapin)]

> Switching to packed bits make the microbenchmark ~3% slower (presumably because testing a bit is more expensive than testing a byte)

I think packing doesn’t make a difference on 64-bit platforms in this case because the struct’s size is rounded up to three words anyway. I don’t know if packing on 32-bit only makes sense.

Comment 19

[Bobby Holley (:bholley)]

(In reply to Simon Sapin (:SimonSapin) from comment #18)
> > Switching to packed bits make the microbenchmark ~3% slower (presumably because testing a bit is more expensive than testing a byte)
> 
> I think packing doesn’t make a difference on 64-bit platforms in this case
> because the struct’s size is rounded up to three words anyway. I don’t know
> if packing on 32-bit only makes sense.

I think the difference is probably marginal enough that it doesn't justify the complexity (if it were 3% across the board it'd be a different story). So I'm going to wait and see if this stuff continues to show up in profiling, and reconsider if it does.

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/bcac887a39da