stylo: track the attributes and state that a DependencySet is sensitive to, and use it to cull snapshot creation

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
2 months ago
16 days ago

People

(Reporter: bz, Assigned: heycam)

Tracking

(Blocks: 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 affected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

ServoRestyleManager::AttributeWillChange/AttributeChanged call Servo_Element_GetSnapshot, which calls maybe_restyle, which calls     bindings::Gecko_SetOwnerDocumentNeedsStyleFlush(element.0) if the element has been styled.

I guess the tradeoff here is that we don't have to check whether this particular attr is relevant.  But it still means we can end up with moderately expensive thrashing in some cases; a PresShell flush is not that cheap even if in practice it turns out there is nothing to restyle.  And that's with Gecko; I haven't even measured with servo, but I suspect it takes servo longer to decide "there is nothing to restyle", given what I understand of how it works.

Can we at least restrict this to cases where the attribute involved is at least _mentioned_ in the stylesheet?
s/the stylesheet/a stylesheet/ of course.
So, the reason we went with the snapshotting approach for servo was that the servo script thread doesn't have sync access to the style system, so it can't do the HasAttrDependentStyle check during AttrWillChange/AttrChanged. Things are different with stylo though, so we _could_ implement the system exactly as Gecko does if we wanted to.

The advantage of the snapshotting approach is that we don't have to match partial selectors on each attr change (smaug said that HasAttrDependentStyle is hot in Gecko), and that it coalesces multiple changes in ways that can potentially reduce style work (i.e. if we briefly :hover and then stop again).

The disadvantage of the snapshotting approach is that we do the unconditional work of heap-allocating a snapshot whenever an attribute or state changes, and the corresponding (potentially-no-op) style flush.

The idea in comment 0 sounds reasonable. The DependencySet machinery can keep a list of all of the attributes that it might be sensitive to, and we can quickly reject attributes that are never mentioned.

If things are still hot, we can look into doing the snapshot comparisons more eagerly, like Gecko does. But that removes much possibility for eliminating the current performance characteristics of HasAttrDependentStyle. Boris, do you think the mitigation is enough?

Anyway, the mitigation here should be relatively straightforward. Most of the work would be in restyle_hints.rs (making the DependencySet track all the attributes it's sensitive to), followed by the exposure of a simple FFI function called something like Servo_DocumentHasSelectorSensitiveToAttr().

P2.
Blocks: 1289864
Flags: needinfo?(bzbarsky)
Priority: -- → P2
Oh, and we should do the same thing for state-dependent style, I think. Updating the bug title.
Summary: stylo: all attr changes on elements that have been styled mark presshell as needing a style flush → stylo: track the attributes and state that a DependencySet is sensitive to, and use it to cull snapshot creation
I'm not saying we should stop snapshotting, for what it's worth.  I agree that Gecko's current setup is quite expensive in practice, though it could probably be made less so if it could be less abstract.

Anyway, when smaug and I were talking about this earlier today, he suggested stuffing all the attrs we might depend on for selector matching in a counting Bloom filter or something, then checking our attr that's changing against it.  That might be cheaper than a hashtable lookup (and use less memory) and just as good in practical terms as an exact test.  We could explicitly not put class and id in the filter and always do a snapshot for those: the set of pages that don't have class or id rules is tiny, and dynamic changes to class/id that are not meant to trigger restyle are quite rare, I suspect.

I can's guarantee what's enough or not enough here, but I think the mitigation is simple enough to do that we should do it before we try to do serious performance measurements.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #4)
> I'm not saying we should stop snapshotting, for what it's worth.  I agree
> that Gecko's current setup is quite expensive in practice, though it could
> probably be made less so if it could be less abstract.
> 
> Anyway, when smaug and I were talking about this earlier today, he suggested
> stuffing all the attrs we might depend on for selector matching in a
> counting Bloom filter or something, then checking our attr that's changing
> against it.

Ah nice, a bloom filter does sound like a nice improvement on a hashtable, and we have all the machinery in servo already.

Any reason it actually needs to be a counting filter? Servo's is counting, but the scheme I'm imagining would just be to rebuild the filter whenever we flush the style set, so I'm wondering if I'm missing something.

>  That might be cheaper than a hashtable lookup (and use less
> memory) and just as good in practical terms as an exact test.  We could
> explicitly not put class and id in the filter and always do a snapshot for
> those: the set of pages that don't have class or id rules is tiny, and
> dynamic changes to class/id that are not meant to trigger restyle are quite
> rare, I suspect.

Fair - though do we really gain much by special-casing these? Is the goal to just reduce the number of bloom filter collisions?
> Any reason it actually needs to be a counting filter?

Good point.  It doesn't.  Then we can have a a bunch more bits, and more bits per entry if desired, for the same memory usage.

> Is the goal to just reduce the number of bloom filter collisions?

Good question.  I'm not sure what I was thinking of.  ;)  Just those two names clearly won't have much impact on a decent bloom filter.  And the cost of checking for them might be comparable to just looking them up in the filter anyway, so it wouldn't necessarily be slower.  So yeah, probably no need to special-case.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #6)
> > Any reason it actually needs to be a counting filter?
> 
> Good point.  It doesn't.  Then we can have a a bunch more bits, and more
> bits per entry if desired, for the same memory usage.

Yes. Though the downside is that we may need to reimplement servo's bloom filter (which uses 8-bit counters) if we want to do that, which may may or may not be worth it.
Emilio, this is the bug you were working towards in [1] right? Assigning to you assuming that's the case, let me know if I've got that wrong.

[1] https://github.com/servo/servo/pull/16293
Assignee: nobody → emilio+bugs
Priority: P2 → P1
Blocks: 1243581
(Assignee)

Comment 9

20 days ago
Stealing this, with Emilio's permission.  I ended up working on this today to make this test faster: https://heycam.github.io/style-perf-tests/perf-reftest/run.html?test=style-attr-1.html

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

This is only for attributes so far, not content state.
Assignee: emilio+bugs → cam
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

20 days ago
Attachment #8865372 - Flags: review?(emilio+bugs)

Comment 12

20 days ago
mozreview-review
Comment on attachment 8865372 [details]
style: Record names that appear in attribute selectors.

https://reviewboard.mozilla.org/r/137034/#review139994

::: servo/components/style/stylist.rs:970
(Diff revision 1)
> +
> +    fn visit_attribute_selector(&mut self, selector: &AttrSelector<Self::Impl>) -> bool {
> +        if selector.lower_name == atom!("style") {
> +            self.0.style_attribute_dependency = true;
> +        } else {
> +            self.0.attribute_dependencies.insert(&selector.name);

Instead of this, let's use `insert_hash(selector.name.precomputed_hash())`, and the same for `lower_name`.

::: servo/ports/geckolib/glue.rs:2274
(Diff revision 1)
> +
> +#[no_mangle]
> +pub extern "C" fn Servo_StyleSet_MightHaveAttributeDependency(raw_data: RawServoStyleSetBorrowed,
> +                                                              local_name: *mut nsIAtom) -> bool {
> +    let data = PerDocumentStyleData::from_ffi(raw_data).borrow();
> +    data.stylist.might_have_attribute_dependency(Atom::from(local_name))

Does this need to use `from_addrefed`?
Attachment #8865372 - Flags: review?(emilio+bugs) → review+

Comment 13

20 days ago
mozreview-review
Comment on attachment 8865371 [details]
Bug 1352306 - Part 1: stylo: Only snapshot attributes if there is some rule that depends on that attribute.

https://reviewboard.mozilla.org/r/137032/#review139998

I guess the commits should be reordered in order for this to build, but that needs to happen anyway to land, so r=me
Attachment #8865371 - Flags: review?(emilio+bugs) → review+
Can you measure the impact of the extra work in the note_attribute_dependencies call? Stylist building can be expensive, so I'd like to understand how much we're adding here (the 100x myspace testcase is good for this).

If it _is_ expensive, I'm wondering whether it would be better to try to combine it with note_selector (and reduce the number of walks over the selector), or whether we should keep it separate so that we can parallelize it in bug 1362538 (since note_selector is currently the long pole in my measurements).
(Assignee)

Comment 15

20 days ago
Can you point me to the 100x myspace testcase?  Thanks.
Flags: needinfo?(bobbyholley)
https://www.dropbox.com/s/h51fspacftf1lcq/myspace.tar.bz2?dl=0

There are two dirs in there. you want the myspace.com one (not the -orig one), since that has the stylesheet concatenated 100x. Last I checked on my machine parsing takes something near 1s, and stylist rebuilding takes about 120ms. I would recommend adding #[inline(never)] on each of the functions called in that loops (note_selector, etc) to get the correct work breakdown in the gecko profiler.
Flags: needinfo?(bobbyholley)

Comment 17

20 days ago
mozreview-review-reply
Comment on attachment 8865372 [details]
style: Record names that appear in attribute selectors.

https://reviewboard.mozilla.org/r/137034/#review139994

> Does this need to use `from_addrefed`?

Never mind this, it doesn't. But this should probably use `WeakAtom`/`BorrowedAtom` though, to avoid the unnecessary refcount bump.
(Assignee)

Comment 18

19 days ago
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #16)
> There are two dirs in there. you want the myspace.com one (not the -orig
> one), since that has the stylesheet concatenated 100x. Last I checked on my
> machine parsing takes something near 1s, and stylist rebuilding takes about
> 120ms. I would recommend adding #[inline(never)] on each of the functions
> called in that loops (note_selector, etc) to get the correct work breakdown
> in the gecko profiler.

This is myspace.com/www.myspace.com/albumart.html?  On my machine I get 94 ms under flush_stylesheets:

* 51 ms for DependencySet::note_selector
* 30 ms for Stylist::add_rule_to_map
* 4 ms for Stylist::note_attribute_dependencies (the new function)
* 2 ms for Stylist::note_for_revalidation
(Assignee)

Comment 19

19 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a55c92bcb94cef581fd05da7cb99cccb98582eaf
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

19 days ago
Attachment #8865807 - Flags: review?(emilio+bugs)

Comment 24

19 days ago
mozreview-review
Comment on attachment 8865807 [details]
style: Record ElementState bits that selectors depend on.

https://reviewboard.mozilla.org/r/137420/#review140518

r=me.

Have you done any measurent that indicates that this is a worthwile optimization, or is this just for simmetry with the attribute dependency tracking?

::: servo/components/style/stylist.rs:995
(Diff revision 1)
>          true
>      }
> +
> +    fn visit_simple_selector(&mut self, s: &Component<SelectorImpl>) -> bool {
> +        if let Component::NonTSPseudoClass(ref p) = *s {
> +            self.0.state_dependencies.insert(SelectorImpl::pseudo_class_state_flag(p));

I think you can use `p.state_flag()` directly.
Attachment #8865807 - Flags: review?(emilio+bugs) → review+

Comment 25

19 days ago
mozreview-review
Comment on attachment 8865808 [details]
Bug 1352306 - Part 2: stylo: Only snapshot EventStates if there is some rule that depends on it.

https://reviewboard.mozilla.org/r/137422/#review140526
Attachment #8865808 - Flags: review?(emilio+bugs) → review+
(In reply to Cameron McCormack (:heycam) from comment #18)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #16)
> > There are two dirs in there. you want the myspace.com one (not the -orig
> > one), since that has the stylesheet concatenated 100x. Last I checked on my
> > machine parsing takes something near 1s, and stylist rebuilding takes about
> > 120ms. I would recommend adding #[inline(never)] on each of the functions
> > called in that loops (note_selector, etc) to get the correct work breakdown
> > in the gecko profiler.
> 
> This is myspace.com/www.myspace.com/albumart.html?  On my machine I get 94
> ms under flush_stylesheets:
> 
> * 51 ms for DependencySet::note_selector
> * 30 ms for Stylist::add_rule_to_map
> * 4 ms for Stylist::note_attribute_dependencies (the new function)
> * 2 ms for Stylist::note_for_revalidation

Ok, that corroborates my mental model that the visitor traversal is cheap and that most of the expense is the hash table insertions and the arc clones (which are much more expensive than the bloom filter insertion). Let's leave this as-is, and then we can win a tiny bit with parallelizing it too.

Thanks for checking this.
(Assignee)

Comment 27

19 days ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #24)
> Have you done any measurent that indicates that this is a worthwile
> optimization, or is this just for simmetry with the attribute dependency
> tracking?

I didn't do any measurement.  It's the same kind of wins that can be had from the style-attr-1.html test (i.e., if there's a large number of dependencies to check, it's helpful).
(Assignee)

Comment 28

19 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84025d376b5a5807c956cf1262689f1b0ab502df
(Assignee)

Comment 29

17 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fecc4c26b933f00c756a98f18abd31e238ee7b05
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

17 days ago
Attachment #8865372 - Attachment is obsolete: true
(Assignee)

Updated

17 days ago
Attachment #8865807 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 34

16 days ago
https://github.com/servo/servo/pull/16811

Comment 35

16 days ago
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5f55c10a0a72
Part 1: stylo: Only snapshot attributes if there is some rule that depends on that attribute. r=emilio
https://hg.mozilla.org/integration/autoland/rev/72fe8375faa0
Part 2: stylo: Only snapshot EventStates if there is some rule that depends on it. r=emilio
Backed out for ElementInlines.h assertions.
https://treeherder.mozilla.org/logviewer.html#?job_id=98696365&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/cb377b17ba5c

Comment 37

16 days ago
mozreview-review
Comment on attachment 8865371 [details]
Bug 1352306 - Part 1: stylo: Only snapshot attributes if there is some rule that depends on that attribute.

https://reviewboard.mozilla.org/r/137032/#review142128

::: layout/base/ServoRestyleManager.cpp:614
(Diff revision 4)
>    if (!aElement->HasServoData()) {
>      return;
>    }
>  
>    ServoElementSnapshot& snapshot = SnapshotFor(aElement);
> +  if (snapshot.HasAttrs()) {

Btw, while we're here, we should probably switch the order of this and avoid creating the snapshot altogether if there can't be any dependency.
Also, I've investigated the assertions, and they all seem related to bug 1364361.
See Also: → bug 1364361
(Assignee)

Comment 39

16 days ago
My WIP patches for bug 1364361 do fix the widget/cocoa/crashtests/464589-1.html assertion, so I'll work to get that landed before re-landing this bug's patches.
(Assignee)

Comment 40

16 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e83d4226033aae8d925f54542e2759b7e7edacc0
You need to log in before you can comment on or make changes to this bug.