stylo: Cache media query results and only flush and restyle if they changed.

RESOLVED FIXED in Firefox 55

Status

()

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

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Blocks: 3 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

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

3 months ago
I thought we had a bug on file on this, but we don't, so let's do it.

Right now we post a full restyle every time anything that may change media query evaluation results changes. We should definitely get that fixed before shipping, since I'm pretty sure it'll make window resizes unacceptably slow.
Priority: -- → P1
(Assignee)

Comment 1

2 months ago
I finally got time to write this. Patches incoming. I doubt patches pass tests without the mUsesVieportUnits lie, but I'll do that as a Part 0, I think. I need to run now though.
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 9

2 months ago
mozreview-review
Comment on attachment 8873956 [details]
Bug 1357461: style: Move the stylesheet invalidation code to another submodule.

https://reviewboard.mozilla.org/r/145372/#review149456
Attachment #8873956 - Flags: review?(cam) → review+

Comment 10

2 months ago
mozreview-review
Comment on attachment 8873957 [details]
Bug 1357461: Make RulesIterator not iterate over rules for which we don't process nested rules.

https://reviewboard.mozilla.org/r/145374/#review149458

Maybe document on NestedRuleIterationCondition that these functions mean to skip the rule as well as their nested rules?
Attachment #8873957 - Flags: review?(cam) → review+

Comment 11

2 months ago
mozreview-review
Comment on attachment 8873958 [details]
Bug 1357461: Cache effective media query results.

https://reviewboard.mozilla.org/r/145376/#review149460

::: servo/components/style/invalidation/media_queries.rs:16
(Diff revision 1)
> +/// NOTE: It happens to be the case that all the media lists we care about
> +/// happen to have a stable address, so we can just use an opaque pointer to
> +/// represent them.

Is it possible that we can destroy a MediaRule (for example) and create a new one soon after, and we think we have the same MediaListKey?  Are we guaranteed that we'll invalidate any of the MediaListKeys we've stored by clearing the Stylist's EffectiveMediaQueryResults when rules/sheets are removed?

::: servo/components/style/stylist.rs:783
(Diff revision 1)
> -            let mq = stylesheet.media.read_with(guard);
> -            if mq.evaluate(&self.device, self.quirks_mode) != mq.evaluate(&device, self.quirks_mode) {
> +        let features_changed = self.media_features_changed(
> +            stylesheets.iter(),
> +            guard
> +        );

Nit: not really a fan of wrapping arguments like this when we can easily fit within our 99 (or 119 at a stretch) column limit on one line.  I think it makes it harder to read.

::: servo/components/style/stylist.rs:820
(Diff revision 1)
> -                    guard);
> +                    guard
> +                );

Nit: neither this.  It's not some new rustfmt rule is it?
Attachment #8873958 - Flags: review?(cam) → review+

Comment 12

2 months ago
mozreview-review
Comment on attachment 8873959 [details]
Bug 1357461: Add some logging.

https://reviewboard.mozilla.org/r/145378/#review149462
Attachment #8873959 - Flags: review?(cam) → review+

Comment 13

2 months ago
mozreview-review
Comment on attachment 8873960 [details]
Bug 1357461: Ensure the device is up-to-date before evaluating media queries.

https://reviewboard.mozilla.org/r/145380/#review149464

::: servo/ports/geckolib/glue.rs:762
(Diff revision 1)
> +    // We need to ensure the default computed values are up to date though,
> +    // because those can influence the result of media query evaluation.
> +    let mut data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
> +    data.stylist.device_mut().reset();

Is there a way we can avoid recomputing the default computed values, but ensuring they're up to date?  I think we'll call into this function for every window resize event, so it would be good to avoid doing work that we don't need to.

::: servo/ports/geckolib/glue.rs:1435
(Diff revision 1)
> -    data.reset_device(&guard);
> +    data.stylesheets.force_dirty();
> +    data.flush_stylesheets::<GeckoElement>(&guard, None);

What's the effect of this change?

Comment 14

2 months ago
mozreview-review
Comment on attachment 8873961 [details]
Bug 1357461: Ensure not to increment the restyle generation if we haven't restyled after all.

https://reviewboard.mozilla.org/r/145382/#review149466

::: layout/base/ServoRestyleManager.h:118
(Diff revision 2)
>    /**
>     * Performs post-Servo-traversal processing on this element and its descendants.
>     */
> -  void ProcessPostTraversal(Element* aElement,
> +  bool ProcessPostTraversal(Element* aElement,
>                              nsStyleContext* aParentContext,
>                              ServoStyleSet* aStyleSet,
>                              nsStyleChangeList& aChangeList);
>  
>    struct TextPostTraversalState;
> -  void ProcessPostTraversalForText(nsIContent* aTextNode,
> +  bool ProcessPostTraversalForText(nsIContent* aTextNode,
>                                     nsStyleChangeList& aChangeList,
>                                     TextPostTraversalState& aState);

Please comment here what the return value means.
Attachment #8873961 - Flags: review?(cam) → review+
(Assignee)

Comment 15

2 months ago
mozreview-review-reply
Comment on attachment 8873960 [details]
Bug 1357461: Ensure the device is up-to-date before evaluating media queries.

https://reviewboard.mozilla.org/r/145380/#review149464

> Is there a way we can avoid recomputing the default computed values, but ensuring they're up to date?  I think we'll call into this function for every window resize event, so it would be good to avoid doing work that we don't need to.

I'll file a bug for this.

> What's the effect of this change?

Which change? This is effectively doing the same we were doing before, though now we don't need to call device.reset() it didn't make a lot of sense to have a `reset_device` function, so I instead removed it and inlined it here, since there was only one caller. If there can be other ways that the default computed values may change, we may need to do it here too.
(Assignee)

Updated

2 months ago
Blocks: 1369984

Comment 16

2 months ago
mozreview-review-reply
Comment on attachment 8873960 [details]
Bug 1357461: Ensure the device is up-to-date before evaluating media queries.

https://reviewboard.mozilla.org/r/145380/#review149464

> Which change? This is effectively doing the same we were doing before, though now we don't need to call device.reset() it didn't make a lot of sense to have a `reset_device` function, so I instead removed it and inlined it here, since there was only one caller. If there can be other ways that the default computed values may change, we may need to do it here too.

OK, I just didn't know (or check) whether it was equivalent to the old code.

Comment 17

2 months ago
mozreview-review
Comment on attachment 8873960 [details]
Bug 1357461: Ensure the device is up-to-date before evaluating media queries.

https://reviewboard.mozilla.org/r/145380/#review149474
Attachment #8873960 - Flags: review?(cam) → review+
(Assignee)

Comment 18

2 months ago
mozreview-review-reply
Comment on attachment 8873958 [details]
Bug 1357461: Cache effective media query results.

https://reviewboard.mozilla.org/r/145376/#review149460

> Is it possible that we can destroy a MediaRule (for example) and create a new one soon after, and we think we have the same MediaListKey?  Are we guaranteed that we'll invalidate any of the MediaListKeys we've stored by clearing the Stylist's EffectiveMediaQueryResults when rules/sheets are removed?

Yes, I think so, since as of right now rule removal and stylesheet removal both trigger a full rebuild.

We could do better, and we'd need to ensure we remove the keys in that case.

> Nit: neither this.  It's not some new rustfmt rule is it?

Yes, see https://github.com/rust-lang-nursery/fmt-rfcs and the related issues. rustfmt is switching to block-formatting for arguments.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

2 months ago
mozreview-review
Comment on attachment 8874094 [details]
Bug 1357461: Assume viewport units are used on resize.

https://reviewboard.mozilla.org/r/145554/#review149480

::: layout/base/nsPresContext.cpp:2077
(Diff revision 1)
> -  if (mUsesViewportUnits && mPendingViewportChange) {
> +  if (mPendingViewportChange && (
> +      (mUsesViewportUnits || mDocument->IsStyledByServo()))) {

Nit: drop the extra set of parens (the one surrounding the second subexpression starting on the first line).
Attachment #8874094 - Flags: review?(cam) → review+
(Assignee)

Comment 25

2 months ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f29c25155b8485716fa0b0ddde6abd7afa72d8e1
PR: https://github.com/servo/servo/pull/17147
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

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

Comment 29

2 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/54e9145b8892
Cache effective media query results. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e0477439b222
Ensure not to increment the restyle generation if we haven't restyled after all. r=heycam
https://hg.mozilla.org/integration/autoland/rev/6ddc4c39388e
Assume viewport units are used on resize. r=heycam
https://hg.mozilla.org/mozilla-central/rev/54e9145b8892
https://hg.mozilla.org/mozilla-central/rev/e0477439b222
https://hg.mozilla.org/mozilla-central/rev/6ddc4c39388e
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.