Closed Bug 1410826 Opened 5 years ago Closed 5 years ago

Move up the IsPrimaryFrame() in nsIFrame::HasAnimationOfTransform


(Core :: Layout, enhancement, P3)




Tracking Status
firefox58 --- fixed


(Reporter: ywu, Assigned: ywu)


(Keywords: perf)


(1 file, 2 obsolete files)

In nsIFrame::HasAnimationOfTransform, we do

return mContent &&
  nsLayoutUtils::HasAnimationOfProperty(effects, eCSSProperty_transform) &&
  IsFrameOfType(eSupportsCSSTransforms) &&

We should move IsPrimaryFrame() to the place after we check |mContent| because if IsPrimaryFrame() is false, we don't need to query |HasAnimationOfProperty| and |IsFrameOfType|.
Assignee: nobody → ywu
Attachment #8920993 - Flags: review?(mats)
Comment on attachment 8920993 [details] [diff] [review]

I suspect IsPrimaryFrame() is true in most cases here.
mContent is pretty much always true. (We can remove the mContent
check here, GetEffectSet already does that.)
HasAnimationOfProperty(effects, eCSSProperty_transform) is probably rare though.
I suspect IsFrameOfType(eSupportsCSSTransforms) is also true for most frames,
but since it's virtual it might be a win to move IsPrimaryFrame() before it.
EffectSet::GetEffectSet looks rather slow though.

IsPrimaryFrame() is fast nowadays though, so we can move that first
as an early return before the GetEffectSet call.
Attachment #8920993 - Flags: review?(mats) → review-
It looks like we have a more serious performance issue with all these
methods that takes a "EffectSet* aEffectSet = nullptr" param though.
Looking at BuildDisplayListForStackingContext for example,
we (unconditionally) get the EffectSet for the frame, then call
IsTransformed(effectSet) etc with it:,2409,2422,2439-2440,2501
If effectSet is null (quite common) then there will be a number of
(slow) GetEffectSet calls in all those methods.
This seems like a suboptimal design.
I am working on Bug 1381153 in which I try to remove calling GetEffectSet.
That's why I notice |nsIFrame::HasAnimationOfTransform| and think that 
calling |IsPrimaryFrame()| is really cheap why should we not place it
before |HasAnimationOfProperty| and |IsFrameOfType|.
Priority: -- → P3

In this patch, I remove mContent and move |IsPrimaryFrame()| to first place. For calling and passing |GetEffectSet| is inefficient is Bug 1381153. In fact, I am working on Bug 1381153 which try to remove passing |EffectSet| and that's why I notice |nsIFrame::HasAnimationOfTransform| and think that calling
 |IsPrimaryFrame()| is really cheap why should we not place it
before |HasAnimationOfProperty| and |IsFrameOfType|.

Attachment #8920993 - Attachment is obsolete: true
Attachment #8921324 - Flags: review?(mats)
Keywords: perf
Comment on attachment 8921324 [details] [diff] [review]

r=mats, assuming the local var |effects| is removed in bug 1381153.

(Don't forget to update the commit message with bug number and reviewer.)
Attachment #8921324 - Flags: review?(mats) → review+
try looks ok.
Keywords: checkin-needed
Pushed by
Check IsPrimaryFrame() first in nsIFrame::HasAnimationOfTransform. r=mats
Keywords: checkin-needed
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.