Closed Bug 1410826 Opened 5 years ago Closed 5 years ago
Move up the Is
Primary Frame() in ns IFrame::Has Animation Of Transform
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|.
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|.
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
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.
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9b6d988e3ac1 Check IsPrimaryFrame() first in nsIFrame::HasAnimationOfTransform. r=mats
You need to log in before you can comment on or make changes to this bug.