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)
Core
CSS Parsing and Computation
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 hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
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+
Comment 3•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a59e4b5a87ac
Avoid recreating the stylist in RebuildAllStyleData. r=heycam
Comment 6•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
status-firefox55:
--- → wontfix
status-firefox56:
--- → wontfix
status-firefox-esr52:
--- → unaffected
Comment 7•7 years ago
|
||
https://hg.mozilla.org/projects/date/rev/a59e4b5a87acd31027896ec9855d18241b698c33
Bug 1386602: Avoid recreating the stylist in RebuildAllStyleData. r=heycam
You need to log in
before you can comment on or make changes to this bug.
Description
•