1231471 - Re-use ComputedTimingFunction objects representing cubic-bezier keyword values

Status: NEW

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

As described in bug 1216842 comment 13 we are currently storing the same set of cubic bezier points *and* cache of sample values for every single keyframe that uses 'ease' for example (the initial value for animation-timing-function). We should really have a common definition of each of these keyword values and simply point to it every time we need to.

Comment 1

[Boris Chiou [:boris]]

My idea about this bug:
If we want to re-use the ComputedTimingFunction objects for these keyword values, we may need to store it as a pointer (e.g. RefPtr) to ComputedTimingFunction, which means we have to change the member type from Maybe<ComputedTimingFunction> to RefPtr<ComputedTimingFunction>. And then, we can declare some static RefPtr<ComputedTimingFunction>s for re-using.

So the steps look like (if we only re-use them in |Keyframe|):
1. In |Keyframe|, use RefPtr<ComputedTimingFunction>, instead of Maybe<ComputedTimingFunction>
2. Declare some static RefPtr<ComputedTimingFunction>s for those keywords. e.g.

struct ComputedTimingFunction {
  static RefPtr<ComputedTimingFunction> sKeywordTimingFunction[5]; // linear, ease, ease-in, ease-out, ease-in-out
}
And initialize these function somewhere.

3. Declare a static function to get those ComputedTimingFunctions. e.g.

struct ComputedTimingFunction {
  staitc RefPtr<ComputedTimingFunction> GetKeywordComputedTimingFunction() {...}
}
This function can return sKeywordTimingFunction or a null value. If it is null, we have to create it by ourself (just like we new and init() it before).

Comment 2

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris]  from comment #1)
> So the steps look like (if we only re-use them in |Keyframe|):
> 1. In |Keyframe|, use RefPtr<ComputedTimingFunction>, instead of
> Maybe<ComputedTimingFunction>

Also in |AnimationPropertySegment|, so keyframe-level timing function can re-use them.

BTW, we almost use Maybe<ComputedTimingFunction> everywhere, e.g. ParseEasing(), so we also need to change the return type of those function used by keyframe-level timing function.

Comment 3

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris]  from comment #2)
> (In reply to Boris Chiou [:boris]  from comment #1)
> > So the steps look like (if we only re-use them in |Keyframe|):
> > 1. In |Keyframe|, use RefPtr<ComputedTimingFunction>, instead of
> > Maybe<ComputedTimingFunction>
> 
> Also in |AnimationPropertySegment|, so keyframe-level timing function can
> re-use them.
> 
> BTW, we almost use Maybe<ComputedTimingFunction> everywhere, e.g.
> ParseEasing(), so we also need to change the return type of those function
> used by keyframe-level timing function.

Or another simpler idea is to re-use nsSMILKeySpline object in ComputedTimingFunction if the critical part is the cached sample values in nsSMILKeySpline.

Comment 4

[Boris Chiou [:boris]]

Oops, I forgot this. If we want to use RefPtr<ComputedTimingFunction>, we have to declare its destructor as private/protected, and if the destructor is not public, we cannot put it into Maybe<> or declare it as a normal object (if we don't use "friend"). This is another problem. So I think if we want to re-use it, we have to define a creator function and always use it to get the object, which means the step 3 in comment 1 should be revised, and it's better to use RefPtr<ComputedTimingFunction>/ComputedTimingFunction* everywhere.

Comment 5

[Boris Chiou [:boris]]

Hi Hiro,

How do you think about my comments (comment 1 and comment 4)? Does it make sense to replace Maybe<ComputedTimingFunction> with RefPtr<ComputedTimingFunction> in all places?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris]  from comment #1)
> My idea about this bug:
> If we want to re-use the ComputedTimingFunction objects for these keyword
> values, we may need to store it as a pointer (e.g. RefPtr) to
> ComputedTimingFunction, which means we have to change the member type from
> Maybe<ComputedTimingFunction> to RefPtr<ComputedTimingFunction>. And then,
> we can declare some static RefPtr<ComputedTimingFunction>s for re-using.
> 
> So the steps look like (if we only re-use them in |Keyframe|):
> 1. In |Keyframe|, use RefPtr<ComputedTimingFunction>, instead of
> Maybe<ComputedTimingFunction>
> 2. Declare some static RefPtr<ComputedTimingFunction>s for those keywords.
> e.g.
> 
> struct ComputedTimingFunction {
>   static RefPtr<ComputedTimingFunction> sKeywordTimingFunction[5]; //
> linear, ease, ease-in, ease-out, ease-in-out
> }
> And initialize these function somewhere.

I am not sure we really need RefPtr there.  I just wonder nsSMILKeySpline for those keywords can be constexpr.  If we can do it, we don't need to initialize them at run-time.  And we can also make nsSMILKeySpline mTimingFunction Maybe<>?  At lease in case of step functions it is not needed at all.  

> 3. Declare a static function to get those ComputedTimingFunctions. e.g.
> 
> struct ComputedTimingFunction {
>   staitc RefPtr<ComputedTimingFunction> GetKeywordComputedTimingFunction()
> {...}
> }
> This function can return sKeywordTimingFunction or a null value. If it is
> null, we have to create it by ourself (just like we new and init() it
> before).

Though I am not sure where this function will be used, I think ParseEasing() returns sKeywordTimingFunction directly.

We can do the same thing on compositor if we pass those keywords to compositor.  Or we can modify ComputedTimingFunction::Init when known point sets are passed to ComputedTimingFunction::Init.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Boris Chiou [:boris]  from comment #1)
> 2. Declare some static RefPtr<ComputedTimingFunction>s for those keywords.

Drive-by note -- if we really do end up needing a "static RefPtr" here, you should actually use "static StaticRefPtr", which is defined in xpcom/base/StaticPtr.h

StaticRefPtr is a bit more efficient than RefPtr for startup-time; see comment at the top of
 http://mxr.mozilla.org/mozilla-central/source/xpcom/base/StaticPtr.h

Comment 8

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> I am not sure we really need RefPtr there.  I just wonder nsSMILKeySpline
> for those keywords can be constexpr.

Thanks for your feedback, Hiro. If we don't have to use RefPtr, it'd be better. :)

If we want to declare nsSMILKeySpline as constexpr, we need a constructor satisfying the requirements of constexpr constructor. I checked nsSMILKeySpline.h, and if we want to initialize mSampleValues[kSplineTableSize] in the constexpr constructor, we may need to use |for| statement. However, C++11 doesn't allow it. (C++14 constexpr functions may use local variables and loops, but I think we should follow rules of C++11.) Or do we have any trick to do it, e.g. expand the array initialization by typing 0~11? If it's not a good idea, we might need to find a workaround to do this.

Thanks.

Comment 9

[Boris Chiou [:boris]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Drive-by note -- if we really do end up needing a "static RefPtr" here, you
> should actually use "static StaticRefPtr", which is defined in
> xpcom/base/StaticPtr.h
> 
> StaticRefPtr is a bit more efficient than RefPtr for startup-time; see
> comment at the top of
>  http://mxr.mozilla.org/mozilla-central/source/xpcom/base/StaticPtr.h

Thanks for your reminder, Daniel. I note it down!

Comment 10

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris]  from comment #8)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > I am not sure we really need RefPtr there.  I just wonder nsSMILKeySpline
> > for those keywords can be constexpr.
> 
> Thanks for your feedback, Hiro. If we don't have to use RefPtr, it'd be
> better. :)
> 
> If we want to declare nsSMILKeySpline as constexpr, we need a constructor
> satisfying the requirements of constexpr constructor. I checked
> nsSMILKeySpline.h, and if we want to initialize
> mSampleValues[kSplineTableSize] in the constexpr constructor, we may need to
> use |for| statement. However, C++11 doesn't allow it. (C++14 constexpr
> functions may use local variables and loops, but I think we should follow
> rules of C++11.) Or do we have any trick to do it, e.g. expand the array
> initialization by typing 0~11? If it's not a good idea, we might need to
> find a workaround to do this.
> 
> Thanks.

BTW, we cannot use constexpr in MSVC 2013 and before, so I think we shouldn't use it to optimize nsSMILKeySpline.

Comment 11

[Boris Chiou [:boris]]

BTW, I'm not sure using MOZ_CONSTEXPR is a good way to optimize this because constexpr is not supported in MSVC 2013 and before.

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Boris Chiou [:boris]  from comment #11)
> BTW, I'm not sure using MOZ_CONSTEXPR is a good way to optimize this because
> constexpr is not supported in MSVC 2013 and before.

That's going to change very soon. We already build using MSVC 2015 and I expect we'll drop support for building on MSVC 2013 within 1~2 release cycles (at which point someone will probably do a big s/MOZ_CONSTEXPR/constexpr/ on the tree).

Comment 13

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #12)
> That's going to change very soon. We already build using MSVC 2015 and I
> expect we'll drop support for building on MSVC 2013 within 1~2 release
> cycles (at which point someone will probably do a big
> s/MOZ_CONSTEXPR/constexpr/ on the tree).

OK. Thanks, Brian. Looks like the first thing I should do in this bug is: try to make nsSMILKeySpline constexpr-constructed.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris]  from comment #8)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > I am not sure we really need RefPtr there.  I just wonder nsSMILKeySpline
> > for those keywords can be constexpr.
> 
> Thanks for your feedback, Hiro. If we don't have to use RefPtr, it'd be
> better. :)
> 
> If we want to declare nsSMILKeySpline as constexpr, we need a constructor
> satisfying the requirements of constexpr constructor. I checked
> nsSMILKeySpline.h, and if we want to initialize
> mSampleValues[kSplineTableSize] in the constexpr constructor, we may need to
> use |for| statement. However, C++11 doesn't allow it. (C++14 constexpr
> functions may use local variables and loops, but I think we should follow
> rules of C++11.) Or do we have any trick to do it, e.g. expand the array
> initialization by typing 0~11? If it's not a good idea, we might need to
> find a workaround to do this.

As you may already notice, mSampleValues can be calculated outside the constructor.  And nsSMILKeySpline::CalcBezier can be also a constexpr function.  As a result we can pass the sample values to the constructor.

I can confirm the following code works fine on Linux now.

  static MOZ_CONSTEXPR_VAR uint8_t kSplineTableSize = 11;                       
  template <size_t N, uint8_t ... i>                                            
  constexpr nsSMILKeySpline(double aX1, double aY1,                             
                            double aX2, double aY2,                             
                            const double (&aSampleValues)[N])                   
    : mX1(aX1)                                                                  
    , mY1(aY1)                                                                  
    , mX2(aX2)                                                                  
    , mY2(aY2)                                                                  
    , mSampleValues{ aSampleValues[i] ... }                                     
  {                                                                             
    static_assert(N == kSplineTableSize, "");                                   
  }