Closed
Bug 961871
(will-change)
Opened 11 years ago
Closed 10 years ago
Enable will-change CSS property
Categories
(Core :: Layout, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla36
Tracking | Status | |
---|---|---|
relnote-firefox | --- | 36+ |
People
(Reporter: BenWa, Assigned: BenWa)
References
(Blocks 1 open bug, )
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?
Updated•11 years ago
|
Severity: normal → enhancement
Assignee | ||
Comment 1•11 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?
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)
Comment 3•11 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.
Assignee | ||
Comment 4•11 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.
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.
Assignee | ||
Comment 7•11 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.
That sounds reasonable.
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.
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8501215 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•10 years ago
|
Attachment #8501236 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 14•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9b653b7aae79
Updated•10 years ago
|
Attachment #8501215 -
Flags: review?(matt.woodrow) → review+
Updated•10 years ago
|
Attachment #8501236 -
Flags: review?(matt.woodrow) → review+
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).
Attachment #8501240 -
Flags: review?(matt.woodrow) → review+
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 */
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
Attachment #8501240 -
Attachment is obsolete: true
Assignee | ||
Comment 18•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8117094472c9
Attachment #8501970 -
Attachment is obsolete: true
Assignee | ||
Comment 19•10 years ago
|
||
Looks like nsDisplayTransform::Init decides if we pre-render. Its used for things like opacity calculation for instance.
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?
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)
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
Flags: needinfo?(roc)
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.
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)
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
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.
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.
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.
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)
Assignee | ||
Comment 30•10 years ago
|
||
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-
Updated•10 years ago
|
Attachment #8508270 -
Flags: review?(matt.woodrow)
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.
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.
Assignee | ||
Comment 37•10 years ago
|
||
Attachment #8501215 -
Attachment is obsolete: true
Attachment #8509157 -
Flags: review+
Assignee | ||
Comment 38•10 years ago
|
||
Attachment #8501236 -
Attachment is obsolete: true
Attachment #8509158 -
Flags: review+
Assignee | ||
Comment 39•10 years ago
|
||
Attachment #8502292 -
Attachment is obsolete: true
Attachment #8509159 -
Flags: review+
Assignee | ||
Comment 40•10 years ago
|
||
Attachment #8508270 -
Attachment is obsolete: true
Attachment #8509161 -
Flags: review+
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
Keywords: leave-open
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
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?
Flags: needinfo?(bgirard)
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.
Flags: needinfo?(bgirard)
Blocks: 1092144
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.
Depends on: 1092320
Attachment #8515144 -
Flags: review?(roc) → review+
Assignee | ||
Comment 47•10 years ago
|
||
Full run to be safe: https://tbpl.mozilla.org/?tree=Try&rev=7440f9ada2fa
Assignee | ||
Comment 48•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ce6a51c662
Assignee | ||
Updated•10 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
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)]: ???
relnote-firefox:
--- → ?
Target Milestone: --- → mozilla36
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.
Comment 52•10 years ago
|
||
I added a note in https://developer.mozilla.org/en-US/Firefox/Releases/36#CSS too. Thank you all!
Comment 53•10 years ago
|
||
Added to the release notes with "CSS3 will-change property implemented" as wording
Comment 54•10 years ago
|
||
Sylvestre: shouldn't the flag relnote-firefox be 36+ instead of 38+?
Flags: needinfo?(sledru)
Depends on: 1195142
Blocks: 1202908
You need to log in
before you can comment on or make changes to this bug.
Description
•