Closed Bug 1410826 Opened 5 years ago Closed 5 years ago

Move up the IsPrimaryFrame() in nsIFrame::HasAnimationOfTransform

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: ywu, Assigned: ywu)

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

In nsIFrame::HasAnimationOfTransform, we do

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


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]
Check-IsPrimaryFrame-first-in-nsIFrame-HasAnimationO.patch

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:
http://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/layout/generic/nsFrame.cpp#2397,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
mats:

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

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e35aa11c2f350cd2d77af9f6e8dacc1bfd2606b&selectedJob=139074238
Attachment #8920993 - Attachment is obsolete: true
Attachment #8921324 - Flags: review?(mats)
Keywords: perf
Comment on attachment 8921324 [details] [diff] [review]
0001-Check-IsPrimaryFrame-first-in-nsIFrame-HasAnimationO.patch

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 ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b6d988e3ac1
Check IsPrimaryFrame() first in nsIFrame::HasAnimationOfTransform. r=mats
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9b6d988e3ac1
Status: NEW → RESOLVED
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.