Closed Bug 1232577 Opened 9 years ago Closed 9 years ago

Move EnsureStyleRule to EffectCompositor

Categories

(Core :: DOM: Animation, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox46 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

(Blocks 1 open bug)

Details

Attachments

(19 files, 21 obsolete files)

part 3 - Move RestyleType to EffectCompositor
11.12 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 1 - Add EffectCompositor as a member of nsPresContext
10.12 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 2 - Add a hashmap to ElementCompositor to track which (pseudo-) elements need to have their animation style rule updated
6.30 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 5 - Make sure CSSTransition::CancelFromStyle updates the transitions level of the cascade
1.86 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 6 - Add animation rule refresh time to EffectSet
9.55 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 7 - Move call to SetNeedStyleFlush() to EffectCompositor::RequestRestyle
3.08 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 8 - Move call to PostRestyleForAnimation to EffectCompositor
9.81 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 9 - Remove AnimationCollection::mStyleRuleRefreshTime
13.89 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 10 - Remove AnimationCollection::mStyleChanging
14.98 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 11 - Remove AnimationCollection::mHasPendingAnimationRestyle
3.58 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 12 - Move the remainder of RequestRestyle from AnimationCollection to EffectCompositor
23.13 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 13 - Move EnsureStyleRuleFor from AnimationCollection to EffectCompositor
10.44 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 14 - Move FlushAnimations to EffectCompositor
9.03 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 15 - Drop LastStyleUpdateForAllAnimations flag from pres context
5.87 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 16 - Move GetAnimationRule to EffectCompositor
12.03 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 19 - Move ClearIsRunningOnCompositor to EffectCompositor
8.51 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 17 - Move AddStyleUpdatesTo to EffectCompositor
12.91 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 4 - Add and remove (pseudo-)elements needing an animation style rule update to the EffectCompositor
11.53 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 18 - Drop RestyleManager::mLastUpdateForThrottledAnimations
8.30 KB, patch
heycam
: review+
Details | Diff | Splinter Review 
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.
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.
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.
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).
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.
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.
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.
Attachment #8701733 - Attachment is obsolete: true
Attachment #8701737 - Attachment is obsolete: true
Attachment #8701893 - Attachment is obsolete: true
Attachment #8701743 - Attachment is obsolete: true
Attachment #8701883 - Attachment is obsolete: true
Attachment #8701884 - Attachment is obsolete: true
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.
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.
This flag is no longer needed since the same information is tracked in the
hashmap stored on EffectSet.
Attachment #8701895 - Attachment is obsolete: true
This also allows us to remove all references to AnimationCollection and the
animation managers from Animation.
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.
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.
Attachment #8701906 - Attachment is obsolete: true
Attachment #8701897 - Attachment is obsolete: true
Attachment #8701898 - Attachment is obsolete: true
Attachment #8701899 - Attachment is obsolete: true
Attachment #8701903 - Attachment is obsolete: true
Attachment #8701904 - Attachment is obsolete: true
Attachment #8701905 - Attachment is obsolete: true
Attachment #8701907 - Attachment is obsolete: true
Attachment #8701908 - Attachment is obsolete: true
Attachment #8701909 - Attachment is obsolete: true
Attachment #8701911 - Attachment is obsolete: true
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)
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+
(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.
(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.
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+
(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+
(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+
Attachment #8704890 - Attachment is obsolete: true
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)
(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+
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)
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)
This appears to give us a ~6% performance improvement on tab animation tests
Blocks: 1219236
Depends on: 1240646
Depends on: 1245601
Blocks: 1254264
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: