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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
3 months ago
17 days ago

People

(Reporter: bholley, Assigned: emilio)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(5 attachments, 6 obsolete attachments)

59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
See bug 1273303. Emilio is going to take this.
(Assignee)

Updated

3 months ago
See Also: → bug 1348481
Blocks: 1273303
(Assignee)

Updated

2 months ago
Depends on: 1367245
(Assignee)

Updated

2 months ago
Depends on: 1367553
(Assignee)

Comment 1

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

Comment 2

2 months ago
Let's assume that's not horribly broken, I'll start posting patches for review, since I need to run now.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1367245
(Assignee)

Updated

2 months ago
Attachment #8871960 - Flags: review?(cam)
(Assignee)

Comment 14

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

Comment 15

2 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

2 months ago
Here's a try run with the last commit: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb9394bfd4f421a1f835a5f4c9e8e565ecec9bf3

Comment 19

2 months ago
mozreview-review
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 20

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

::: 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 21

2 months ago
mozreview-review
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 22

2 months ago
mozreview-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 23

2 months ago
mozreview-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 24

2 months ago
mozreview-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 25

2 months ago
mozreview-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 26

2 months ago
mozreview-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 27

2 months ago
mozreview-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 28

2 months ago
mozreview-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 29

2 months ago
mozreview-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+
(Assignee)

Comment 30

2 months ago
mozreview-review-reply
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.
(Assignee)

Comment 31

2 months ago
mozreview-review-reply
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 32

2 months ago
mozreview-review-reply
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 33

2 months ago
mozreview-review
Comment on attachment 8871800 [details]
Bug 1357583: Remove ServoStyleSheet::ClearRuleCascades.

https://reviewboard.mozilla.org/r/143258/#review147574
Attachment #8871800 - Flags: review?(cam) → review+

Comment 34

2 months ago
mozreview-review-reply
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 35

2 months ago
mozreview-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/#review147578
Attachment #8871801 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8871802 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8871804 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8871806 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8871909 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8871960 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8871977 - Attachment is obsolete: true

Comment 41

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

Comment 42

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

Comment 43

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3deb86de58ce
https://hg.mozilla.org/mozilla-central/rev/d279a19a1e15
https://hg.mozilla.org/mozilla-central/rev/386f7a3132e7
https://hg.mozilla.org/mozilla-central/rev/142539198f30
https://hg.mozilla.org/mozilla-central/rev/8f0667eca06e
https://hg.mozilla.org/mozilla-central/rev/07778bb17a4a
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1368690
(Assignee)

Updated

2 months ago
No longer depends on: 1368690
(Assignee)

Updated

a month ago
See Also: → bug 1372061
(Assignee)

Updated

17 days ago
Blocks: 1379433
You need to log in before you can comment on or make changes to this bug.