make fewer CSS units prevent styles from being cached in the rule tree

RESOLVED FIXED in Firefox 18

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18 fixed)

Details

Attachments

(6 attachments)

(Assignee)

Description

6 years ago
Having things cached in the rule tree is good; it makes style reresolution cheaper, which is particularly important when non-inherited styles are changed on an ancestor (e.g., dynamic transform changes).

A bunch of CSS length units force styles not to be cached in the rule tree.  For em/ex/ch this is necessary because of the dependency on font data (though I have some interesting ideas for fixing that too).  But a bunch of other units got added to the same code, in some cases intentionally (rem), and in some cases unnecessarily (calc()), and in some cases originally intentionally but then it turned out to be unnecessary (vh/vw/vmin/vmax).

To some degree this is followup on bug 780341 comment 48, to prevent rem units from being cached in the rule tree.  But it also fixes two other simpler problems (calc(), viewport units), where units prevent things from being cached in the rule tree for no good reason.
(Assignee)

Comment 1

6 years ago
Created attachment 674611 [details] [diff] [review]
, patch 1: Don't force calc() units out of the rule tree (for calc() that is fully computed in style data).
Attachment #674611 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

6 years ago
Created attachment 674612 [details] [diff] [review]
, patch 3: Don't force vw/vh/vmin/vmax units out of the rule tree.
Attachment #674612 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

6 years ago
Created attachment 674614 [details] [diff] [review]
, patch 4: Refactor part of RebuildAllStyleData into a helper function (slightly reordering it), so that we can share that part.
Attachment #674614 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

6 years ago
Created attachment 674615 [details] [diff] [review]
, patch 5: Handle dynamic changes to the basis for 'rem' units by rebuilding all style data.
Attachment #674615 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

6 years ago
Created attachment 674616 [details] [diff] [review]
, patch 6: Don't force rem units out of the rule tree.
Attachment #674616 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

6 years ago
Created attachment 674617 [details] [diff] [review]
, patch 2: Add test for dynamic changes of viewport units.
Attachment #674617 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

6 years ago
(In reply to David Baron [:dbaron] from comment #0)
> For em/ex/ch this is necessary because of the dependency on font data
> (though I have some interesting ideas for fixing that too).

These are filed as bug 804975.
Comment on attachment 674611 [details] [diff] [review]
, patch 1: Don't force calc() units out of the rule tree (for calc() that is fully computed in style data).

r=me
Attachment #674611 - Flags: review?(bzbarsky) → review+
Comment on attachment 674612 [details] [diff] [review]
, patch 3: Don't force vw/vh/vmin/vmax units out of the rule tree.

The "common code" comment may want updating.  Maybe.

r=me
Attachment #674612 - Flags: review?(bzbarsky) → review+
Comment on attachment 674617 [details] [diff] [review]
, patch 2: Add test for dynamic changes of viewport units.

r=me
Attachment #674617 - Flags: review?(bzbarsky) → review+
Comment on attachment 674614 [details] [diff] [review]
, patch 4: Refactor part of RebuildAllStyleData into a helper function (slightly reordering it), so that we can share that part.

Why is it ok to do the EndReconstruct() before the ProcessPendingRestyles()?  The comments claim that's not OK...
Attachment #674614 - Flags: review?(bzbarsky) → review-
Comment on attachment 674615 [details] [diff] [review]
, patch 5: Handle dynamic changes to the basis for 'rem' units by rebuilding all style data.

The ResolveStyleFor call can just pass null as last arg, since we just tested !oldContext->GetParent().

r=me
Attachment #674615 - Flags: review?(bzbarsky) → review+
Comment on attachment 674616 [details] [diff] [review]
, patch 6: Don't force rem units out of the rule tree.

r=me
Attachment #674616 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 15

6 years ago
Comment on attachment 674614 [details] [diff] [review]
, patch 4: Refactor part of RebuildAllStyleData into a helper function (slightly reordering it), so that we can share that part.

The comments claim it has to be after the ProcessRestyledFrames, not that it has to be after the ProcessPendingRestyles (which is for animation restyles).  (And we certainly don't want the ProcessPendingRestyles in the new caller of DoRebuildAllStyleData, since that's within the handling of a single restyle tracker.)

(I think if this reordering isn't safe, then the thing to do is leave the copy to DoRebuildAllStyleData as-is, and just leave RebuildAllStyleData unmodified.  But I don't see why it wouldn't be safe.)
Attachment #674614 - Flags: review- → review?(bzbarsky)
(Assignee)

Comment 16

6 years ago
To be clear, the ordering constraints I'm aware of are:
  SetProcessingRestyles(true) < ComputeStyleChangeFor < SetProcessingRestyles(false)
  BeginReconstruct < ComputeStyleChangeFor < ProcessRestyledFrames < EndReconstruct
  ComputeStyleChangeFor < ProcessPendingRestyles
  ProcesRestyledFrames < ProcessPendingRestyles
  SetProcessingRestyles(false) < ProcessPendingRestyles
Comment on attachment 674614 [details] [diff] [review]
, patch 4: Refactor part of RebuildAllStyleData into a helper function (slightly reordering it), so that we can share that part.

Ah, right.  r=me
Attachment #674614 - Flags: review?(bzbarsky) → review+
Comment on attachment 674616 [details] [diff] [review]
, patch 6: Don't force rem units out of the rule tree.

Sounds interesting to have this in FxOS, would it make sense to backport this patch to Aurora?
Attachment #674616 - Flags: approval-mozilla-aurora?
Comment on attachment 674611 [details] [diff] [review]
, patch 1: Don't force calc() units out of the rule tree (for calc() that is fully computed in style data).

Same here, as we’re using a lot of `calc()' in FxOS it might be interesting to backport this patch to Aurora as well.
Attachment #674611 - Flags: approval-mozilla-aurora?
(In reply to David Baron [:dbaron] from comment #18)
>    https://hg.mozilla.org/integration/mozilla-inbound/rev/eac41b17bc52
>    https://hg.mozilla.org/integration/mozilla-inbound/rev/0776bcfd7e0c
>    https://hg.mozilla.org/integration/mozilla-inbound/rev/d7073db05fe0
>    https://hg.mozilla.org/integration/mozilla-inbound/rev/ba9ee3171801
>    https://hg.mozilla.org/integration/mozilla-inbound/rev/b707050a4ac4
>    https://hg.mozilla.org/integration/mozilla-inbound/rev/8f8cc2ad5f88

Cab you please help us by filling the approval request comment used for aurora noms and also outline any risk to desktop/mobile ? Thanks !
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
String or UUID changes made by this patch:
(Assignee)

Comment 23

6 years ago
Comment on attachment 674616 [details] [diff] [review]
, patch 6: Don't force rem units out of the rule tree.

This patch depends on patches 4 and 5.
(Assignee)

Updated

6 years ago
Attachment #674611 - Attachment description: , patch 1: Don't force calc() units out of the rule tree. → , patch 1: Don't force calc() units out of the rule tree (for calc() that is fully computed in style data).
Attachment #674611 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

6 years ago
Attachment #674616 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 24

6 years ago
Comment on attachment 674611 [details] [diff] [review]
, patch 1: Don't force calc() units out of the rule tree (for calc() that is fully computed in style data).

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 363249 (patch 5)
User impact if declined: Slower performance and increased memory use
Testing completed (on m-c, etc.): on mozilla-central for a week
Risk to taking this patch (and alternatives if risky): Very low; this moves code above the point where we set aCanStoreInRuleTree = false, which essentially just removes the unnecessary assignment for these cases.  This de-optimization (disabling storage in the rule tree, which is a performance and memory optimization) was unnecessary for this case.  I think the primary risk is that there's some case where this deoptimization was covering up some other bug.
String or UUID changes made by this patch: None.
Attachment #674611 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 25

6 years ago
Comment on attachment 674614 [details] [diff] [review]
, patch 4: Refactor part of RebuildAllStyleData into a helper function (slightly reordering it), so that we can share that part.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 472195
User impact if declined: Slower performance for dynamic changes and increased memory use when 'rem' units are used.
Testing completed (on m-c, etc.): on mozilla-central
Risk to taking this patch (and alternatives if risky): This patch is a little bit risky in that it reorders some existing code for handling style changes so that part of it can be reused by another caller.  The alternative (which might be more reasonable for the branch than it is for trunk) would be to just copy the necessary parts of the code into the second caller (which is in patch 5).  However, I suspect it should be ok given that we haven't heard anything bad about it yet.
String or UUID changes made by this patch: None.
Attachment #674614 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 26

6 years ago
Comment on attachment 674615 [details] [diff] [review]
, patch 5: Handle dynamic changes to the basis for 'rem' units by rebuilding all style data.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 472195
User impact if declined: Slower performance for dynamic changes and increased memory use when 'rem' units are used.
Testing completed (on m-c, etc.): on mozilla-central
Risk to taking this patch (and alternatives if risky):  This adds a new codepath for handling dynamic changes of the root element's font size when 'rem' units are present.  This is a rare edge case, and it is tested by a mochitest, so I think it's reasonably safe.
String or UUID changes made by this patch: None.
Attachment #674615 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 27

6 years ago
Comment on attachment 674616 [details] [diff] [review]
, patch 6: Don't force rem units out of the rule tree.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 472195
User impact if declined: Slower performance for dynamic changes and increased memory use when 'rem' units are used.
Testing completed (on m-c, etc.): on mozilla-central
Risk to taking this patch (and alternatives if risky):  This is the performance and memory use improvement for 'rem' units, made possible by the new dynamic change handling codepath in patch 5.  Like patch 1, I think the primary risk is that there's a very low chance this de-optimization was covering up some other bug.  Note that the diff for this patch is confusing since instead of showing one chunk of code being moved up (which is what it's doing, logically), it's showing the code it's being moved across being moved down.
String or UUID changes made by this patch: None.
Attachment #674616 - Flags: approval-mozilla-aurora?
We're very close to the merge to beta, and these appear to be longstanding issues (from our read). What's the rationale behind taking this in FF18 instead of FF19 for the first time?
(Assignee)

Comment 29

6 years ago
The rationale was that it's about a 2% performance improvement on a testcase reduced from panning the B2G homescreen; not nearly as much as the 14% win from bug 572200, though.
Comment on attachment 674611 [details] [diff] [review]
, patch 1: Don't force calc() units out of the rule tree (for calc() that is fully computed in style data).

[Triage Comment]
These changes are fairly low risk, and should have good coverage in our test suites. Given this is only a 2% increase in home screen performance, if any major regressions are found we'll likely back out.
Attachment #674611 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Attachment #674614 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Attachment #674615 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Attachment #674616 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.