Closed Bug 961871 (will-change) Opened 6 years ago Closed 5 years ago

Enable will-change CSS property

Categories

(Core :: Layout, enhancement)

x86
macOS
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
relnote-firefox --- 36+

People

(Reporter: BenWa, Assigned: BenWa)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete)

Attachments

(6 files, 8 obsolete files)

1.22 KB, text/plain
Details
7.56 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
17.52 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
22.10 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
24.13 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
1.03 KB, patch
roc
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #940842 +++

We decided to first land will-change pref-ed off everywhere. This bug will track the requirements for turning it on.

Roc will like some abuse prevention. What other requirements do we have for turning it on everywhere once bug 940842 lands?
Severity: normal → enhancement
Working on 964885. Roc can you let me know what you have in mind for abuse protection so I can start on that once bug 964885 is ready?
Assignee: nobody → bgirard
Flags: needinfo?(roc)
I was thinking: if will-change applies to more than K elements in a document at one time (e.g. K=10), ignore it completely in that document.
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> I was thinking: if will-change applies to more than K elements in a document
> at one time (e.g. K=10), ignore it completely in that document.

I'd suggest that we make this based on area rather than number of elements - I could imagine wanting to animate lots (>10) of small elements. Though I suppose the layerisation cost of small elements isn't so high... At the same time, this is will-change though, rather than please-layerise, so if people use the annotation correctly, this use-case may end up sub-optimal when we ought to respect it.
I agree with Cwiiis. K layers deal with the two extremes very poorly:
- Small layers in a DOM game or many dynamic element something like this: https://www.mozilla.org/en-US*/firefox/os/. In this case we wont layerize the page even if we have more then 10 simple layers in view.
- Huge fullscreen layer. On mobile K of such layers wont fit in memory.
* This test case could set will-change a bit before the scroll offset of the animation is hit.

I don't think there are any value of K that is really effective.

We could allocate a budget of estimated layer memory usage (say proportional to the viewport) and perhaps even draw call and use that as a cap.

Perhaps at some point we could investigate having weighted inputs to trigger an active layer where in some cases will-change would be taken into account but may not be itself produce a layer. That way we wouldn't throw away information in platform where we have a small budget. We could weight it in the layer expiry timers as well.
I wasn't thinking about preventing resource exhaustion but about preventing abuse, i.e., authors who just set will-change on everything. I think resource exhaustion should be handled separately since there are lots of ways to produce it that don't involve will-change.
I.e. the goal here is to try to catch authors setting 'will-change' when they shouldn't, and stop them from getting a speedup. Preventing running out of VRAM is a different goal, and to do that we should modify layer construction in general.

A reasonable question is, if will-change hurts performance, why do we need to do anything to detect and punish abuse? I think a reasonable answer is that excessive use of will-change could easily help performance on desktop, or at least not hurt it, but hurt (a lot) on mobile. Maybe if people are developing sites mostly mobile-first or mobile-only, this answer is invalid.

Assuming we do need anti-abuse heuristics, I agree with comment #3 that a K-limit would catch some non-abusive cases, so it could be improved.

This is a tricky problem because the abuse we want to catch is when authors sprinkle will-change over a lot of elements and the property values for those values don't actually change. So we could detect that with timeouts. But there are cases, like scrolling the main element of an app, where we want to allow the value to not change for a long time but still keep the element layerized. Detecting the former but not breaking the latter is hard.

So, different idea #1: track the elements that have will-change and when it was set on each element. At any given time, allow up to K of those elements to have "will-change permanently honored" status on a first-come, first-served basis. For the rest of the elements, if the property does not actually change within some time limit T since will-change was set, ignore will-change on it forever.

Different idea #2: when will-change is set by a rule in a CSS stylesheet that matches exactly one element, honor it indefinitely. But if it's set in any other way, stop honoring it after a while if the property value doesn't actually change.

I don't like the complexity of those ideas but I don't have better ones.
Right, let's ignore the abuse case and focus on webdevs following the best practice.

The ideal implementation would let authors specify appropriate will-change and not have to think about what hardware the device will run. Having JS conditional logic would be a big failure IMO. Therefore I suggest that the browser engine allot a layers budget appropriate for that device. This budget could be 'no more then 5 Megapixel of layers, no layers smaller then a page in size before of high draw overhead in this platform'.

We then give priority to the normal active layers and the remaining will-change. If we run out then the will-change layers wont fit first and we might even skip active layers if we're constraint. This also solve our problem with getting too many active layers on mobile where we currently limit a container layer to 10 children which is a terrible band-aid.

Jeff suggest that the first time a will-change hint is ignored we could print to the console to notify that we're hitting a budget limit.

#1: I don't like the idea of tracking how long it takes for a will-change to provide a useful results and then distrusting the author. will-change should be use to optimize something as the result of user input (say homescreen panning). We're just going to cause a performance cliff if the user doesn't interact with the app within a delay and will penalize the author without providing them a good solution.

#2: Similarly we're penalizing the author when they might be doing something completely logical like |.ship { will-change: transform;}|.

Thus I vote we (1) Implement a page layer budget, (2) distribute the budget between LAYER_FORCE_ACTIVE (and never disallow), LAYER_ACTIVE, will-change in that order, (3) then focus on better detecting cases where layering an element is a performance regression on that device.
Depends on: 974125
Depends on: 975769
Depends on: 975789
Depends on: 980454
We discussed this during the Fall Graphics meetup. Here's our current plan for the budget:
- We're going to impose a limit based on the *CSS Pixel* surface area of the document. We're tentatively using 3x for that limit but that may be adjusted.
- We're NOT going to be using layer pixel because it is too difficult to calculate the layer pixel during the layer tree building phase (and async transform would make it inaccurate anyways) and also because it would make the heuristics more difficult for the users to stay within.
- If we fall outside of that budget ALL will-change property will be ignored.

I'm working out a few issues with how to track the budget and refactoring how active scrollable layer are determined.
Attached patch Part 1: Refactor ScrollingActive (obsolete) — Splinter Review
Attached patch Part 1: Refactor ScrollingActive (obsolete) — Splinter Review
Attachment #8501148 - Attachment is obsolete: true
Attachment #8501240 - Flags: review?(matt.woodrow)
Attachment #8501215 - Flags: review?(matt.woodrow)
Attachment #8501236 - Flags: review?(matt.woodrow)
Attachment #8501215 - Flags: review?(matt.woodrow) → review+
Attachment #8501236 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8501240 [details] [diff] [review]
Part 3: Implement will-change budget

Review of attachment 8501240 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.cpp
@@ +1111,5 @@
> +  if (!mWillChangeBudget.count(aFrame->PresContext())) {
> +    mWillChangeBudget[aFrame->PresContext()] = new DocumentWillChangeBudget();
> +  }
> +  mWillChangeBudget[aFrame->PresContext()]->mBudget +=
> +    std::max(64 * 64,

Comment about these magical numbers (even if it's just explaining that they were picked arbitrarily), and why we want them.

@@ +1118,5 @@
> +}
> +
> +bool
> +nsDisplayListBuilder::IsInWillChangeBudget(nsIFrame* aFrame) const {
> +  if (!mWillChangeBudget.count(aFrame->PresContext())) {

It would be nice if we only called this once we've already checked for will-change, then we could assert this condition since it would mean something missed calling AddToWillChangeBudget.

::: layout/base/nsDisplayList.h
@@ +720,5 @@
>    }
>  
>    DisplayListClipState& ClipState() { return mClipState; }
>  
> +  void AddToWillChangeBudget(nsIFrame* aFrame, const nsRect& aRect);

nsSize since we don't care about the position?

Add comments about how this is called during display list building for all frames that *want* will-change so we can make a per-document decision about whether it's accepted or not during layer building (when we call IsInWillChangeBudget).
Attachment #8501240 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #15)
> Comment about these magical numbers (even if it's just explaining that they
> were picked arbitrarily), and why we want them.
> 

+  // There's significant overhead for each layer created from Gecko
+  // (IPC+Shared Objects) and from the backend (like an OpenGL texture).
+  // Therefore we set a minimum cost threshold of a 64x64 area.

> It would be nice if we only called this once we've already checked for
> will-change, then we could assert this condition since it would mean
> something missed calling AddToWillChangeBudget.

done

> Add comments about how this is called during display list building for all
> frames that *want* will-change so we can make a per-document decision about
> whether it's accepted or not during layer building (when we call
> IsInWillChangeBudget).

 724   /**
 725    * The will-change budget is calculated during the display list building
 726    * phase for all the frames that want will change on a per-document basis.
 727    * The cost should be fully calculated during the layer building phase
 728    * and a decission to allow or disallow will-change for all frames of
 729    * that document will be made by IsInWillChangeBudget.
 730    */
I've also stopped using at()/count() and use find() instead.

https://tbpl.mozilla.org/?tree=Try&rev=2038505b3a50
Attachment #8501240 - Attachment is obsolete: true
Looks like nsDisplayTransform::Init decides if we pre-render. Its used for things like opacity calculation for instance.
Comment on attachment 8502571 [details]
Style flush querying if we're pre-rendering stack

Roc nsDisplayTransform needs to know if it's going to be an active layer during style flush/display list building. Do you think we can/should refactor that?
Flags: needinfo?(roc)
I think with that stack the transform we're computing in nsLayoutUtils::GetLayerTransformForFrame doesn't depend on whether we're prerendering or not. So we should be able to easily work around that.
Flags: needinfo?(roc)
For that one you're right. But in general we need to kill/refactor mPrerender because it looks like it's reads in non trivial ways during the display list phase ahead of the FrameLayerBuilder phase. For instance it's used to determine if the transform hierarchy is opaque. I know the visibility phase has changed significantly this quarter. I though the value was also used with OMTA but I can't find that now. Maybe we just need to fix the opacity/visibility?

The obvious proposal would be to make transform content always non-opaque.

http://mxr.mozilla.org/mozilla-central/ident?i=mPrerender
Flags: needinfo?(roc)
(In reply to Benoit Girard (:BenWa) from comment #22)
> The obvious proposal would be to make transform content always non-opaque.

i.e. just during the displaylist phase. We could still make the generated layer opaque. We would just de-tangle mPrerender and opacity.
We can change ShouldPrerender() to MayPrerender(). nsDisplayTransform::GetOpaqueRegion can return an empty region if MayPrerender(); that's safe if we eventually decide not to prerender. Does that help?
Flags: needinfo?(roc)
Had to also split IsStyle(Maybe)Animated.

Fully try run since all these changes are regression prone:
https://tbpl.mozilla.org/?tree=Try&rev=90acce4fdd4b
Found a failure. I'll look tomorrow into how to deal with mScrollPosForLayerPixelAlignment in ScrollFrameHelper::BuildDisplayList and not knowing yet for sure if we're going to get a layer or not.
Here's a patch that handles more cases that the assertions were catching.

https://tbpl.mozilla.org/?tree=Try&rev=804c8027f73d

One problem left is that we still layerize scrollbars if the will change is out of budget. I don't think that's a huge deal IMO.
I'm going to ask for 2 reviews for now since I'm touching a lot of stuff.

If a scrollframe is out of budget we still build the scrollbar layer(s). IMO this is fine for now, we may fix it later in a follow up but I don't think it's super easy or useful to fix.
Attachment #8508236 - Flags: review?(roc)
Attachment #8508236 - Flags: review?(matt.woodrow)
Comment on attachment 8508236 [details] [diff] [review]
Part 4: Fix assertions by splitting Maybe functions

Review of attachment 8508236 [details] [diff] [review]:
-----------------------------------------------------------------

This doesn't look like stuff I should review
Attachment #8508236 - Flags: review?(roc)
Attachment #8508236 - Attachment is obsolete: true
Attachment #8508236 - Flags: review?(matt.woodrow)
Attachment #8508270 - Flags: review?(roc)
Attachment #8508270 - Flags: review?(matt.woodrow)
Comment on attachment 8508270 [details] [diff] [review]
Part 4: Fix assertions by splitting Maybe functions

Review of attachment 8508270 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.h
@@ +3468,5 @@
>    bool CanUseAsyncAnimations(nsDisplayListBuilder* aBuilder) MOZ_OVERRIDE;
>  
> +  /**
> +   * This will return if it's possible for this element to be prerendered.
> +   * This should never return false if we're going to pre-render.

if we're *not* going to prerender
Attachment #8508270 - Flags: review?(roc) → review+
Comment on attachment 8502292 [details] [diff] [review]
Part 3: Implement will-change budget v3

Review of attachment 8502292 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.h
@@ +30,3 @@
>  
>  #include <stdint.h>
> +#include <map>

Use nsTHashtable instead of map.
Attachment #8502292 - Flags: review-
Attachment #8508270 - Flags: review?(matt.woodrow)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> Comment on attachment 8502292 [details] [diff] [review]
> Part 3: Implement will-change budget v3
> 
> Review of attachment 8502292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsDisplayList.h
> @@ +30,3 @@
> >  
> >  #include <stdint.h>
> > +#include <map>
> 
> Use nsTHashtable instead of map.

I'll make the change to avoid delaying this patch any further. But I think std::map is better in this context. The data we have is ordered, it's not performance critical, std::map has a better API.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31)
> Comment on attachment 8508270 [details] [diff] [review]
> Part 4: Fix assertions by splitting Maybe functions
> 
> Review of attachment 8508270 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsDisplayList.h
> @@ +3468,5 @@
> >    bool CanUseAsyncAnimations(nsDisplayListBuilder* aBuilder) MOZ_OVERRIDE;
> >  
> > +  /**
> > +   * This will return if it's possible for this element to be prerendered.
> > +   * This should never return false if we're going to pre-render.
> 
> if we're *not* going to prerender

I believe it's correct as written:
If MaybePrerender() == false then we should never ever decide to prerender later because the opacity will be wrong.
Attachment #8501215 - Attachment is obsolete: true
Attachment #8509157 - Flags: review+
Attachment #8502292 - Attachment is obsolete: true
Attachment #8509159 - Flags: review+
(In reply to Benoit Girard (:BenWa) from comment #41)
> Note: This doesn't enable the property yet. Lets let this stuff bake a bit.

Have we got a bug for the pref flipping?
Flags: needinfo?(bgirard)
No I pplan on doing it in this bug. I was waiting to see if the previous patches would cause regression. I'm probably going to do it this next week. I'll prepare an intent to ship e-mail and coordinate with documentation.
Flags: needinfo?(bgirard)
This property has been documented here: https://developer.mozilla.org/en-US/docs/Web/CSS/will-change

The doc currently says at the bottom that the attribute will be preffed on in Firefox 36 or 37. This bug will remain dev-doc-needed until know for certain and can update the doc accordingly.
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Release Note Request (optional, but appreciated)
[Why is this notable]: allows authors to hint for optimizations in animations
[Suggested wording]: Implemented CSS3 will-change property.
[Links (documentation, blog post, etc)]: ???
relnote-firefox: --- → ?
Target Milestone: --- → mozilla36
(In reply to Florian Bender from comment #50)
> [Links (documentation, blog post, etc)]: ???

https://developer.mozilla.org/en-US/docs/Web/CSS/will-change

I'll also be writing a hacks post before this is released.
Added to the release notes with "CSS3 will-change property implemented" as wording
Sylvestre: shouldn't the flag relnote-firefox be 36+ instead of 38+?
Flags: needinfo?(sledru)
Indeed. Thanks jetlag! ;)
Flags: needinfo?(sledru)
Depends on: 1501774
You need to log in before you can comment on or make changes to this bug.