Closed Bug 1357583 Opened 3 years ago Closed 3 years ago

stylo: Avoid triggering restyle when a stylesheet is added that can't affect the document

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bholley, Assigned: emilio)

References

Details

Attachments

(5 files, 6 obsolete files)

59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
59 bytes, text/x-review-board-request
heycam
: review+
Details
See bug 1273303. Emilio is going to take this.
See Also: → 1348481
Depends on: 1367245
Depends on: 1367553
Let's assume that's not horribly broken, I'll start posting patches for review, since I need to run now.
Duplicate of this bug: 1367245
Attachment #8871960 - Flags: review?(cam)
So, there's only a single failure from this patches, and I doubt it's because of these changes tbh, it's a single pixel difference in a jpeg image test (o.o).

Seems reliable, though...
Comment on attachment 8871800 [details]
Bug 1357583: Remove ServoStyleSheet::ClearRuleCascades.

https://reviewboard.mozilla.org/r/143258/#review147404

::: layout/style/StyleSheet.cpp:441
(Diff revision 1)
>    // Ensure we're using the new rules.
> -  ClearRuleCascades();
> +  if (CSSStyleSheet* geckoSheet = GetAsGecko()) {
> +    geckoSheet->ClearRuleCascades();
> +  }

Can you explain why we don't need to do this for Servo sheets?  Is is that we've already done the work (marking the stylist dirty etc.)?
Comment on attachment 8871801 [details]
Bug 1357583: Ensure we send the document element to Servo when flushing stylesheets.

https://reviewboard.mozilla.org/r/143260/#review147406

::: commit-message-709de:6
(Diff revision 1)
> +Bug 1357583: Ensure we send the document element to Servo when flushing stylesheets. r?heycam
> +
> +We'll use it to invalidate stuff.
> +
> +MozReview-Commit-ID: Il3wO5JQh1Y
> +Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>

Drop this (here and in the other patches).

::: layout/style/ServoStyleSet.cpp:134
(Diff revision 1)
>      mPresContext->RestyleManager()->PostRestyleEventForCSSRuleChanges(
> -        root, eRestyle_Subtree, nsChangeHint(0));
> +      root, nsRestyleHint(0), nsChangeHint(0));

This won't do anything, will it?  ServoRestyleManager::PostRestyleEvent returns early if the restyle hint and change hints are both zero.

::: layout/style/ServoStyleSet.cpp
(Diff revision 1)
>    mAuthorStyleDisabled = aStyleDisabled;
> +  ForceAllStyleDirty();
>  
> -  // If we've just disabled, we have to note the stylesheets have changed and
> -  // call flush directly, since the PresShell won't.
> -  if (mAuthorStyleDisabled) {

Why don't we need to do this check any more?
Comment on attachment 8871802 [details]
Bug 1357583: style: Add an initial version of the stylesheet invalidation code.

https://reviewboard.mozilla.org/r/143262/#review147408

::: servo/components/style/invalidation/mod.rs:5
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +//! A collection of invalidations due stylesheets added to the document.

"due to"

::: servo/components/style/invalidation/mod.rs:19
(Diff revision 1)
> +use selectors::parser::{Component, Selector};
> +use shared_lock::SharedRwLockReadGuard;
> +use stylesheets::{CssRule, CssRules, Stylesheet};
> +use stylist::Stylist;
> +
> +/// A style scope represents a kind of subtree that may need to be restyled.

s/A style scope/An invalidation scope/?  Sounds a bit like <style scoped> or XBL style sheet scopes otherwise.

::: servo/components/style/invalidation/mod.rs:158
(Diff revision 1)
> +    /// We prefer id scopes to class scopes, and outermost scopes to innermost
> +    /// scopes (to reduce the amount of traversal we need to do).

I guess I don't have a good sense of whether to do that, or to look at the rightmost ID/class and do more traversal.  But seems fine to do this.  And prioritising IDs sounds like a good idea.

::: servo/components/style/invalidation/mod.rs:187
(Diff revision 1)
> +        match scope {
> +            Some(s) => {
> +                self.invalid_scopes.insert(s);
> +            }
> +            None => {
> +                // If we didn't found a scope, any selector could match this, so

find

::: servo/components/style/invalidation/mod.rs:211
(Diff revision 1)
> +                if !doc_rule.condition.evaluate(stylist.device()) {
> +                    return false; // Won't apply anything else after this.
> +                }

Why is this different from @media rules, where we return true?

::: servo/components/style/invalidation/mod.rs:267
(Diff revision 1)
> +                // guess.
> +                self.fully_invalid = true;
> +            }
> +        }
> +
> +        return !self.fully_invalid

Nit: drop "return".
Attachment #8871802 - Flags: review?(cam) → review+
Comment on attachment 8871803 [details]
Bug 1357583: style: Hook up the invalidator in the StyleSheetSet.

https://reviewboard.mozilla.org/r/143264/#review147412

::: servo/components/style/gecko_string_cache/mod.rs:37
(Diff revision 1)
> -macro_rules! local_name {
> -    ($s: tt) => { atom!($s) }
> -}
> +// macro_rules! local_name {
> +//     ($s: tt) => { atom!($s) }
> +// }

?

::: servo/components/style/invalidation/mod.rs:109
(Diff revision 1)
>      /// Process style invalidations in a given subtree, that is, look for all
>      /// the relevant scopes in the subtree, and mark as dirty only the relevant
>      /// ones.
>      ///
>      /// Returns whether it invalidated at least one element's style.

Something to think about for a followup, but I wonder if instead of expanding out the invalidations by traversing the subtree here before doing the main restyle traversal, we could instead have the invalidation set accessible from the restyle worker threads (in the shared style context), and check it during the normal traversal.  It would mean that the dirty descendants bits don't quite accurately describe where we can stop traversing, but it would be nice to avoid this additional (serialized) traversal here.

::: servo/ports/geckolib/glue.rs:794
(Diff revision 1)
>  }
>  
>  #[no_mangle]
>  pub extern "C" fn Servo_StyleSet_FlushStyleSheets(
>      raw_data: RawServoStyleSetBorrowed,
> -    doc_element: RawServoElementBorrowedOrNull)
> +    doc_element: RawGeckoElementBorrowedOrNull)

Maybe fix this in the earlier patch.
Attachment #8871803 - Flags: review?(cam) → review+
Comment on attachment 8871804 [details]
Bug 1357583: Add an API to get the restyle generation from the pres context.

https://reviewboard.mozilla.org/r/143266/#review147418
Attachment #8871804 - Flags: review?(cam) → review+
Comment on attachment 8871805 [details]
Bug 1357583: Tidy the PostRestyleEventForCSSRuleChanges API.

https://reviewboard.mozilla.org/r/143268/#review147420
Attachment #8871805 - Flags: review?(cam) → review+
Comment on attachment 8871806 [details]
Bug 1357583: Add a bunch of logging, shortcuts, and look also at the rightmost selector while invalidating sheets.

https://reviewboard.mozilla.org/r/143270/#review147422
Attachment #8871806 - Flags: review?(cam) → review+
Comment on attachment 8871807 [details]
Bug 1357583: Test.

https://reviewboard.mozilla.org/r/143272/#review147424

Looks good.  Maybe add some perf-reftests for this too.

::: layout/style/test/test_stylesheet_additions.html:25
(Diff revision 1)
> +<style id="target" disabled></style>
> +<script>
> +SimpleTest.waitForExplicitFinish();
> +const utils = SpecialPowers.getDOMWindowUtils(window);
> +const TESTS = [
> +  { selector: ".unexistentClassScope", restyle: false },

s/unexisting/nonexistent/g
Attachment #8871807 - Flags: review?(cam) → review+
Comment on attachment 8871909 [details]
Bug 1357583: style: Recurse into @import rules when looking at the added rules.

https://reviewboard.mozilla.org/r/143430/#review147426
Attachment #8871909 - Flags: review?(cam) → review+
Comment on attachment 8871960 [details]
Bug 1357583: style: Make effective_rules return an iterator, stop refcounting the Device.

https://reviewboard.mozilla.org/r/143484/#review147432

::: servo/components/style/stylist.rs:435
(Diff revision 2)
> -        // Cheap `Arc` clone so that the closure below can borrow `&mut Stylist`.
> +        for rule in stylesheet.effective_rules(&self.device, guard) {
> -        let device = self.device.clone();

Did you inline the style rule processing function calls to avoid this?  The out of line functions probably make this function a bit more manageable...
Attachment #8871960 - Flags: review?(cam) → review+
Comment on attachment 8871977 [details]
Bug 1357583: Avoid unconditionally popping under RulesIterator.

https://reviewboard.mozilla.org/r/143510/#review147436

::: servo/components/style/stylesheets.rs:991
(Diff revision 1)
> +                let mut nested_iter = self.stack.last_mut().unwrap();
> +                rule = match nested_iter.next() {
> -                Some(r) => r,
> +                    Some(r) => r,
> -                None => continue,
> +                    None => {
> +                        pop = true;
> +                        continue
> +                    }
> -            };
> +                };

Maybe rename "pop" to "nested_iter_finished"?

I guess because of lifetime stuff you can't just do the popping in here, as soon as you detect the nested iterator is finished.
Attachment #8871977 - Flags: review?(cam) → review+
Comment on attachment 8871800 [details]
Bug 1357583: Remove ServoStyleSheet::ClearRuleCascades.

https://reviewboard.mozilla.org/r/143258/#review147404

> Can you explain why we don't need to do this for Servo sheets?  Is is that we've already done the work (marking the stylist dirty etc.)?

So, I removed this because it wasn't clear at all it's needed. Basically every change that makes us change the set of selectors/rules we match is covered by the presshell notifications.

I understand this may be needed in Gecko because the selectors are not refcounted (IIUC), but this is not the case in Servo.
Comment on attachment 8871801 [details]
Bug 1357583: Ensure we send the document element to Servo when flushing stylesheets.

https://reviewboard.mozilla.org/r/143260/#review147406

> Drop this (here and in the other patches).

Yikes, I need to make `git format-patch` smarter than auto-signing off.

> This won't do anything, will it?  ServoRestyleManager::PostRestyleEvent returns early if the restyle hint and change hints are both zero.

Right, the idea was that it did set the mRestyleForCSSRuleChanges boolean. But this doesn't force a style flush to happen, nor this patch implements the "dirtyness" bit when flushing, so as written it's basically wrong. It's fixed in the following patches though. I can squash this commit somehow into another one so that it's not broken.

> Why don't we need to do this check any more?

We didn't need it in the first place.

I think this was intended as an "optimization"? I never understood that comment about JS and the presshell notifications (neither did bz btw).

It doesn't optimize anything basically, and only relied on other part of the code (which one, it isn't clear to me) to mark the stylist as dirty.

I removed it to be explicit.
Comment on attachment 8871800 [details]
Bug 1357583: Remove ServoStyleSheet::ClearRuleCascades.

https://reviewboard.mozilla.org/r/143258/#review147404

> So, I removed this because it wasn't clear at all it's needed. Basically every change that makes us change the set of selectors/rules we match is covered by the presshell notifications.
> 
> I understand this may be needed in Gecko because the selectors are not refcounted (IIUC), but this is not the case in Servo.

OK, can you please add a comment here explaining this.
Comment on attachment 8871800 [details]
Bug 1357583: Remove ServoStyleSheet::ClearRuleCascades.

https://reviewboard.mozilla.org/r/143258/#review147574
Attachment #8871800 - Flags: review?(cam) → review+
Comment on attachment 8871801 [details]
Bug 1357583: Ensure we send the document element to Servo when flushing stylesheets.

https://reviewboard.mozilla.org/r/143260/#review147406

> Right, the idea was that it did set the mRestyleForCSSRuleChanges boolean. But this doesn't force a style flush to happen, nor this patch implements the "dirtyness" bit when flushing, so as written it's basically wrong. It's fixed in the following patches though. I can squash this commit somehow into another one so that it's not broken.

Ah, yes, I did see in the later patches this was changed.  Not a big deal, if you want to leave this as is.

> We didn't need it in the first place.
> 
> I think this was intended as an "optimization"? I never understood that comment about JS and the presshell notifications (neither did bz btw).
> 
> It doesn't optimize anything basically, and only relied on other part of the code (which one, it isn't clear to me) to mark the stylist as dirty.
> 
> I removed it to be explicit.

OK.
Comment on attachment 8871801 [details]
Bug 1357583: Ensure we send the document element to Servo when flushing stylesheets.

https://reviewboard.mozilla.org/r/143260/#review147578
Attachment #8871801 - Flags: review?(cam) → review+
Attachment #8871802 - Attachment is obsolete: true
Attachment #8871804 - Attachment is obsolete: true
Attachment #8871806 - Attachment is obsolete: true
Attachment #8871909 - Attachment is obsolete: true
Attachment #8871960 - Attachment is obsolete: true
Attachment #8871977 - Attachment is obsolete: true
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3deb86de58ce
Remove ServoStyleSheet::ClearRuleCascades. r=heycam
https://hg.mozilla.org/integration/autoland/rev/d279a19a1e15
Ensure we send the document element to Servo when flushing stylesheets. r=heycam
https://hg.mozilla.org/integration/autoland/rev/386f7a3132e7
style: Hook up the invalidator in the StyleSheetSet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/142539198f30
Tidy the PostRestyleEventForCSSRuleChanges API. r=heycam
https://hg.mozilla.org/integration/autoland/rev/8f0667eca06e
Test. r=heycam
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/07778bb17a4a
Add an API to get the restyle generation from the pres context. r=heycam
Depends on: 1368589
Depends on: 1368690
No longer depends on: 1368690
See Also: → 1372061
Blocks: 1379433
See Also: → 1407022
You need to log in before you can comment on or make changes to this bug.