Benoit Girard (:BenWa) Assignee
	Description • 10 years ago

+++ 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?

	Benoit Girard (:BenWa) Assignee
	Comment 1 • 10 years ago

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?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 2 • 10 years ago

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.

	Chris Lord [:cwiiis]
	Comment 3 • 10 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 4 • 10 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 5 • 10 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 6 • 10 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 7 • 10 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 8 • 10 years ago

That sounds reasonable.

	Benoit Girard (:BenWa) Assignee
	Comment 9 • 10 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 14 • 10 years ago

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9b653b7aae79

	Matt Woodrow (:mattwoodrow)
	Comment 15 • 10 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 16 • 10 years ago

(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    */

	Benoit Girard (:BenWa) Assignee
	Comment 17 • 10 years ago

I've also stopped using at()/count() and use find() instead.

https://tbpl.mozilla.org/?tree=Try&rev=2038505b3a50

	Benoit Girard (:BenWa) Assignee
	Comment 18 • 10 years ago

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8117094472c9

	Benoit Girard (:BenWa) Assignee
	Comment 19 • 10 years ago

Looks like nsDisplayTransform::Init decides if we pre-render. Its used for things like opacity calculation for instance.

	Benoit Girard (:BenWa) Assignee
	Comment 20 • 10 years ago

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?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 21 • 10 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 22 • 10 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 23 • 10 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 24 • 10 years ago

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?

	Benoit Girard (:BenWa) Assignee
	Comment 25 • 10 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 26 • 10 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 27 • 10 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 28 • 10 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 29 • 10 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 31 • 10 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 32 • 10 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 33 • 10 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 34 • 10 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 35 • 10 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 36 • 10 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 41 • 10 years ago

Note: This doesn't enable the property yet. Lets let this stuff bake a bit.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5a21befb60a1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9f7ffa570839
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aea7f440fb61
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c62411c27b55

	Ryan VanderMeulen [:RyanVM]
	Comment 42 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/5a21befb60a1
https://hg.mozilla.org/mozilla-central/rev/9f7ffa570839
https://hg.mozilla.org/mozilla-central/rev/aea7f440fb61
https://hg.mozilla.org/mozilla-central/rev/c62411c27b55

	Jet Villegas (inactive)
	Comment 43 • 10 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 44 • 10 years ago

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.

	Eric Shepherd [:sheppy]
	Comment 46 • 10 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 47 • 10 years ago

Full run to be safe:
https://tbpl.mozilla.org/?tree=Try&rev=7440f9ada2fa

	Benoit Girard (:BenWa) Assignee
	Comment 48 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ce6a51c662

	Ryan VanderMeulen [:RyanVM]
	Comment 49 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/d9ce6a51c662

	Florian Bender
	Comment 50 • 10 years ago

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)]: ???

	Benoit Girard (:BenWa) Assignee
	Comment 51 • 10 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 52 • 10 years ago

I added a note in https://developer.mozilla.org/en-US/Firefox/Releases/36#CSS too. Thank you all!

	Sylvestre Ledru [:Sylvestre]
	Comment 53 • 10 years ago

Added to the release notes with "CSS3 will-change property implemented" as wording

	Jean-Yves Perrier [:teoli]
	Comment 54 • 10 years ago

Sylvestre: shouldn't the flag relnote-firefox be 36+ instead of 38+?

	Sylvestre Ledru [:Sylvestre]
	Comment 55 • 10 years ago

Indeed. Thanks jetlag! ;)