stylo: We're doing way too much Stylist::update

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

53 Branch
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, 1 obsolete attachment)

Steps to reproduce: see bug 1358806 comment 0.  I created two profiles for that pageload (Gecko and Stylo), and I'm seeing a bunch of Stylist::update time; way more than nsCSSRuleProcessor::RefreshRuleCascade takes.

Gecko: https://perf-html.io/public/044181aaa877d66032e934aa57fa00a7553b2622/calltree/?search=RefreshRuleCascade&thread=3

Stylo: https://perf-html.io/public/1a62b6632b2331ebc4ad3d0dcbfaba4e8118ee01/calltree/?search=Stylist%3A%3Aupdate&thread=3

Note that gecko spends ~3ms and stylo ~30ms here.  There are two reasons stylo is so much slower:

1)  Half that 30ms is actually from stylesheet _removals_.  Yes, we need to update on sheet removals, but these are coming from PresShell::Destroy() as the previous page is unloaded.  This is almost certainly removal of the preference stylesheet.  We should very much not need to do any stylist updating here!

2)  Relatedly, sheet _additions_ also call into Stylist::update.  They do it after every sheet is added.  Each call redoes all the work of updating the stylist.  By way of contrast, Gecko does RefreshRuleCascade() at the moment when it needs it, if the cascade is dirty.  The page has multiple sheets in it; we add them one by one.  As a result we end up doing unnecessary work just to drop it immediately.

I'm going to look into doing these updates more lazily.
As noticed on IRC, we should coordinate with bug 1357583 here.
In particular, I think we should:

1)  Split update() into clear() and rebuild().
2)  Call clear() on sheet removals.  It will no-op if already cleared, else do more or less
    http://searchfox.org/mozilla-central/rev/abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/servo/components/style/stylist.rs#252-267
3)  Call update() on style set flushes.
4)  In Gecko, don't do style set flushes on EndUpdate(); instead do them at places where we might actually
    be getting style (e.g. the places where we call PreTraverse(), but we might need to verify that that's
    enough; places that are OK with "stale" style might still want to update the stylist).
5)  Figure out the servo side.

Working on this.
Depends on: 1362538
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

a year ago
mozreview-review
Comment on attachment 8866401 [details]
Bug 1361843 part 1.  Split up Stylist's update() method into separate clear() and rebuild() methods.

https://reviewboard.mozilla.org/r/138034/#review141144

::: servo/components/style/stylist.rs:248
(Diff revision 1)
> +        }
> +
> +        self.viewport_constraints = None;
> +        // preserve current device
> +        self.is_device_dirty = true;
> +        self.is_cleared = true;

I think it'd be slightly clearer to set is_cleared just after the bailout, but no big deal.

::: servo/components/style/stylist.rs:350
(Diff revision 1)
> +                      stylesheets_changed: bool,
> +                      author_style_disabled: bool,
> +                      extra_data: &mut ExtraStyleData<'a>) -> bool {
> +        // We have to do a dirtiness check before clearing, because if
> +        // we're not actually dirty we need to no-op here.
> +        if !(self.is_device_dirty || stylesheets_changed) {

I think this check should conceptually include self.is_cleared, but given is_cleared implies is_device_dirty, it wouldn't really matter.

Perhaps `debug_assert!(!self.is_cleared || self.is_device_dirty)` at the beginning of the function?
Attachment #8866401 - Flags: review?(emilio+bugs) → review+

Comment 9

a year ago
mozreview-review
Comment on attachment 8866402 [details]
Bug 1361843 part 2.  Track whether the stylist may need a rebuild in ServoStyleSet and do on-demand rebuilds as needed.

https://reviewboard.mozilla.org/r/138036/#review141160
Attachment #8866402 - Flags: review?(emilio+bugs) → review+

Comment 10

a year ago
mozreview-review
Comment on attachment 8866403 [details]
Bug 1361843 part 3.  Clear the stylist when styleshets change, but don't do anything else until there's an explicit sheet flush.

https://reviewboard.mozilla.org/r/138038/#review141164

I might be missing something, care to explain and re-request review?

::: servo/components/style/gecko/data.rs:125
(Diff revision 1)
> -                       &StylesheetGuards::same(guard),
> +                        &StylesheetGuards::same(guard),
> -                       /* ua_sheets = */ None,
> +                        /* ua_sheets = */ None,
> -                       /* stylesheets_changed = */ true,
> +                        /* stylesheets_changed = */ true,
> -                       author_style_disabled,
> +                        author_style_disabled,
> -                       &mut extra_data);
> +                        &mut extra_data);
> +        self.stylist_dirty = false;

Isn't the `stylist_dirty` state basically tracked internally by `Stylist::is_cleared`?

Given `Stylist::clear()` is actually a no-op when it's already cleared, is it really necessary to track it here?
Attachment #8866403 - Flags: review?(emilio+bugs)

Comment 11

a year ago
mozreview-review
Comment on attachment 8866404 [details]
Bug 1361843 part 4.  Remove the now-unused mBatching member of ServoStyleSet.

https://reviewboard.mozilla.org/r/138040/#review141166
Attachment #8866404 - Flags: review?(emilio+bugs) → review+

Comment 12

a year ago
mozreview-review
Comment on attachment 8866403 [details]
Bug 1361843 part 3.  Clear the stylist when styleshets change, but don't do anything else until there's an explicit sheet flush.

https://reviewboard.mozilla.org/r/138038/#review141178

We just talked about this. r=me with that bit removed.
Attachment #8866403 - Flags: review+
(Assignee)

Comment 13

a year ago
mozreview-review-reply
Comment on attachment 8866401 [details]
Bug 1361843 part 1.  Split up Stylist's update() method into separate clear() and rebuild() methods.

https://reviewboard.mozilla.org/r/138034/#review141144

> I think this check should conceptually include self.is_cleared, but given is_cleared implies is_device_dirty, it wouldn't really matter.
> 
> Perhaps `debug_assert!(!self.is_cleared || self.is_device_dirty)` at the beginning of the function?

> Perhaps debug_assert!(!self.is_cleared || self.is_device_dirty) at the beginning

Good idea, done.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8866401 - Attachment is obsolete: true

Comment 17

a year ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/97b43ea69c41
part 2.  Track whether the stylist may need a rebuild in ServoStyleSet and do on-demand rebuilds as needed.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/c64fde685fb1
part 3.  Clear the stylist when styleshets change, but don't do anything else until there's an explicit sheet flush.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/0324db517245
part 4.  Remove the now-unused mBatching member of ServoStyleSet.  r=emilio
https://hg.mozilla.org/mozilla-central/rev/97b43ea69c41
https://hg.mozilla.org/mozilla-central/rev/c64fde685fb1
https://hg.mozilla.org/mozilla-central/rev/0324db517245
Status: NEW → RESOLVED
Last Resolved: a year 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.