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.

Rebase

Use EnumeratedArray

Fix some alignment

Drop some unnecessary casts

Rebase

Rebase

Rebase

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.

Fix a bug in restyle type calculation

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.

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.

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.

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?

(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.

(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

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.

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?

(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?

(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 }) {

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.

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/

Address review feedback from comment 42

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!

(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!

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.

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?

https://hg.mozilla.org/integration/mozilla-inbound/rev/68108d1f2d46 https://hg.mozilla.org/integration/mozilla-inbound/rev/4fa5cf94bb6d https://hg.mozilla.org/integration/mozilla-inbound/rev/92654d4ad2eb https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb0835ccdc0 https://hg.mozilla.org/integration/mozilla-inbound/rev/c642726a9f8e https://hg.mozilla.org/integration/mozilla-inbound/rev/191626c92b35 https://hg.mozilla.org/integration/mozilla-inbound/rev/6511f2832185 https://hg.mozilla.org/integration/mozilla-inbound/rev/e3aa66edbd85 https://hg.mozilla.org/integration/mozilla-inbound/rev/35725757afce https://hg.mozilla.org/integration/mozilla-inbound/rev/f6fcc3f908c0 https://hg.mozilla.org/integration/mozilla-inbound/rev/697caeac54f5 https://hg.mozilla.org/integration/mozilla-inbound/rev/04ea9a6a29a5 https://hg.mozilla.org/integration/mozilla-inbound/rev/69236594a8ef https://hg.mozilla.org/integration/mozilla-inbound/rev/ddfdec87e448 https://hg.mozilla.org/integration/mozilla-inbound/rev/d3abdd12484d https://hg.mozilla.org/integration/mozilla-inbound/rev/9ffa5c570547 https://hg.mozilla.org/integration/mozilla-inbound/rev/50cc47215375 https://hg.mozilla.org/integration/mozilla-inbound/rev/b3831779e4d1 https://hg.mozilla.org/integration/mozilla-inbound/rev/cf9a5f6f14db

This appears to give us a ~6% performance improvement on tab animation tests

https://hg.mozilla.org/mozilla-central/rev/68108d1f2d46 https://hg.mozilla.org/mozilla-central/rev/4fa5cf94bb6d https://hg.mozilla.org/mozilla-central/rev/92654d4ad2eb https://hg.mozilla.org/mozilla-central/rev/2cb0835ccdc0 https://hg.mozilla.org/mozilla-central/rev/c642726a9f8e https://hg.mozilla.org/mozilla-central/rev/191626c92b35 https://hg.mozilla.org/mozilla-central/rev/6511f2832185 https://hg.mozilla.org/mozilla-central/rev/e3aa66edbd85 https://hg.mozilla.org/mozilla-central/rev/35725757afce https://hg.mozilla.org/mozilla-central/rev/f6fcc3f908c0 https://hg.mozilla.org/mozilla-central/rev/697caeac54f5 https://hg.mozilla.org/mozilla-central/rev/04ea9a6a29a5 https://hg.mozilla.org/mozilla-central/rev/69236594a8ef https://hg.mozilla.org/mozilla-central/rev/ddfdec87e448 https://hg.mozilla.org/mozilla-central/rev/d3abdd12484d https://hg.mozilla.org/mozilla-central/rev/9ffa5c570547 https://hg.mozilla.org/mozilla-central/rev/50cc47215375 https://hg.mozilla.org/mozilla-central/rev/b3831779e4d1 https://hg.mozilla.org/mozilla-central/rev/cf9a5f6f14db