Closed
Bug 1442817
Opened 6 years ago
Closed 6 years ago
Do not flush throttled animations for Element::GetAnimations()
Categories
(Core :: DOM: Animation, enhancement)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
Details
Attachments
(3 files)
As far as I can tell, the flush for getAnimations() is needed for update pending CSS animations/transitions (I guess it's only for creation/deletion of them?). So even if there are throttled animations that are not affected by the CSS animations/transitions changes, we don't need to flush style for the throttled animations. I am not sure how hard it is though, but I think theoretically it can be done.
Comment 1•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #0) > As far as I can tell, the flush for getAnimations() is needed for update > pending CSS animations/transitions (I guess it's only for creation/deletion > of them?). It's also deletion in the sense of "animation-duration used to be 5s, but is now 3s, hence it should now be finished and not returned from getAnimations()".
Assignee | ||
Comment 2•6 years ago
|
||
Right, I am saying it's deletion case.
Assignee | ||
Comment 3•6 years ago
|
||
After some more thought about this, I think it can be easily done since if there are throttled animations that are affected by pending style update, we do also update the throttled animations by sequential task for EffectProperties or CascadeResults. So what we should for getAnimations() is just to flush pending styles like we do for normal styling process in refresh driver's tick.
Assignee | ||
Comment 4•6 years ago
|
||
I believe we can also skip flushing throttled animations in Animation::FlushStyle(). A try; https://treeherder.mozilla.org/#/jobs?repo=try&revision=10d9a29535179f36f8729f3453a4b1feab000fe8 I couldn't finish writing any test cases for that.
Assignee | ||
Comment 5•6 years ago
|
||
In the try above, I did forget to pass |aTarget| in the new FlushPendingNotifications(), silly! https://treeherder.mozilla.org/#/jobs?repo=try&revision=da22a5a3f734fceae898a32a7d6f898eee20f11d
Assignee | ||
Comment 6•6 years ago
|
||
It seems that old style system does one more redundant restyle in these cases for some reasons. I haven't dug into it yet. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a402e38e0ee5f3c2d2c7cd05553de021f07be8d
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=594e617d64ee08741e07b57002436ee6f340e42c
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8956386 [details] Bug 1442817 - Add another variant of nsDocument::FlushPendingNotifications which are able to skip to flushing throttled animations. https://reviewboard.mozilla.org/r/225268/#review231236 This part in particular looks ok, though I suspect the whole series is just working around a deeper issue. I don't understand why flushing styles without flushing throttled animations _just in `getAnimations`_ is ok. Why is `element.getAnimations()` different than `getComputedStyle(element).color; element.getAnimations();` (or `anythingElseThatFlushesStyle(); element.getAnimations()`? Wouldn't that similarly expose the bug? Looks like the underlying issue is somewhere else. ::: dom/base/nsDocument.h:533 (Diff revision 1) > virtual void FlushPendingNotifications(mozilla::FlushType aType, > mozilla::FlushTarget aTarget > = mozilla::FlushTarget::Normal) override; > + virtual void FlushPendingNotifications(mozilla::ChangesToFlush aFlush, > + mozilla::FlushTarget aTarget > + = mozilla::FlushTarget::Normal) override; Should be `final`. ::: dom/base/nsDocument.cpp:7608 (Diff revision 1) > } > > void > nsDocument::FlushPendingNotifications(FlushType aType, FlushTarget aTarget) > { > + // by default, flush animations if aType >= FlushType::Style We should really convert the `mFlushAnimations` boolean in something like actually talks about throttled animations... Though that's pre-existing of course, so no worries for now :)
Attachment #8956386 -
Flags: review?(emilio)
Assignee | ||
Comment 12•6 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #11) > Comment on attachment 8956386 [details] > Bug 1442817 - Add another variant of nsDocument::FlushPendingNotifications > which are able to skip to flushing throttled animations. > > https://reviewboard.mozilla.org/r/225268/#review231236 > > This part in particular looks ok, though I suspect the whole series is just > working around a deeper issue. > > I don't understand why flushing styles without flushing throttled animations > _just in `getAnimations`_ is ok. > > Why is `element.getAnimations()` different than > `getComputedStyle(element).color; element.getAnimations();` (or > `anythingElseThatFlushesStyle(); element.getAnimations()`? > > Wouldn't that similarly expose the bug? Looks like the underlying issue is > somewhere else. As I told on IRC, this is an optimization. And yes, we can do the same optimization for Element::GetPrimaryFrame() in most cases. We can't optimize GetPrimaryFrame in Element::GetScrollFrame() and some other functions related to transform, but for example, for Element::FontSizeInflation() we don't need to flush throttled transform animation, as far as I can tell.
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8956386 [details] Bug 1442817 - Add another variant of nsDocument::FlushPendingNotifications which are able to skip to flushing throttled animations. https://reviewboard.mozilla.org/r/225268/#review231242 Yeah, per IRC discussion, given this is just intended as an optimization, it looks fine. We should probably make the animation thing an enum class so that callers are clear that this only has to do with throttled / compositor animations. Thanks for the explanation! r=me with that function marked as final, or actually just in nsIDocument directly, without it being virtual.
Attachment #8956386 -
Flags: review+
Assignee | ||
Comment 14•6 years ago
|
||
Thank you for the review and the discussion. The discussion was quite useful for me. Now I think getComptuedStyle() also fluses unrelated throttled animations as well. We can do the same optimization there.
Assignee | ||
Updated•6 years ago
|
Attachment #8956386 -
Flags: review?(bbirtles)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8956386 [details] Bug 1442817 - Add another variant of nsDocument::FlushPendingNotifications which are able to skip to flushing throttled animations. https://reviewboard.mozilla.org/r/225268/#review231906 ::: dom/base/nsDocument.cpp:7601 (Diff revision 2) > } > > void > nsIDocument::FlushPendingNotifications(FlushType aType) > { > + // by default, flush animations if aType >= FlushType::Style s/by default/By default/ But actually, I don't think we need "by default" at all. Indeed, I think don't think comment adds anything useful and we can just drop it. ::: dom/base/nsDocument.cpp:7656 (Diff revision 2) > - FlushType parentType = aType; > - if (aType >= FlushType::Style) > - parentType = std::max(FlushType::Layout, aType); > + mozilla::ChangesToFlush parentFlush = aFlush; > + if (parentFlush.mFlushType >= FlushType::Style) { > + parentFlush.mFlushType = std::max(FlushType::Layout, flushType); This is a bit weird to compare the value of |parentFlush.mFlushType| but then pass |flushType| to std::max. I understand that they will have the same value at this point but I think it might make sense to just use |flushType| in both places. ::: dom/base/nsIDocument.h:1731 (Diff revision 2) > */ > void FlushPendingNotifications(mozilla::FlushType aType); > > /** > + * Another variant of the above FlushPendingNotifications. This function > + * takes ChangesToFlush to specify whether throttled animations is flushed or s/takes ChangesToFlush/takes a ChangesToFlush argument/ s/is flushed/are flushed/ ::: dom/base/nsIDocument.h:1733 (Diff revision 2) > + * If you are unsure about animations, use the above > + * FlushPendingNotifications. "If in doubt, use the above FlushPendingNotifications which will flush throttled animations whenever style is flushed."
Attachment #8956386 -
Flags: review?(bbirtles) → review+
Comment 19•6 years ago
|
||
mozreview-review |
Comment on attachment 8956387 [details] Bug 1442817 - Introduce a function to observe synchronous restyling easily. https://reviewboard.mozilla.org/r/225270/#review231908 ::: dom/animation/test/mozilla/file_restyles.html:55 (Diff revision 2) > </head> > <body> > <script> > 'use strict'; > > -function observeStyling(frameCount, onFrame) { > +function setupDocShellForRestyleObserver() { s/setupDocShellForRestyleObserver/getDocShellForObservingRestyles/ ? ::: dom/animation/test/mozilla/file_restyles.html:56 (Diff revision 2) > <body> > <script> > 'use strict'; > > -function observeStyling(frameCount, onFrame) { > - var docShell = > +function setupDocShellForRestyleObserver() { > + let docShell = nit: s/let/const/ ::: dom/animation/test/mozilla/file_restyles.html:68 (Diff revision 2) > docShell.popProfileTimelineMarkers(); > > + return docShell; > +} > + > +// Returns observered animation restyle markers during refresh driver tick s/observered/observed/ Nit: better still, change the order of words in this sentence: "Returns the animation restyle markers observed during |frameCount| refresh driver ticks." ::: dom/animation/test/mozilla/file_restyles.html:69 (Diff revision 2) > +// happens |frameCount| times. Normally this function is used for restyle count > +// in scheduled restyling process. If |onFrame| is supplied, it gets called Replace: "Normally...." With: "This function is typically used to count the number of restyles that take place as part of the style update that happens on each refresh driver tick, as opposed to synchronous restyles triggered by script. For the latter, observeSyncStyling (below) should be used." (And then probably add a blank link before the final sentence about |onFrame|.) ::: dom/animation/test/mozilla/file_restyles.html:87 (Diff revision 2) > resolve(stylingMarkers); > }); > }); > } > > +// Returns observered animation restyle markers when |funcToMakeRestyleHappen| s/observered/observed/ ::: dom/animation/test/mozilla/file_restyles.html:90 (Diff revision 2) > +// Unlike the above observeStyling, this function is supposed to be used for > +// |funcToMakeRestyleHappen| causes synchronous restyling. "Unlike the above observeStyling, this function takes a callback function, |funcToMakeRestyleHappen|, which may be expected to trigger a synchronous restyle, and returns any restyle markers produced by calling that function."
Attachment #8956387 -
Flags: review?(bbirtles) → review+
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8956388 [details] Bug 1442817 - Don't flush throttled animations on getAnimations(). https://reviewboard.mozilla.org/r/225272/#review231910 ::: dom/animation/CSSPseudoElement.cpp:59 (Diff revision 2) > - doc->FlushPendingNotifications(FlushType::Style); > + // We don't need explicitly to flush throttled animations here since if this > + // element has throttled animations and the throttled animations will be > + // discarded by pending style changes, we end up discarding them through > + // UpdateEffectProperties, UpdateCascadeResults etc. in sequential tasks. Is this comment right? I thought the only reason this flush was needed was to make sure we return the correct set of animations? So isn't what we want to say: "We don't need to explicitly flush throttled animations here, since update the animation style of (pseudo-)elements will never affect the set of running animations and it's only the set of running animations that is important here." ::: dom/animation/test/mozilla/file_restyles.html:1673 (Diff revision 2) > + 100 * MS_PER_SEC); > + await animation.ready; > + > + ok(SpecialPowers.wrap(animation).isRunningOnCompositor); > + > + var markers = observeSyncStyling(() => { (Reading this here, it seems like we actually want to name these functions observeAnimSyncStyling -- i.e. getAnimations() *should* trigger a restyle, just not an animation one. So it seems like these two methods should have "Anim" somewhere in their name.) ::: dom/animation/test/mozilla/file_restyles.html:1696 (Diff revision 2) > + 'Element.getAnimations() should flush throttled animation style that ' + > + 'the throttled animation is discarded'); s/style that/style so that/ ::: dom/animation/test/mozilla/file_restyles.html:1700 (Diff revision 2) > + is(markers.length, 1, // For discarding the throttled animation. > + 'Element.getAnimations() should flush throttled animation style that ' + > + 'the throttled animation is discarded'); > + } else { > + is(markers.length, 2, // One is done through UpdateOnlyAnimationStyles(), > + // the other is for dicarding the animation. s/dicarding/discarding/ ::: dom/base/Element.cpp:3825 (Diff revision 2) > - doc->FlushPendingNotifications(FlushType::Style); > + // We don't need explicitly to flush throttled animations here since if this > + // element has throttled animations and the throttled animations will be > + // discarded by pending style changes, we end up discarding them through > + // UpdateEffectProperties, UpdateCascadeResults etc. in sequential tasks. Likewise, is this comment what we really want to say here?
Attachment #8956388 -
Flags: review?(bbirtles) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b703e510004f7780e7e3dc2cb37b75858240f22f
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Assignee | ||
Comment 28•6 years ago
|
||
getAnimations() test cases failed on Android. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b703e510004f7780e7e3dc2cb37b75858240f22f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&selectedJob=166713755 I will check it tomorrow.
Flags: needinfo?(hikezoe)
Comment 29•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #20) > Comment on attachment 8956388 [details] > Bug 1442817 - Don't flush throttled animations on getAnimations(). ... > So isn't what we want to say: > > "We don't need to explicitly flush throttled animations here, since update > the animation style of (pseudo-)elements will never affect the set of > running animations and it's only the set of running animations that is > important here." Sorry, I made a mistake here. s/since update/since updating/
Assignee | ||
Comment 30•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #28) > getAnimations() test cases failed on Android. > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=b703e510004f7780e7e3dc2cb37b75858240f22f&filter- > resultStatus=testfailed&filter-resultStatus=busted&filter- > resultStatus=exception&filter-resultStatus=retry&filter- > resultStatus=usercancel&filter-resultStatus=running&filter- > resultStatus=pending&filter-resultStatus=runnable&selectedJob=166713755 > I will check it tomorrow. The failure is intermittent and it seems to be caused by unthrottled transform animations every 200ms. Using opacity seems to solve the failure. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f16c287c534da15e0783c380321f1ea0c9410d8d&selectedJob=166877888
Flags: needinfo?(hikezoe)
Assignee | ||
Comment 31•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #30) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #28) > > getAnimations() test cases failed on Android. > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=b703e510004f7780e7e3dc2cb37b75858240f22f&filter- > > resultStatus=testfailed&filter-resultStatus=busted&filter- > > resultStatus=exception&filter-resultStatus=retry&filter- > > resultStatus=usercancel&filter-resultStatus=running&filter- > > resultStatus=pending&filter-resultStatus=runnable&selectedJob=166713755 > > I will check it tomorrow. > > The failure is intermittent and it seems to be caused by unthrottled > transform animations every 200ms. Using opacity seems to solve the failure. > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=f16c287c534da15e0783c380321f1ea0c9410d8d&selectedJob=1 > 66877888 Note that I can't reproduce the failure locally, probably our infra on try servers is slower than my local machine?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 35•6 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d7bf0574ec4a Add another variant of nsDocument::FlushPendingNotifications which are able to skip to flushing throttled animations. r=birtles,emilio https://hg.mozilla.org/integration/autoland/rev/127daa70875b Introduce a function to observe synchronous restyling easily. r=birtles https://hg.mozilla.org/integration/autoland/rev/32826732144d Don't flush throttled animations on getAnimations(). r=birtles
Comment 36•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d7bf0574ec4a https://hg.mozilla.org/mozilla-central/rev/127daa70875b https://hg.mozilla.org/mozilla-central/rev/32826732144d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•