Move EnsureStyleRule to EffectCompositor

RESOLVED FIXED in Firefox 46

Status

()

Core
DOM: Animation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

(Blocks: 1 bug)

Trunk
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(19 attachments, 21 obsolete attachments)

11.12 KB, patch
heycam
: review+
Details | Diff | Splinter Review
10.12 KB, patch
heycam
: review+
Details | Diff | Splinter Review
6.30 KB, patch
heycam
: review+
Details | Diff | Splinter Review
1.86 KB, patch
heycam
: review+
Details | Diff | Splinter Review
9.55 KB, patch
heycam
: review+
Details | Diff | Splinter Review
3.08 KB, patch
heycam
: review+
Details | Diff | Splinter Review
9.81 KB, patch
heycam
: review+
Details | Diff | Splinter Review
13.89 KB, patch
heycam
: review+
Details | Diff | Splinter Review
14.98 KB, patch
heycam
: review+
Details | Diff | Splinter Review
3.58 KB, patch
heycam
: review+
Details | Diff | Splinter Review
23.13 KB, patch
heycam
: review+
Details | Diff | Splinter Review
10.44 KB, patch
heycam
: review+
Details | Diff | Splinter Review
9.03 KB, patch
heycam
: review+
Details | Diff | Splinter Review
5.87 KB, patch
heycam
: review+
Details | Diff | Splinter Review
12.03 KB, patch
heycam
: review+
Details | Diff | Splinter Review
8.51 KB, patch
heycam
: review+
Details | Diff | Splinter Review
12.91 KB, patch
heycam
: review+
Details | Diff | Splinter Review
11.53 KB, patch
heycam
: review+
Details | Diff | Splinter Review
8.30 KB, patch
heycam
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
This is the third part of bug 1230040. The basic idea is to track elements needing a restyle as a hashmap on the EffectCompositor and use that to replace a bunch of state flags used by EnsureStyleRuleFor. Then we should be able to move EnsureStyleRuleFor to EffectCompositor without having to add a bunch of flags to EffectSet. It should also mean we track more accurately when an animation style rule is up-to-date and avoid unnecessary restyles.
(Assignee)

Comment 1

2 years ago
Created attachment 8701733 [details] [diff] [review]
part 1 - Add EffectCompositor as a member of nsPresContext

Since we want to track elements needing a restyle on EffectCompositor we need
to scope it to an nsPresContext rather than just making if a collection of
static methods.
(Assignee)

Comment 2

2 years ago
Created attachment 8701737 [details] [diff] [review]
part 2 - Add a hashmap to ElementCompositor to track which (pseudo-) elements need to have their animation style rule updated

We will eventually use this in place of the various state flags stored on
AnimationCollection (e.g. mStyleRuleRefreshTime, mStyleChanging,
mHasPendingAnimationRestyle) as well as to do a more targetted update in
FlushAnimations and AddStyleUpdatesTo.
(Assignee)

Comment 3

2 years ago
Created attachment 8701740 [details] [diff] [review]
part 3 - Move RestyleType to EffectCompositor

This is in preparation for moving RequestRestyle to EffectCompositor (and
because we'll run into compile issues if we don't since AnimationCommon.h
includes too many interdependent definitions).
(Assignee)

Comment 4

2 years ago
Created attachment 8701743 [details] [diff] [review]
part 4 - Add and remove (pseudo-)elements needing an animation style rule update to the EffectCompositor

This patch uses the presence/absence of (pseudo-)elements in the "needs
animation rule update" hashmap on EffectCompositor to detect if a style update
is required. The various flags in AnimationCollection that do a similar job
still remain so that we can remove them one-by-one in subsequent patches in
this series.
(Assignee)

Comment 5

2 years ago
Created attachment 8701883 [details] [diff] [review]
part 5 - Add animation rule refresh time to EffectSet

AnimationCollection keeps a TimeStamp that records the refresh driver time when
the animation style rule was last updated. This is used for two purposes:

1. To determine when the style rule is out of date.

2. For animations that are partially throttled on the main thread, e.g.
   transform animations that affect the scrollable region which we update every
   200ms on the main thread.

In this bug we are removing all the overlapping bits of state used to track if
animations are up-to-date or not and replacing them with the hashmap stored on
the EffectCompositor which tracks which animations are currently in need of an
update. As a result, we would like to remove this style rule refresh time.
However, we will need something for case (2) from above.

This patch adds an animation rule refresh time to the EffectSet purely for the
purposes of partially-throttled animations so that we can later remove the style
rule refresh time from AnimationCollection.
(Assignee)

Comment 6

2 years ago
Created attachment 8701884 [details] [diff] [review]
part 6 - Move call to SetNeedStyleFlush() to EffectCompositor::RequestRestyle

In this patch series we are gradually migrating style rule updating
functionality from AnimationCollection to EffectCompositor. This patch moves the
part of RequestRestyle method from one class to the other.

Note that in both cases we only call SetNeedsStyleFlush if we haven't already
posted an animation restyle. (In the case of AnimationCollection we check this
using the mHasPendingAnimationRestyle flag, and in EffectCompositor case we
simply check if the element is already in the "needs restyle" hashmap. If it is,
either we already have a throttled restyle and have called SetNeedsStyleFlush or
we have a standard restyle and have posted an animation restyle.)

The added check for a null pres context matches the behavior of
AnimationCollection::RequestRestyle which has an equivalent early return at the
beginning of the function.
(Assignee)

Comment 7

2 years ago
Created attachment 8701890 [details] [diff] [review]
part 1 - Add EffectCompositor as a member of nsPresContext

Rebase
(Assignee)

Updated

2 years ago
Attachment #8701733 - Attachment is obsolete: true
(Assignee)

Comment 8

2 years ago
Created attachment 8701893 [details] [diff] [review]
part 2 - Add a hashmap to ElementCompositor to track which (pseudo-) elements need to have their animation style rule updated

Use EnumeratedArray
(Assignee)

Updated

2 years ago
Attachment #8701737 - Attachment is obsolete: true
(Assignee)

Comment 9

2 years ago
Created attachment 8701894 [details] [diff] [review]
part 2 - Add a hashmap to ElementCompositor to track which (pseudo-) elements need to have their animation style rule updated

Fix some alignment
(Assignee)

Updated

2 years ago
Attachment #8701893 - Attachment is obsolete: true
(Assignee)

Comment 10

2 years ago
Created attachment 8701895 [details] [diff] [review]
part 4 - Add and remove (pseudo-)elements needing an animation style rule update to the EffectCompositor

Drop some unnecessary casts
(Assignee)

Updated

2 years ago
Attachment #8701743 - Attachment is obsolete: true
(Assignee)

Comment 11

2 years ago
Created attachment 8701897 [details] [diff] [review]
part 5 - Add animation rule refresh time to EffectSet

Rebase
(Assignee)

Updated

2 years ago
Attachment #8701883 - Attachment is obsolete: true
(Assignee)

Comment 12

2 years ago
Created attachment 8701898 [details] [diff] [review]
part 6 - Move call to SetNeedStyleFlush() to EffectCompositor::RequestRestyle

Rebase
(Assignee)

Updated

2 years ago
Attachment #8701884 - Attachment is obsolete: true
(Assignee)

Comment 13

2 years ago
Created attachment 8701899 [details] [diff] [review]
part 7 - Move call to PostRestyleForAnimation to EffectCompositor

Rebase
(Assignee)

Comment 14

2 years ago
Created attachment 8701903 [details] [diff] [review]
part 8 - Remove AnimationCollection::mStyleRuleRefreshTime

Now that we track whether or not animations are up to date using the hashset in
EffectCompositor, we can remove the mStyleRuleRefreshTime flag that is, as of
part 5 of this patch series, now only used for detecting whether or not
animations are up to date.

In order to preserve the existing behavior of FlushAnimations, however, this
patch temporarily introduces a method to indicate if there are throttled
animations or not.

It might not be obvious that FlushAnimations is only concerned with throttled
animations due to its name. FlushAnimations is simply intended to post
animation restyles for out-of-date animations. Any animations that are *not*
throttled will either be up to date, or we will have already posted an
animation restyle so we only need to consider throttled animations in this case.
(Assignee)

Comment 15

2 years ago
Created attachment 8701904 [details] [diff] [review]
part 9 - Remove AnimationCollection::mStyleChanging

This flag is no longer needed because in bug 1232563 we introduced a more
thorough optimization that detects when the animation is not changing by
comparing the progress value between samples and avoids requesting restyles
when it does not change.
(Assignee)

Comment 16

2 years ago
Created attachment 8701905 [details] [diff] [review]
part 10 - Remove AnimationCollection::mHasPendingAnimationRestyle

This flag is no longer needed since the same information is tracked in the
hashmap stored on EffectSet.
(Assignee)

Comment 17

2 years ago
Created attachment 8701906 [details] [diff] [review]
part 4 - Add and remove (pseudo-)elements needing an animation style rule update to the EffectCompositor

Fix a bug in restyle type calculation
(Assignee)

Updated

2 years ago
Attachment #8701895 - Attachment is obsolete: true
(Assignee)

Comment 18

2 years ago
Created attachment 8701907 [details] [diff] [review]
part 11 - Move the remainder of RequestRestyle from AnimationCollection to EffectCompositor

This also allows us to remove all references to AnimationCollection and the
animation managers from Animation.
(Assignee)

Comment 19

2 years ago
Created attachment 8701908 [details] [diff] [review]
part 12 - Move EnsureStyleRuleFor from AnimationCollection to EffectCompositor
(Assignee)

Comment 20

2 years ago
Created attachment 8701909 [details] [diff] [review]
part 13 - Move FlushAnimations to EffectCompositor
(Assignee)

Comment 21

2 years ago
Created attachment 8701911 [details] [diff] [review]
part 14 - Drop LastStyleUpdateForAllAnimations flag from pres context

nsPresContext contains a mLastStyleUpdateForAllAnimations flag which is simply
used to prevent unnecessarily posting restyles when throttled animations are
already up to date. Since part 13 we now accurately record whether we have
posted a restyle for each throttled animation and only post a restyle if we
have not done so already. As a result, this flag is no longer needed since
calling PostRestyleForThrottledAnimations is effectively a noop when throttled
animations are up-to-date.
(Assignee)

Comment 22

2 years ago
I have 4 more parts to go in this:

* Move AnimationRule to EffectCompositor
* Move AddStyleUpdatesTo to EffectCompositor
  (plus a helper patch to split out a DoUpdateStyleRule method)
* Drop LastUpdateForThrottledAnimations
* Move ClearIsRunningOnCompositor to EffectCompositor

Currently, however, I'm stuck on the first part which fails tests because it drops the check for the absence of a collection. In some rare cases this was making us return a nullptr but without it, we don't. I need to work out why. There may be a problem somewhere in parts 1~14 so I'm not putting them up for review just yet.
(Assignee)

Updated

2 years ago
Blocks: 1235112
(Assignee)

Updated

2 years ago
Blocks: 1237453
(Assignee)

Updated

2 years ago
Blocks: 1237467
(Assignee)

Comment 23

2 years ago
Created attachment 8704890 [details] [diff] [review]
part 4 - Add and remove (pseudo-)elements needing an animation style rule update to the EffectCompositor
(Assignee)

Updated

2 years ago
Attachment #8701906 - Attachment is obsolete: true
(Assignee)

Comment 24

2 years ago
Created attachment 8704891 [details] [diff] [review]
part 5 - Make sure CSSTransition::CancelFromStyle updates the transitions level of the cascade
(Assignee)

Comment 25

2 years ago
Created attachment 8704892 [details] [diff] [review]
part 6 - Add animation rule refresh time to EffectSet
(Assignee)

Updated

2 years ago
Attachment #8701897 - Attachment is obsolete: true
(Assignee)

Comment 26

2 years ago
Created attachment 8704893 [details] [diff] [review]
part 7 - Move call to SetNeedStyleFlush() to EffectCompositor::RequestRestyle
(Assignee)

Updated

2 years ago
Attachment #8701898 - Attachment is obsolete: true
(Assignee)

Comment 27

2 years ago
Created attachment 8704894 [details] [diff] [review]
part 8 - Move call to PostRestyleForAnimation to EffectCompositor
(Assignee)

Updated

2 years ago
Attachment #8701899 - Attachment is obsolete: true
(Assignee)

Comment 28

2 years ago
Created attachment 8704895 [details] [diff] [review]
part 9 - Remove AnimationCollection::mStyleRuleRefreshTime
(Assignee)

Updated

2 years ago
Attachment #8701903 - Attachment is obsolete: true
(Assignee)

Comment 29

2 years ago
Created attachment 8704896 [details] [diff] [review]
part 10 - Remove AnimationCollection::mStyleChanging
(Assignee)

Updated

2 years ago
Attachment #8701904 - Attachment is obsolete: true
(Assignee)

Comment 30

2 years ago
Created attachment 8704897 [details] [diff] [review]
part 11 - Remove AnimationCollection::mHasPendingAnimationRestyle
(Assignee)

Updated

2 years ago
Attachment #8701905 - Attachment is obsolete: true
(Assignee)

Comment 31

2 years ago
Created attachment 8704898 [details] [diff] [review]
part 12 - Move the remainder of RequestRestyle from AnimationCollection to EffectCompositor
(Assignee)

Updated

2 years ago
Attachment #8701907 - Attachment is obsolete: true
(Assignee)

Comment 32

2 years ago
Created attachment 8704899 [details] [diff] [review]
part 13 - Move EnsureStyleRuleFor from AnimationCollection to EffectCompositor
(Assignee)

Updated

2 years ago
Attachment #8701908 - Attachment is obsolete: true
(Assignee)

Comment 33

2 years ago
Created attachment 8704900 [details] [diff] [review]
part 14 - Move FlushAnimations to EffectCompositor
(Assignee)

Updated

2 years ago
Attachment #8701909 - Attachment is obsolete: true
(Assignee)

Comment 34

2 years ago
Created attachment 8704901 [details] [diff] [review]
part 15 - Drop LastStyleUpdateForAllAnimations flag from pres context
(Assignee)

Updated

2 years ago
Attachment #8701911 - Attachment is obsolete: true
(Assignee)

Comment 35

2 years ago
Created attachment 8704902 [details] [diff] [review]
part 16 - Move GetAnimationRule to EffectCompositor
(Assignee)

Comment 36

2 years ago
Created attachment 8704903 [details] [diff] [review]
part 17 - Move AddStyleUpdatesTo to EffectCompositor
(Assignee)

Comment 37

2 years ago
Created attachment 8704904 [details] [diff] [review]
part 18 - Drop RestyleManager::mLastUpdateForThrottledAnimations
(Assignee)

Comment 38

2 years ago
Created attachment 8704905 [details] [diff] [review]
part 19 - Move ClearIsRunningOnCompositor to EffectCompositor
(Assignee)

Updated

2 years ago
Attachment #8701890 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8701894 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8701740 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704890 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704891 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704892 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704893 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704894 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704895 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704896 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704897 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704898 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704899 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704900 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704901 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704902 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704903 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704904 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704905 - Flags: review?(cam)
(Assignee)

Comment 39

2 years ago
Comment on attachment 8704903 [details] [diff] [review]
part 17 - Move AddStyleUpdatesTo to EffectCompositor

Cancelling review for part 17 because it can cause intermittent assertion failures. The issue is that, while iterating over mElementsToRestyle, we call MaybeUpdateCascadeResults which can mutate mElementsToRestyle.

I assumed that MaybeUpdateCascadeResults would not affect mElementsToRestyle in this case since it only ever adds the current element to the hashtable and in this case it is already there however I suppose it can change the associated bool value from false to true.

Example failure: http://archive.mozilla.org/pub/firefox/try-builds/bbirtles@mozilla.com-83939bb72496cac2dc5e34002e080a9e96caa6b7/try-win32-debug/try_win7-ix-debug_test-mochitest-5-bm119-tests1-windows-build2460.txt.gz

I guess I need to call "update cascade results" in a separate loop first.
Attachment #8704903 - Flags: review?(cam)
(Assignee)

Comment 40

2 years ago
Created attachment 8704942 [details] [diff] [review]
part 17 - Move AddStyleUpdatesTo to EffectCompositor
Attachment #8704942 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704903 - Attachment is obsolete: true
Attachment #8701890 - Flags: review?(cam) → review+
Comment on attachment 8701894 [details] [diff] [review]
part 2 - Add a hashmap to ElementCompositor to track which (pseudo-) elements need to have their animation style rule updated

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

::: dom/animation/EffectCompositor.h
@@ +132,5 @@
>    static nsPresContext* GetPresContext(dom::Element* aElement);
>  
>    nsPresContext* mPresContext;
> +
> +  // Elements with a pending restyle. The associated bool value is true

Should this be "Elements with an animation that have a pending restyle" or something like that?  Otherwise it sounds like this would contain unanimated elements too.
Attachment #8701894 - Flags: review?(cam) → review+
Attachment #8701740 - Flags: review?(cam) → review+
Comment on attachment 8704890 [details] [diff] [review]
part 4 - Add and remove (pseudo-)elements needing an animation style rule update to the EffectCompositor

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

::: dom/animation/EffectCompositor.h
@@ +78,5 @@
>      // configuration of the animation.
>      Layer
>    };
>  
> +  void RequestRestyle(dom::Element* aElement,

Might be worth adding a comment describing this method?

@@ +86,5 @@
> +
> +  // Updates the animation rule stored on the EffectSet for the
> +  // specified (pseudo-)element for cascade level |aLevel|.
> +  // If the animation rule is not marked as needing an update,
> +  // no work is done.

Maybe I'm misunderstanding the second sentence here -- it sounds like it is saying if the boolean in the table is false, that we won't recompose the style rule.  But that's not what the method is doing.  Does the comment need clarifying or the method need changing?
Attachment #8704890 - Flags: review?(cam)
Attachment #8704891 - Flags: review?(cam) → review+
(Assignee)

Comment 43

2 years ago
(In reply to Cameron McCormack (:heycam) from comment #42)
> @@ +86,5 @@
> > +
> > +  // Updates the animation rule stored on the EffectSet for the
> > +  // specified (pseudo-)element for cascade level |aLevel|.
> > +  // If the animation rule is not marked as needing an update,
> > +  // no work is done.
> 
> Maybe I'm misunderstanding the second sentence here -- it sounds like it is
> saying if the boolean in the table is false, that we won't recompose the
> style rule.  But that's not what the method is doing.  Does the comment need
> clarifying or the method need changing?

If a (pseudo-)element is in the table, regardless of the value the bool entry, its animation rule needs an update. The bool value indicates if we've posted a restyle to the restyle manager or not. If this method gets called, we update the animation rule regardless of the value of the bool.
(Assignee)

Comment 44

2 years ago
(In reply to Brian Birtles (:birtles) from comment #43)
> If a (pseudo-)element is in the table, regardless of the value the bool
> entry, its animation rule needs an update. The bool value indicates if we've
> posted a restyle to the restyle manager or not. If this method gets called,
> we update the animation rule regardless of the value of the bool.

The bool is just a means of tracking if we've posted a restyle or not so we can avoid posting redundant ones.

So, for an animation that is throttled on the main thread, typically we'll mark the animation rule as needing an update but *not* post a restyle. Then, when we need to bring animation style on the main thread up-to-date, we'll flush animation style and update all out-of-date animations.

For an animation that is running on the main thread we'll mark the animation rule as needing an update and immediately post a restyle for that animation.
Comment on attachment 8704892 [details] [diff] [review]
part 6 - Add animation rule refresh time to EffectSet

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

::: dom/animation/KeyframeEffect.cpp
@@ +1928,5 @@
>      presContext->RefreshDriver()->MostRecentRefresh();
>  
> +  EffectSet* effectSet = EffectSet::GetEffectSet(mTarget, mPseudoType);
> +  MOZ_ASSERT(effectSet, "CanThrottleTransformChanges is expected to be called"
> +                         " on an effect in an effect set");

Nit: misaligned quote.
Attachment #8704892 - Flags: review?(cam) → review+
(In reply to Brian Birtles (:birtles) from comment #44)
> The bool is just a means of tracking if we've posted a restyle or not so we
> can avoid posting redundant ones.

OK, thanks.  I guess the comment above the mElementsToRestyle is clear.
(Assignee)

Comment 47

2 years ago
Comment on attachment 8704904 [details] [diff] [review]
part 18 - Drop RestyleManager::mLastUpdateForThrottledAnimations

Clearing review request for part 18 for now since it is causing warnings sometimes:

WARNING: throttled animations not up to date: '!nsLayoutUtils::AreAsyncAnimationsEnabled() || !mPresContext->EffectCompositor()->HasPendingStyleUpdates()', file c:/moz/src/layout/style/nsTransitionManager.cpp, line 327
Attachment #8704904 - Flags: review?(cam)
Comment on attachment 8704893 [details] [diff] [review]
part 7 - Move call to SetNeedStyleFlush() to EffectCompositor::RequestRestyle

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

s/moves the part of/moves part of the/ in the commit message.
Attachment #8704893 - Flags: review?(cam) → review+
Attachment #8704894 - Flags: review?(cam) → review+
Comment on attachment 8704895 [details] [diff] [review]
part 9 - Remove AnimationCollection::mStyleRuleRefreshTime

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

::: layout/style/AnimationCommon.cpp
@@ +275,5 @@
> +
> +    EffectCompositor::CascadeLevel cascadeLevel =
> +      collection->IsForAnimations() ?
> +      EffectCompositor::CascadeLevel::Animations :
> +      EffectCompositor::CascadeLevel::Transitions;

This pattern of computing a CascadeLevel from IsForAnimations() is very common -- can we have a function on AnimationCollection to return the right CascadeLevel?
Attachment #8704895 - Flags: review?(cam) → review+
(Assignee)

Comment 50

2 years ago
(In reply to Cameron McCormack (:heycam) from comment #49)
> Comment on attachment 8704895 [details] [diff] [review]
> part 9 - Remove AnimationCollection::mStyleRuleRefreshTime
> 
> Review of attachment 8704895 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/AnimationCommon.cpp
> @@ +275,5 @@
> > +
> > +    EffectCompositor::CascadeLevel cascadeLevel =
> > +      collection->IsForAnimations() ?
> > +      EffectCompositor::CascadeLevel::Animations :
> > +      EffectCompositor::CascadeLevel::Transitions;
> 
> This pattern of computing a CascadeLevel from IsForAnimations() is very
> common -- can we have a function on AnimationCollection to return the right
> CascadeLevel?

I think all cases of computing a CascadeLevel from the collection are removed by the end of this bug, and the remaining two instances that compute a CascadeLevel from the manager are removed in bug 1235112.
Comment on attachment 8704896 [details] [diff] [review]
part 10 - Remove AnimationCollection::mStyleChanging

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

::: layout/style/nsAnimationManager.cpp
@@ +537,5 @@
>        animation->AsCSSAnimation()->QueueEvents();
>      }
>    }
>    collection->mAnimations.SwapElements(newAnimations);
> +  collection->RequestRestyle(EffectCompositor::RestyleType::Layer);

Can you explain why this is needed?
Attachment #8704896 - Flags: review?(cam) → review+
(Assignee)

Comment 52

2 years ago
(In reply to Cameron McCormack (:heycam) from comment #51)
> Comment on attachment 8704896 [details] [diff] [review]
> part 10 - Remove AnimationCollection::mStyleChanging
> 
> Review of attachment 8704896 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsAnimationManager.cpp
> @@ +537,5 @@
> >        animation->AsCSSAnimation()->QueueEvents();
> >      }
> >    }
> >    collection->mAnimations.SwapElements(newAnimations);
> > +  collection->RequestRestyle(EffectCompositor::RestyleType::Layer);
> 
> Can you explain why this is needed?

I think it's not needed (in one of the later patches I added a comment "Is this necessary?" and I was going to file a follow-up bug to go through and drop any unneeded RequestRestyle calls from the managers but I should just drop it here and see if anything breaks.) Let me drop that line and see what happens.
Comment on attachment 8704898 [details] [diff] [review]
part 12 - Move the remainder of RequestRestyle from AnimationCollection to EffectCompositor

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

::: dom/animation/EffectCompositor.cpp
@@ +520,5 @@
> +    // Update both transitions and animations. We could detect *which* levels
> +    // actually changed and only update them, but that's probably unnecessary.
> +    CascadeLevel levelsToUpdate[] = { CascadeLevel::Animations,
> +                                      CascadeLevel::Transitions };
> +    for (CascadeLevel level : levelsToUpdate) {

We now have a std::initializer_list polyfill in mfbt/InitializerList.h, so you could write this without the separate array, if you want it be slightly more compact:

  for (auto level : { CascadeLevel::Animations, CascadeLevel::Transitions }) {
Attachment #8704898 - Flags: review?(cam) → review+
Attachment #8704899 - Flags: review?(cam) → review+
Comment on attachment 8704900 [details] [diff] [review]
part 14 - Move FlushAnimations to EffectCompositor

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

::: dom/animation/EffectCompositor.cpp
@@ +193,5 @@
> +{
> +  size_t i = 0;
> +  for (auto& elementSet : mElementsToRestyle) {
> +    MOZ_ASSERT(i < kCascadeLevelCount);
> +    CascadeLevel cascadeLevel = CascadeLevel(i++);

Nit: While still not ideal, I wonder if this would look neater using i as the loop variable:

  for (size_t i = 0; i < kCascadeLevelCount; i++) {
    CascadeLevel cascadeLevel = CascadeLevel(i);
    auto& elementSet = mElementsToRestyle[cascadeLevel];

You could rely on the assertion within EnumeratedArray<>::operator[] then.

::: dom/animation/EffectCompositor.h
@@ +94,5 @@
>  
> +  // Posts an animation restyle for any elements whose animation style rule
> +  // is out of date but for which an animation restyle has not yet been
> +  // posted because updates on the main thread are throttled.
> +  void PostRestyleForThrottledAnimations();

+1 on the rename.
Attachment #8704900 - Flags: review?(cam) → review+
Attachment #8704901 - Flags: review?(cam) → review+
Attachment #8704902 - Flags: review?(cam) → review+
Attachment #8704942 - Flags: review?(cam) → review+
Comment on attachment 8704905 [details] [diff] [review]
part 19 - Move ClearIsRunningOnCompositor to EffectCompositor

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

::: dom/animation/EffectCompositor.h
@@ +124,5 @@
>    static nsTArray<RefPtr<dom::Animation>>
>    GetAnimationsForCompositor(const nsIFrame* aFrame,
>                               nsCSSProperty aProperty);
>  
> +  static void ClearIsRunningOnCompositor(const nsIFrame *aFrame,

Bump "*" next to the type while moving this.

::: layout/base/nsDisplayList.cpp
@@ +534,5 @@
>    // Do this check only during layer construction; during updating the
>    // caller is required to check it appropriately.
>    if (aItem && !aItem->CanUseAsyncAnimations(aBuilder)) {
> +    // EffectCompositor needs to know that we refused to run this animation
> +    // asynchronously so that they will not throttle the main thread

s/they/it/
Attachment #8704905 - Flags: review?(cam) → review+
(Assignee)

Comment 56

2 years ago
Created attachment 8705499 [details] [diff] [review]
part 4 - Add and remove (pseudo-)elements needing an animation style rule update to the EffectCompositor

Address review feedback from comment 42
Attachment #8705499 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704890 - Attachment is obsolete: true
(Assignee)

Comment 57

2 years ago
Comment on attachment 8704897 [details] [diff] [review]
part 11 - Remove AnimationCollection::mHasPendingAnimationRestyle

Hi Cameron, thanks for all the reviews!! Was there anything missing from part 11 I can help with? Thanks!
Flags: needinfo?(cam)
(Assignee)

Comment 58

2 years ago
(In reply to Cameron McCormack (:heycam) from comment #54)
> Comment on attachment 8704900 [details] [diff] [review]
> part 14 - Move FlushAnimations to EffectCompositor
> 
> Review of attachment 8704900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/EffectCompositor.cpp
> @@ +193,5 @@
> > +{
> > +  size_t i = 0;
> > +  for (auto& elementSet : mElementsToRestyle) {
> > +    MOZ_ASSERT(i < kCascadeLevelCount);
> > +    CascadeLevel cascadeLevel = CascadeLevel(i++);
> 
> Nit: While still not ideal, I wonder if this would look neater using i as
> the loop variable:

Yeah, that's better. I should do that throughout this file.
Comment on attachment 8704897 [details] [diff] [review]
part 11 - Remove AnimationCollection::mHasPendingAnimationRestyle

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

No I just skipped over it!
Attachment #8704897 - Flags: review?(cam) → review+
(Assignee)

Comment 60

2 years ago
Created attachment 8705508 [details] [diff] [review]
part 18 - Drop RestyleManager::mLastUpdateForThrottledAnimations

RestyleManager currently has a piece of state for tracking if throttled
animations are up-to-date or not. Actually, it's not so much about throttled
animations but really and outstanding changes to animation styles (which
is typically expected to be due to throttling animations but there are
other cases that invalidate the animation style rule that we should be
considering here).

We now have that same information stored in the EffectCompositor so we can
remove the redundant state from RestyleManager. Furthermore, the state stored
in EffectCompositor is more accurate since it captures the case when animation
style needs to be updated twice within a tick, or when nothing needs to be
updated within a tick.

This patch, therefore, introduces EffectCompositor::HasPendingStyleUpdates in
place of setting RestyleManager::mLastUpdateForThrottledAnimations.

nsTransitionManager also uses mLastUpdateForThrottledAnimations to warn if we
have not processed throttled animations. We can't use HasPendingStyleUpdates
here however, since it will return true in the case where we have triggered new
transitions in the process of restyling. However, any new transitions will
trigger "standard" (i.e. not throttled) restyles so we introduce another
method, HasThrottledStyleUpdates, that returns true only if we have outstanding
throttled updates and use this for the warning inside nsTransitionManager.
Attachment #8705508 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Attachment #8704904 - Attachment is obsolete: true
Attachment #8705499 - Flags: review?(cam) → review+
Comment on attachment 8705508 [details] [diff] [review]
part 18 - Drop RestyleManager::mLastUpdateForThrottledAnimations

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

s/but really and/but really about/ in the commit message?
Attachment #8705508 - Flags: review?(cam) → review+
Flags: needinfo?(cam)
(Assignee)

Comment 63

2 years ago
This appears to give us a ~6% performance improvement on tab animation tests
Blocks: 1219236
Depends on: 1240646

Updated

2 years ago
Depends on: 1245601

Updated

2 years ago
Blocks: 1254264
You need to log in before you can comment on or make changes to this bug.