Closed Bug 1386602 Opened 7 years ago Closed 7 years ago

stylo: We rebuild stylesheets most of the time when we don't need to when media query affecting values change.

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

The semantics of RebuildAllStyleData are rather funny, and they don't expect the actual Stylist to be rebuilt most of the time.

Also, servo doesn't need to track most of the rule tree stuff, since we don't cache style data in there.

Most of the time we only need to recompute the default computed values and probably clear the non-inheriting anon contexts, though even those I think aren't needed, because they shouldn't depend on media related stuff or prefs.

I'm working on this and have some WIP patches, but need to clean it up still.
Comment on attachment 8893277 [details]
Bug 1386602: Avoid recreating the stylist in RebuildAllStyleData.

https://reviewboard.mozilla.org/r/164330/#review169672

Great, I think this separation makes a lot more sense than before!

::: layout/base/ServoRestyleManager.cpp:218
(Diff revision 1)
>  void
>  ServoRestyleManager::PostRebuildAllStyleDataEvent(nsChangeHint aExtraHint,
>                                                    nsRestyleHint aRestyleHint)
>  {
> -  StyleSet()->ClearDataAndMarkDeviceDirty();
> +  // NOTE(emilio): The semantics of these methods are quite funny, in the sense
> +  // that we're not supposed to need to rebuild the actual stylist data.
> +  //
> +  // That's handled as part of the MediumFeaturesChanged stuff, if needed.

I guess "AllStyleData" is not very clear about what it includes.  Do you want to rename the method to be more accurate about what it needs to clear?  Or at least can you document it on the declaration on RestyleManager (and on nsPresContext)?

::: layout/style/ServoStyleSet.cpp:119
(Diff revision 1)
> -  return Servo_StyleSet_MediumFeaturesChanged(mRawSet.get(), aViewportChanged);
> +  nsRestyleHint hint =
> +    Servo_StyleSet_MediumFeaturesChanged(mRawSet.get(), aViewportChanged);
> +  if (hint == eRestyle_Subtree) {
> +    mStylistState = StylistState::StyleSheetsDirty;
> +  }
> +  return hint;

That we have knowledge here about what the different returned hint values means is not ideal.  It might be better to have Servo_StyleSet_MediumFeaturesChanged return a separate enum with values like RulesChanged, ViewportChanged, and Nothing, and then determining the restyle hint to return here in this function.
Attachment #8893277 - Flags: review?(cam) → review+
Cervantes, this might be the cause of the additional work you were seeing in Gmail when we were rebuilding the user font set.  Once it lands, you might like to re-profile.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a59e4b5a87ac
Avoid recreating the stylist in RebuildAllStyleData. r=heycam
Blocks: 1386580
https://hg.mozilla.org/mozilla-central/rev/a59e4b5a87ac
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: