Closed Bug 974125 Opened 11 years ago Closed 10 years ago

Changing CSS will-change should not reconstruct the frame

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36
Tracking Flags:
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: BenWa, Assigned: cwiiis)

References

Details

Attachments

(1 file)

Only RepaintFrame in response to changes in will-change
(deleted), patch
dbaron
: review+
fabrice
: approval-mozilla-b2g34+
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #964885 +++
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #7)
> >diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp
> >@@ -2462,17 +2464,18 @@ nsChangeHint nsStyleDisplay::CalcDiffere
> > 
> >-  if (mWillChangeBitField != aOther.mWillChangeBitField) {
> >+  if (mWillChangeBitField != aOther.mWillChangeBitField ||
> >+    mWillChangeStackingContext != aOther.mWillChangeStackingContext) {
> >     NS_UpdateHint(hint, nsChangeHint_ReconstructFrame);
> >   }
> 
> I think this is overkill in both cases (the existing BitField case, and your
> new case).  I believe RepaintFrame should be sufficient for
> mWillChangeStackingContext.
> 
> (The existing case is a little more complicated, but we should probably have
> a bug on file on improving it.  The whole thing about this being a property
> for performance optimization doesn't work out so well if it performs badly.)
>
Currently setting will-change:transform on an element will cause a 10-20ms reflow (on a flame).

This is preventing us from doing stuff like having will-change activated on some elements only while the user is interacting with the screen, and probably contributed to some of the will-change abuse.


Is it targeted for 2.1?
Flags: needinfo?(milan)
I presume you mean a style flush.

I believe we may need bug 931668 fixed first to address the performance there but you should check with :heycam.
Bug 931668 is slated for 2.1/34. I'm going to add the dependency on it, either way it will be useful to get pinged in this bug when bug 931668 gets fixed.
Depends on: 931668
Flags: needinfo?(milan)
Bug 931668 is fixed, so n?milan to see if there's any action that can be taken here.
Flags: needinfo?(milan)
From reading the code, it seems that RepaintFrame would be an adequate response to any will-change change now? will-change only affects layerisation, so a repaint should be enough now that bug 931668 is fixed, I think?

Will put a patch here for review rather than just asking and then doing that...
Thanks Chris, yes, let's see if we can keep nudging this and related issues along.  BenWa has been pinging people involved, and the big part was getting bug 931668 done.
Flags: needinfo?(milan)
It doesn't look like ReconstructFrame is necessary anymore. Seems to work fine locally, but I don't know enough to know what ways this would break things.

A try push: https://tbpl.mozilla.org/?tree=Try&rev=9bb7b655bea6
Attachment #8514389 - Flags: review?(dbaron)
Blocks: 1089621
Try is green, ftr, but I don't know how many tests we have covering will-change...
Comment on attachment 8514389 [details] [diff] [review]
Only RepaintFrame in response to changes in will-change

>   uint8_t willChangeBitsChanged =
>     mWillChangeBitField ^ aOther.mWillChangeBitField;
>-  if (willChangeBitsChanged & NS_STYLE_WILL_CHANGE_STACKING_CONTEXT) {
>+  if (willChangeBitsChanged) {
>     NS_UpdateHint(hint, nsChangeHint_RepaintFrame);
>   }
>-  if (willChangeBitsChanged & ~uint8_t(NS_STYLE_WILL_CHANGE_STACKING_CONTEXT)) {
>-    // FIXME (Bug 974125): Don't reconstruct the frame
>-    NS_UpdateHint(hint, nsChangeHint_ReconstructFrame);
>-  }

I checked all the places that check will-change, and I think this is ok, but only because of other code in the same function.

So r=dbaron if you add a comment that's something like:

    // Note that the HasTransformStyle() != aOther.HasTransformStyle() 
    // test above handles relevant changes in the
    // NS_STYLE_WILL_CHANGE_TRANSFORM bit, which in turn handles frame 
    // reconstruction for changes in the containing block of 
    // fixed-positioned elements.  Other than that, all changes to 
    // 'will-change' can be handled by a repaint.

(I'm not even convinced that we need to repaint for all of them, but one step at a time...)
Attachment #8514389 - Flags: review?(dbaron) → review+
Comment added, pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4400abcbec
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Don't we need the reframe hint to ensure NS_FRAME_MAY_BE_TRANSFORMED is set on the frame state bit when you add will-change:transform?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> Don't we need the reframe hint to ensure NS_FRAME_MAY_BE_TRANSFORMED is set
> on the frame state bit when you add will-change:transform?

Is that not handled by this block? http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp#2755
(Though perhaps the comment should mention that bit as well.)
https://hg.mozilla.org/mozilla-central/rev/2b4400abcbec
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8514389 [details] [diff] [review]
Only RepaintFrame in response to changes in will-change

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Adding/removing will-change causes a potentially expensive reflow
User impact if declined: Possible jank in some situations for Gaia apps using will-change
Testing completed: Tested locally and now in master
Risk to taking this patch (and alternatives if risky): Small risk that will-change will not work correctly, but tested to work fine and only Gaia certified apps can use will-change. Will-change not working doesn't break anything, but could cause worse animation/dragging performance
String or UUID changes made by this patch: None
Attachment #8514389 - Flags: approval-mozilla-b2g34?
ah, presumably this would require bug 931668 to also be uplifted... n?heycam to get feedback on what the risk for that is like.
Flags: needinfo?(cam)
(In reply to Chris Lord [:cwiiis] from comment #17)
> Comment on attachment 8514389 [details] [diff] [review]
> Only RepaintFrame in response to changes in will-change
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Adding/removing will-change causes
> a potentially expensive reflow
> User impact if declined: Possible jank in some situations for Gaia apps
> using will-change
> Testing completed: Tested locally and now in master
> Risk to taking this patch (and alternatives if risky): Small risk that
> will-change will not work correctly, but tested to work fine and only Gaia
> certified apps can use will-change. Will-change not working doesn't break
> anything, but could cause worse animation/dragging performance
> String or UUID changes made by this patch: None

cwiiis, given we want to avoid non-blocking change's on 2.1 at this point, I think this is fine to ride the trains. This isn't a 2.1 specific regression and any fallout that we may identify late worries me.
(In reply to Chris Lord [:cwiiis] from comment #18)
> ah, presumably this would require bug 931668 to also be uplifted... n?heycam
> to get feedback on what the risk for that is like.

I don't understand the relationship to bug 931668, but we shouldn't be uplifting bug 931668.
Comment on attachment 8514389 [details] [diff] [review]
Only RepaintFrame in response to changes in will-change

Let's let this ride the trains then, we can put up with the extra reflows in 2.1.
Flags: needinfo?(cam)
Attachment #8514389 - Flags: approval-mozilla-b2g34?
This shouldn't have any dependency on bug 931668 so you should be able to apply the patch by hand (you'll get conflicts due to some of bug 931668's other changes in CalcDifference) to older trees.
I don't understand this code very well, so I'd like to know why this bug was marked as dependent on bug 931668 in the first place if it doesn't need it... David, do you have any insight here?
Flags: needinfo?(dbaron)
comment 2 sounds like it's saying that this bug speeds things up, but to have the performance that we really want for some particular case, we also need bug 931668, which is a separate performance improvement.  I don't know why that led to a dependency being marked.
No longer depends on: 931668
Flags: needinfo?(dbaron)
Comment on attachment 8514389 [details] [diff] [review]
Only RepaintFrame in response to changes in will-change

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Reflows during edge-swiping
User impact if declined: Potential reduced performance
Testing completed: Tested on master for a while, no regressions filed
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

Uplifting this potentially increases out performance for edge-swiping gestures (and various other places we use will-change), but better, allows us to re-enable some tests we disabled when uplifting bug 1089621.
Attachment #8514389 - Flags: approval-mozilla-b2g34?
Note, I'm holding off on uplifting bug 1089621 to see if we can uplift this - it would be cleaner (and better performing) to uplift both, so I'd like to avoid having to make fix-up commits if we can uplift this also.
This is soft-blocking bug 1089621, a 2.1 blocker - can we get a decision on this soon, if possible?

To reiterate the situation: We can land bug 1089621 without this, but performance wouldn't be as good and we need to disable a test. If we land this low-risk patch, performance is improved and we can maintain our existing test coverage.
Flags: needinfo?(fabrice)
Attachment #8514389 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Flags: needinfo?(fabrice)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: