Closed Bug 1374882 Opened 7 years ago Closed 7 years ago

Precompute active duration and end time

Categories

(Core :: DOM: Animation, enhancement)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(6 files)

Bug 1374882 - Make TimingParamsFromOptionsUnion member function.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1374882 - Add a TimingParams ctor for CSS animations/transitions.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1374882 - Add a TimingParams ctor on the compositor.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1374882 - Encapsulate TimingParams's member variables.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1374882 - Precompute active duration and end time.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1374882 - Insert 'Timing' word into the names for setter/getter for timing function.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
On https://greensock.com/js/speed.html with 750 dots and web animations, TimingParams::EndTime() is pretty high running time. See invert call stack in a profile [1].

We can precompute end time and active time when updating TimingParams. We don't need to compute on every EndTime() call.

[1] http://bit.ly/2son6py
Got a new profile with a binary on the try in comment 1.

http://bit.ly/2soBfDf

EndTime() has gone in the invert call stack list.
Attachment #8879793 - Flags: review?(bbirtles)
Attachment #8879794 - Flags: review?(bbirtles)
Comment on attachment 8879793 [details]
Bug 1374882 - Make TimingParamsFromOptionsUnion member function.

https://reviewboard.mozilla.org/r/151172/#review156032

::: commit-message-0a5e5:3
(Diff revision 1)
> +Bug 1374882 - Move TimingParamsFromCSSParams into TimingParams as a static member function. r?birltes
> +
> +In this bug, we make TimingParam() construct private to prevent TimingParam

constructor

s/TimingParam/TimingParams/g
Attachment #8879793 - Flags: review?(bbirtles) → review+
Comment on attachment 8879794 [details]
Bug 1374882 - Add a TimingParams ctor for CSS animations/transitions.

https://reviewboard.mozilla.org/r/151174/#review156050
Attachment #8879794 - Flags: review?(bbirtles) → review+
Comment on attachment 8879795 [details]
Bug 1374882 - Add a TimingParams ctor on the compositor.

https://reviewboard.mozilla.org/r/151176/#review156052
Attachment #8879795 - Flags: review?(bbirtles) → review+
Comment on attachment 8879796 [details]
Bug 1374882 - Encapsulate TimingParams's member variables.

https://reviewboard.mozilla.org/r/151178/#review156042

::: dom/animation/AnimationEffectTiming.cpp:24
(Diff revision 1)
> -static inline void
> -PostSpecifiedTimingUpdated(KeyframeEffect* aEffect)
> +inline void
> +AnimationEffectTiming::PostSpecifiedTimingUpdated()
>  {
> -  if (aEffect) {
> -    aEffect->NotifySpecifiedTimingUpdated();
> +  mTiming.Update();
> +  if (mEffect) {
> +    mEffect->NotifySpecifiedTimingUpdated();
>    }

Since this now does more than just posting a notification, we should call it UpdateSpecifiedTiming instead.

However, I feel like we should just go ahead and add all the GetDelay/SetDelay methods to TimingParams and make it completely encapsulated so we never have an issue with forgetting to call Update.

If we make the getters / setters inline it shouldn't introduce any performance cost. The only cost it would introduce is if we do batch updates, but it doesn't look like we do that yet (and if we start to, we should just set the new properties by creating a new object, passing the new values to the ctor, and then implementing the assignment/move operator).

Adding the getters / setters would also remove the awkwardness between being able to call ActiveDuration() or simply fetch mActiveDuration (i.e. the assertions in ActiveDuration / EndTime don't really give us any guarantees.)

If we do that, then we perhaps don't need some of the patches in this series. (Tying TimingParams to Animations on layers seems like a bit of an odd coupling to introduce.) It would also mean we could keep the default constructor as public.

What do you think?

::: dom/animation/TimingParams.h:132
(Diff revision 1)
> +  // The duration of the active interval calculated by duration and iteration
> +  // count.

Let's leave this comment with the ActiveDuration() method. It feels a bit odd here in the middle of all the member declarations and it's probably callers of ActiveDuration() who really want to read this.

::: dom/animation/TimingParams.h:139
(Diff revision 1)
> -    // If either the iteration duration or iteration count is zero,
> -    // Web Animations says that the active duration is zero. This is to
> -    // ensure that the result is defined when the other argument is Infinity.
> -    static const StickyTimeDuration zeroDuration;
> -    if (!mDuration || *mDuration == zeroDuration || mIterations == 0.0) {
> +    MOZ_ASSERT(((!mDuration ||
> +                 *mDuration == StickyTimeDuration() ||
> +                 mIterations == 0.0) &&
> +                mActiveDuration == StickyTimeDuration()) ||
> +               mActiveDuration == mDuration->MultDouble(mIterations));

Let's add a message to the assertion, "Cached value for active duration should be up to date"

Also, this is a pretty large assertion. Would it make sense to factor out a static method that takes "Maybe<StickyTimeDuration>& aDuration, double aIterations" and calculates the active duration and then use that both here in this assertion and in Update()?

e.g.

  MOZ_ASSERT(CalcActiveDuration(mDuration, mIterations) == mActiveDuration,
             "Cached value for active duration should be up to date");

::: dom/animation/TimingParams.h:149
(Diff revision 1)
> -    return std::max(mDelay + ActiveDuration() + mEndDelay,
> -                    StickyTimeDuration());
> +    MOZ_ASSERT(mEndTime == std::max(mDelay + mActiveDuration + mEndDelay,
> +                                    StickyTimeDuration()));

Likewise, "Cached value of end time should be up to date"

::: dom/animation/TimingParams.h:149
(Diff revision 1)
> -    return mDuration->MultDouble(mIterations);
>    }
>  
>    StickyTimeDuration EndTime() const
>    {
> -    return std::max(mDelay + ActiveDuration() + mEndDelay,
> +    MOZ_ASSERT(mEndTime == std::max(mDelay + mActiveDuration + mEndDelay,

Replace mActiveDuration with ActiveDuration() so we also check if mActiveDuration is up-to-date?
(In reply to Brian Birtles (:birtles) from comment #10)
> Comment on attachment 8879796 [details]
> Bug 1374882 - Precompute active duration and end time.
> 
> https://reviewboard.mozilla.org/r/151178/#review156042
> 
> ::: dom/animation/AnimationEffectTiming.cpp:24
> (Diff revision 1)
> > -static inline void
> > -PostSpecifiedTimingUpdated(KeyframeEffect* aEffect)
> > +inline void
> > +AnimationEffectTiming::PostSpecifiedTimingUpdated()
> >  {
> > -  if (aEffect) {
> > -    aEffect->NotifySpecifiedTimingUpdated();
> > +  mTiming.Update();
> > +  if (mEffect) {
> > +    mEffect->NotifySpecifiedTimingUpdated();
> >    }
> 
> Since this now does more than just posting a notification, we should call it
> UpdateSpecifiedTiming instead.
> 
> However, I feel like we should just go ahead and add all the
> GetDelay/SetDelay methods to TimingParams and make it completely
> encapsulated so we never have an issue with forgetting to call Update.
> 
> If we make the getters / setters inline it shouldn't introduce any
> performance cost. The only cost it would introduce is if we do batch
> updates, but it doesn't look like we do that yet (and if we start to, we
> should just set the new properties by creating a new object, passing the new
> values to the ctor, and then implementing the assignment/move operator).
> 
> Adding the getters / setters would also remove the awkwardness between being
> able to call ActiveDuration() or simply fetch mActiveDuration (i.e. the
> assertions in ActiveDuration / EndTime don't really give us any guarantees.)
> 
> If we do that, then we perhaps don't need some of the patches in this
> series. (Tying TimingParams to Animations on layers seems like a bit of an
> odd coupling to introduce.) It would also mean we could keep the default
> constructor as public.
> 
> What do you think?

This sounded reasonable to me, but it turns out that some setter vary its argument.
For duration case we will have;

 void SetDuration(const TimeDuration& aDuration);  // For for layers::Animation::duration()
 void SetDuration(Maybe<StickyTimeDuration>&& aDuration);  For AnimationEffectTiming::SetDuration()

Also we need to call Update() in each setters respectively, it doesn't look nice.

So, I've decided to add ctors with variables for gfx/layers case and css animations/transitions, and add a single setter for each property (i.e. a SetDuration() for AnimationEffectTiming::SetDuration) and call Update() inside some setters.

Here is a try what I have in mind.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a58844d4d3cbf1f8c2d787c14e0924441f7750ff
Comment on attachment 8879796 [details]
Bug 1374882 - Encapsulate TimingParams's member variables.

https://reviewboard.mozilla.org/r/151178/#review156510

::: dom/animation/TimingParams.h:155
(Diff revision 2)
>      return !(*this == aOther);
>    }
> +
> +  void SetDuration(Maybe<StickyTimeDuration>&& aDuration)
> +  {
> +    mDuration = aDuration;

Does this need to be Move(aDuration) ?

::: dom/animation/TimingParams.h:195
(Diff revision 2)
> +  void SetFunction(Maybe<ComputedTimingFunction>&& aFunction)
> +  {
> +    mFunction = aFunction;
> +  }
> +  const Maybe<ComputedTimingFunction>& Function() const { return mFunction; }

I think we should rename this to s/Function/TimingFunction/. That can be a separate patch, however.

::: dom/animation/TimingParams.h:197
(Diff revision 2)
> +  }
> +  dom::FillMode Fill() const { return mFill; }
> +
> +  void SetFunction(Maybe<ComputedTimingFunction>&& aFunction)
> +  {
> +    mFunction = aFunction;

Does this need to be Move(aFunction) ?
Attachment #8879796 - Flags: review?(bbirtles) → review+
Comment on attachment 8880169 [details]
Bug 1374882 - Precompute active duration and end time.

https://reviewboard.mozilla.org/r/151532/#review156484

::: dom/animation/TimingParams.h:147
(Diff revision 1)
> +  // Return the duration of the active interval calculated by duration and
> +  // iteration count.
> +  StickyTimeDuration ActiveDuration() const
> +  {
> +    MOZ_ASSERT(CalcActiveDuration(mDuration, mIterations) == mActiveDuration,
> +               "Cached value of active duration should up to date");

should be up to date

::: dom/animation/TimingParams.h:155
(Diff revision 1)
>  
>    StickyTimeDuration EndTime() const
>    {
> -    return std::max(mDelay + ActiveDuration() + mEndDelay,
> -                    StickyTimeDuration());
> +    MOZ_ASSERT(mEndTime == std::max(mDelay + ActiveDuration() + mEndDelay,
> +                                    StickyTimeDuration()),
> +               "Cached value of end time should up to date");

should be up to date

::: dom/animation/TimingParams.h:218
(Diff revision 1)
>      mFunction = aFunction;
>    }
>    const Maybe<ComputedTimingFunction>& Function() const { return mFunction; }
>  
>  private:
> +  // Update mActiveDuration and mEndTime with mDuration, mDelay and mEndDelay.

And mIterations, right?

But perhaps we don't need to list all the members the calculation depends on here? (And especially not if we decide to move this function definition into the header file.)

Perhaps it would be enough to say:

"Update the calculated mActiveDuration and mEndTime members."

(Again, if we move the definition of the function to the header, even that is probably unnecessary.)

::: dom/animation/TimingParams.cpp:190
(Diff revision 1)
> +void
> +TimingParams::Update()
> +{
> +  mActiveDuration = CalcActiveDuration(mDuration, mIterations);
> +
> +  mEndTime = std::max(mDelay + mActiveDuration + mEndDelay,
> +                      StickyTimeDuration());
> +}
> +

Perhaps we should put this in the header file so it can possibly be inlined? (Although I hear that compilers are smart enough these days that they don't need that anymore?)

::: dom/animation/TimingParams.cpp:202
(Diff revision 1)
>    return mDuration == aOther.mDuration &&
>           mDelay == aOther.mDelay &&
>           mIterations == aOther.mIterations &&
>           mIterationStart == aOther.mIterationStart &&
>           mDirection == aOther.mDirection &&
>           mFill == aOther.mFill &&
>           mFunction == aOther.mFunction;

Perhaps we should add a comment here saying:

"We don't compare mActiveDuration and mEndTime because they are calculated from the other timing parameters."
Attachment #8880169 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #18)

> ::: dom/animation/TimingParams.h:218
> (Diff revision 1)
> >      mFunction = aFunction;
> >    }
> >    const Maybe<ComputedTimingFunction>& Function() const { return mFunction; }
> >  
> >  private:
> > +  // Update mActiveDuration and mEndTime with mDuration, mDelay and mEndDelay.
> 
> And mIterations, right?
> 
> But perhaps we don't need to list all the members the calculation depends on
> here? (And especially not if we decide to move this function definition into
> the header file.)
> 
> Perhaps it would be enough to say:
> 
> "Update the calculated mActiveDuration and mEndTime members."
> 
> (Again, if we move the definition of the function to the header, even that
> is probably unnecessary.)
> 
> ::: dom/animation/TimingParams.cpp:190
> (Diff revision 1)
> > +void
> > +TimingParams::Update()
> > +{
> > +  mActiveDuration = CalcActiveDuration(mDuration, mIterations);
> > +
> > +  mEndTime = std::max(mDelay + mActiveDuration + mEndDelay,
> > +                      StickyTimeDuration());
> > +}
> > +
> 
> Perhaps we should put this in the header file so it can possibly be inlined?
> (Although I hear that compilers are smart enough these days that they don't
> need that anymore?)

Neat idea!
Comment on attachment 8880198 [details]
Bug 1374882 - Insert 'Timing' word into the names for setter/getter for timing function.

https://reviewboard.mozilla.org/r/151564/#review156524
Attachment #8880198 - Flags: review?(bbirtles) → review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe0fe81ad2c5
Make TimingParamsFromOptionsUnion member function. r=birtles
https://hg.mozilla.org/integration/autoland/rev/5d961658e35f
Add a TimingParams ctor for CSS animations/transitions. r=birtles
https://hg.mozilla.org/integration/autoland/rev/d9ee6dc097db
Add a TimingParams ctor on the compositor. r=birtles
https://hg.mozilla.org/integration/autoland/rev/317f3e92be06
Encapsulate TimingParams's member variables. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ebe04b9961fd
Precompute active duration and end time. r=birtles
https://hg.mozilla.org/integration/autoland/rev/c6d7ed956142
Insert 'Timing' word into the names for setter/getter for timing function. r=birtles
Blocks: 1372642
Assignee: nobody → hikezoe
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: