Closed Bug 1442817 Opened 6 years ago Closed 6 years ago

Do not flush throttled animations for Element::GetAnimations()

Categories

(Core :: DOM: Animation, enhancement)

Product:
Core
Component:
DOM: Animation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: hiro, Assigned: hiro)

Details

Attachments

(3 files)

Bug 1442817 - Add another variant of nsDocument::FlushPendingNotifications which are able to skip to flushing throttled animations.
59 bytes, text/x-review-board-request
emilio
: review+
birtles
: review+
Details
Bug 1442817 - Introduce a function to observe synchronous restyling easily.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1442817 - Don't flush throttled animations on getAnimations().
59 bytes, text/x-review-board-request
birtles
: review+
Details 
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.
(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()".
Right, I am saying it's deletion case.
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.
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.
In the try above, I did forget to pass |aTarget| in the new FlushPendingNotifications(), silly!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da22a5a3f734fceae898a32a7d6f898eee20f11d
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 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)
(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 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+
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.
Attachment #8956386 - Flags: review?(bbirtles)
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 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 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+
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(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/
(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)
(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?
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: