Closed Bug 1361843 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Bug 1361843 part 2. Track whether the stylist may need a rebuild in ServoStyleSet and do on-demand rebuilds as needed.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1361843 part 3. Clear the stylist when styleshets change, but don't do anything else until there's an explicit sheet flush.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1361843 part 4. Remove the now-unused mBatching member of ServoStyleSet.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
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 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 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 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 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 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+
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.
Attachment #8866401 - Attachment is obsolete: true
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
Blocks: 1348477
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: