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

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
4 months ago
a month ago

People

(Reporter: bz, Assigned: heycam)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 wontfix, firefox56 fixed)

Details

MozReview Requests

()

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

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

4 months ago
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?
(Reporter)

Comment 1

4 months ago
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
(Reporter)

Comment 4

4 months ago
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?
(Reporter)

Comment 6

4 months ago
> 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

3 months 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

3 months ago
Attachment #8865372 - Flags: review?(emilio+bugs)

Comment 12

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months ago
Attachment #8865807 - Flags: review?(emilio+bugs)

Comment 24

3 months 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

3 months 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

3 months 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

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

Comment 29

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

Updated

3 months ago
Attachment #8865372 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 34

2 months ago
https://github.com/servo/servo/pull/16811

Comment 35

2 months 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

2 months 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

2 months 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

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e83d4226033aae8d925f54542e2759b7e7edacc0
(Assignee)

Updated

a month ago
Depends on: 1364361
(Assignee)

Comment 41

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b59054b92b0270ba4a3a5eac66fcd13f63ef9375
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 44

a month ago
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb6b79e53e82
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/46d7719ee8ae
Part 2: stylo: Only snapshot EventStates if there is some rule that depends on it. r=emilio
Apart from the tests, which seem intermittent and work for me in a local build, this patch makes Treeherder not show up properly in a Stylo build.

I've investigated a bit and it's because we're incorrectly thinking that an |ng-cloak| attribute doesn't affect style.

The stylist is marked as dirty at that point. However it should be fine, because we should invalidate the whole document as a result of that stylesheet addition... So something fishy is going on there.
Backed out as requested by emilio:

https://hg.mozilla.org/integration/autoland/rev/5325bf42a7a24f3fe60bf270b7eb4b8fb4b7ec24
https://hg.mozilla.org/integration/autoland/rev/6aee7eba7adc9b5bb4d1c4fdb529056e983ad593
Oh, didn't mention, the tests that started failing when this landed are layout/reftests/image/image-object-position-dyn-1.html and layout/reftests/image/image-object-fit-dyn-1.html, due to the images of two of the elements not being loaded yet I think.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #45)
> Apart from the tests, which seem intermittent and work for me in a local
> build, this patch makes Treeherder not show up properly in a Stylo build.
> 
> I've investigated a bit and it's because we're incorrectly thinking that an
> |ng-cloak| attribute doesn't affect style.
> 
> The stylist is marked as dirty at that point. However it should be fine,
> because we should invalidate the whole document as a result of that
> stylesheet addition... So something fishy is going on there.

Ok, so what happens is that _another_ stylesheet gets added/removed in the meantime, and we clear the stylist then until the next flush happens. So the bloom filter will be empty, and we'll eat the attribute changes that happen between that and the next flush.

So either:

 * We unconditionally return true in MayHave{Attribute/State}Dependency if the stylist is dirty.
 * We ensure the stylist is up-to-date by then.

I think the first is the easiest, personally, but not a big deal which path we take.
Created attachment 8878780 [details]
Test case for the treeherder issue.
Flags: needinfo?(cam)
(Assignee)

Comment 50

a month ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #48)
>  * We unconditionally return true in MayHave{Attribute/State}Dependency if
> the stylist is dirty.

Sounds reasonable.  Thanks for investigating!
Flags: needinfo?(cam)
(Assignee)

Comment 51

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c09a155cb87d7341f577d9f5d317b6ccfe9c436e
(Assignee)

Comment 52

a month ago
Helps if I add the test file: https://treeherder.mozilla.org/#/jobs?repo=try&revision=201e3576a73f190a2fad8717c6d797ab5dcef3d5
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 55

a month ago
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 14 changes to 9 files
remote: (0b394c16a22d modifies servo/components/style/stylist.rs from Servo; not enforcing peer review)
remote: (0b394c16a22d modifies servo/ports/geckolib/glue.rs from Servo; not enforcing peer review)
remote: (8ce101fbeca1 modifies servo/components/style/stylist.rs from Servo; not enforcing peer review)
remote: (2 changesets contain changes to protected servo/ directory: 8ce101fbeca1, 0b394c16a22d)
remote: ************************************************************************
remote: you do not have permissions to modify files under servo/
remote: 
remote: the servo/ directory is kept in sync with the canonical upstream
remote: repository at https://github.com/servo/servo
remote: 
remote: changes to servo/ are only allowed by the syncing tool and by sheriffs
remote: performing cross-repository "merges"
remote: 
remote: to make changes to servo/, submit a Pull Request against the servo/servo
remote: GitHub project
remote: ************************************************************************
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.e_prevent_vendored hook failed
abort: push failed on remote
(Assignee)

Comment 56

a month ago
https://github.com/servo/servo/pull/17397
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 59

a month ago
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cde7eed48208
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/064975f358bc
Part 2: stylo: Only snapshot EventStates if there is some rule that depends on it. r=emilio

Comment 60

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cde7eed48208
https://hg.mozilla.org/mozilla-central/rev/064975f358bc
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
status-firefox55: affected → wontfix
You need to log in before you can comment on or make changes to this bug.