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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: , patch 1: Don't force calc() units out of the rule tree (for calc() that is fully computed in style data).

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: , patch 3: Don't force vw/vh/vmin/vmax units out of the rule tree.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: , patch 4: Refactor part of RebuildAllStyleData into a helper function (slightly reordering it), so that we can share that part.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: , patch 5: Handle dynamic changes to the basis for 'rem' units by rebuilding all style data.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: , patch 6: Don't force rem units out of the rule tree.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: , patch 2: Add test for dynamic changes of viewport units.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://tbpl.mozilla.org/?tree=Try&rev=6bea4fc94636

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 674617 [details] [diff] [review]
, patch 2: Add test for dynamic changes of viewport units.

r=me

Comment 12

[Boris Zbarsky [:bzbarsky]]

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...

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

   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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eac41b17bc52
https://hg.mozilla.org/mozilla-central/rev/0776bcfd7e0c
https://hg.mozilla.org/mozilla-central/rev/d7073db05fe0
https://hg.mozilla.org/mozilla-central/rev/ba9ee3171801
https://hg.mozilla.org/mozilla-central/rev/b707050a4ac4
https://hg.mozilla.org/mozilla-central/rev/8f8cc2ad5f88

Comment 20

[Fabien Cazenave [:kaze]]

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?

Comment 21

[Fabien Cazenave [:kaze]]

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.

Comment 22

[bhavana bajaj [:bajaj]]

(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:

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 28

[Alex Keybl [:akeybl]]

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?

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 30

[Alex Keybl [:akeybl]]

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.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/dee6da155653
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/6a102953fc82
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/0201c9ec104e
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/1f9ef298ceeb
(the last one had bug 806310 rolled in)