Closed Bug 1352306 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: bzbarsky, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Bug 1352306 - Part 1: stylo: Only snapshot attributes if there is some rule that depends on that attribute.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1352306 - Part 2: stylo: Only snapshot EventStates if there is some rule that depends on it.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Test case for the treeherder issue.
273 bytes, text/html
Details 
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: stylo-perf
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
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
Attachment #8865372 - Flags: review?(emilio+bugs)
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 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).
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 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.
(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
Attachment #8865807 - Flags: review?(emilio+bugs)
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 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.
(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).
Attachment #8865372 - Attachment is obsolete: true
Attachment #8865807 - Attachment is obsolete: true
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
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: → 1364361
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.
Depends on: 1364361
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.
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.
Flags: needinfo?(cam)
(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)
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
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
https://hg.mozilla.org/mozilla-central/rev/cde7eed48208
https://hg.mozilla.org/mozilla-central/rev/064975f358bc
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: