1381153 - Calling GetEffectSet during display list building is not very cache friendly

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P2)

Description

[Markus Stange [:mstange]]

A cache line wastage profile of one frame of displaylist building on https://jrmuizel.github.io/implementation-tests/dl-test.html shows that GetEffectSet is responsible for loading a lot of unused memory into the processor's L3 cache. This takes up memory bandwidth that could be used for other things during display list building.

The profiles are here:
 - read_bytes: https://perfht.ml/2tc6PG4
 - wasted_bytes: https://perfht.ml/2tcjDws

See bug 1379694 comment 4 about how to interpret these profiles.

The profiles show that GetEffectSet causes us to read 650KB from memory of which 479KB are never accessed. This is because we read in 64 byte chunks (cache lines) and the data that we need within each of those chunks is a lot less than that.

The elements in the testcase don't have animations on them.

So EffectSet::GetEffectSet(const nsIFrame* aFrame), which is called from nsIFrame::BuildDisplayListForChild, basically only does this: It gets the frame's nsIContent, checks whether content->IsElement(), and then checks whether that element MayHaveAnimations().

The cache miss here occurs when EffectCompositor::GetAnimationElementAndPseudoForFrame calls content->IsElement(), because that's the first time we access data inside the element. So the only bytes we use from the element are the ones used by IsElement() and MayHaveAnimations().

It would be nice if we could store information on the frame that lets us skip calling GetContent() for nsIFrames without animations.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I think we can do that. One thing I am curious is that moving the information into nsIFrame means that the cost is moved into frame construction right? If so, though I don't quite understand about retain display lists, even once it happens, moving it will get some performance win? (I think it is).

Another thing I am concerned is that MayHaveAnimations() is also used for optimizations in stylo through nsINode [1]. So If we moved it into nsIFrame, we need to manage the flag in both of nsINode and nsIFrame. (Of course we use the flag in nsINode in other places regardless of stylo)  It's bit awkward. 

One more thing, I just noticed that we can check MayHaveAnimations() in GetAnimationElementAndPseudoForFrame, here [2]. I am not sure it will reduce the cache misses.

[1] https://hg.mozilla.org/mozilla-central/file/bd1b6a4e5d8e/servo/components/style/gecko/wrapper.rs#l1090
[2] https://hg.mozilla.org/mozilla-central/file/bd1b6a4e5d8e/dom/animation/EffectCompositor.cpp#l693

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Also I realized that we set NS_FRAME_MAY_BE_TRANSFORMED in nsFrame::Init() if the frame has transform animations. So, as for transform animations, we don't need to re-check it again I think.

[1] http://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#655

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> Also I realized that we set NS_FRAME_MAY_BE_TRANSFORMED in nsFrame::Init()
> if the frame has transform animations. So, as for transform animations, we
> don't need to re-check it again I think.

This is wrong. We don't clear NS_FRAME_MAY_BE_TRANSFORMED flag, so when the transform animation is finished, the flag still persists.

After some more thought, before this change [1], we could early return from nsIFram::IsTransform() if the frame does not have NS_FRAME_MAY_BE_TRANSFORMED.  So in the test case in comment 0, the cost of GetEffectSet did not matter at all.

CCing CJ.

[1] https://hg.mozilla.org/mozilla-central/rev/cf4e37d17683e0cbe134debc1d3685fb3f84e3d1

Comment 4

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

Adding Matt since it seems like if we could put this opacity state on the frame (as bug 1205475 tried to do, except that the frame state bits were full) we could possibly fix the issue he is facing with removed opacity animations sometimes not triggering a display list update.

Comment 5

[Astley Chen (inactive)]

Ya-Chieh, as we discussed, please help to progress the bug and raise your questions if any.

Comment 6

[Ya-Chieh Wu (inactive)]

Attachment: wip

hey cj,

I just read through the comments above and have a first wip. But I am not sure if I am on the right path. Could you give a feedback? And since mState in nsIFrame is full, what is your suggestion for storing this two flags?  

many thanks.

Comment 7

[Ya-Chieh Wu (inactive)]

Attachment: wip

a bit update.

Comment 8

[u459114]

Comment on attachment 8905435 [details] [diff] [review]
wip

Review of attachment 8905435 [details] [diff] [review]:
-----------------------------------------------------------------

This change is reasonable for me. The only concern is you increase the size of nsIFrame by introducing two more data members which enlarge the size of a frame (and that's the reason why I chose to put them into effect set instead).
Is that possible to put those two flags in nsIFrame::mState instead? Is there any flag in nsFrameStateBits.h not being used?

Comment 9

[u459114]

Please refer to this commit message
https://hg.mozilla.org/mozilla-central/rev/d7b25abd4866

It explains why I did not put these two variables in nsIFrame

Comment 10

[Markus Stange [:mstange]]

The two bools can fit into the remaining 10 bit gap here: http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/layout/generic/nsIFrame.h#4192

(This gap is shrinking rapidly.)

Comment 11

[Ya-Chieh Wu (inactive)]

Attachment: Part 1. Move MAY_HAVE_OPACITY_ANIM/ MAY_HAVE_TRANSFOMR_ANIM information in nsIFrame

working on part 2 now.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

UpdateEffectSet() should be renamed to an appropriate name. Also I am wondering we don't need to call UpdateEffectSet() in UpdateTargetRegistration() any more.

Comment 13

[Ya-Chieh Wu (inactive)]

Attachment: Part 1. Move MAY_HAVE_OPACITY_ANIM/ MAY_HAVE_TRANSFOMR_ANIM information in nsIFrame.

Comment 14

[Ya-Chieh Wu (inactive)]

Attachment: Part 2. Remove unused EffectSet.

Comment 15

[Ya-Chieh Wu (inactive)]

there are try fails. not sure where it goes wrong...

Comment 16

[u459114]

(In reply to Ya-Chieh Wu from comment #15)
> there are try fails. not sure where it goes wrong...
link?

Comment 17

[Ya-Chieh Wu (inactive)]

a lot of animation tests fail.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=192375eef63bf2f6d1c51e7ce1aa1bb4ea8fa128&selectedJob=129443493
I probably can't use |GetAnimationFrame()| there or the place I put |UpdateAnimationFlagsInFrame()| is wrong.

Comment 18

[u459114]

Comment on attachment 8905783 [details] [diff] [review]
Part 1. Move MAY_HAVE_OPACITY_ANIM/ MAY_HAVE_TRANSFOMR_ANIM information in nsIFrame.

Review of attachment 8905783 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +472,3 @@
>                                        nsCSSPropertyID aProperty)
>  {
> +  if (!MayHaveAnimationOfProperty(aFrame, aProperty)) {

not compilable. The first argument of MayHaveAnimationOfProperty has to be const.
MayHaveAnimationOfProperty(const nsIFrame* aFrame, nsCSSPropertyID aProperty)

Each patch should be able to compiled separately

Comment 19

[u459114]

Comment on attachment 8905783 [details] [diff] [review]
Part 1. Move MAY_HAVE_OPACITY_ANIM/ MAY_HAVE_TRANSFOMR_ANIM information in nsIFrame.

Review of attachment 8905783 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/KeyframeEffectReadOnly.cpp
@@ +1885,2 @@
>  {
> +  nsIFrame* aFrame = GetAnimationFrame();

Rename aFrame as frame or targetFrame

@@ +1888,1 @@
>      return;

For those failure reftests, such as stacking-context-opacity-1-animation.html, I found the frames of mTarget->mElement was not created when UpdateAnimationFlagsInFrame() is called.
That means KeyframeEffectReadOnly::UpdateAnimationFlagsInFrame() is used before frame construction. So we always early return here.

Comment 20

[u459114]

Attachment: Carry mHaveXXXXAnim on Element

The relation between EffectSet and nsIFrame is 1-to-N(primary + ::before + ::after)

The relation between EffectSet and Element is 1-to-1, so carry those two flags in Element is more suitable.

But we need to find a way to reduce the size of element.(see the ASSERT_ELEMENT_SIZE that I comment out in Element.cpp)

Comment 21

[u459114]

Ya-Chieh,
Please refer to the patch I attached.
And:
Move Element::mHaveXXXAnim to nsINode::nsSlots so that we can keep the size of Element still 
nsINode::mSlots is created on demand, so it's default a nullptr.

So, you can change the logic of code to
class Element:
  void SetMayHaveOpacityAnimation() {
    Slots()->mMayHaveOpacityAnim;
  }
  bool MayHaveOpacityAnimation() const {
    // Is mSlots is nullptr, that means SetMayHaveOpacityAnimation was
    // not called, we can just return false.
    return HasSlots() ? Slots()->mHaveOpacityAnimation : false;
  }
};

Comment 22

[u459114]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #20)
> Created attachment 8906127 [details] [diff] [review]
> Carry mHaveXXXXAnim on Element
> 
> The relation between EffectSet and nsIFrame is 1-to-N(primary + ::before +
> ::after)
This can be wrong. Please ignore it. The reason that we can not carry these two flags in frame in written in comment 19

Comment 23

[Hiroyuki Ikezoe (:hiro)]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #19)
> For those failure reftests, such as
> stacking-context-opacity-1-animation.html, I found the frames of
> mTarget->mElement was not created when UpdateAnimationFlagsInFrame() is
> called.
> That means KeyframeEffectReadOnly::UpdateAnimationFlagsInFrame() is used
> before frame construction. So we always early return here.

I thought moving the flags to frame means we set the flags when we initialize the frame, isn't it?

Comment 24

[u459114]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> (In reply to C.J. Ku[:cjku](UTC+8) from comment #19)
> > For those failure reftests, such as
> > stacking-context-opacity-1-animation.html, I found the frames of
> > mTarget->mElement was not created when UpdateAnimationFlagsInFrame() is
> > called.
> > That means KeyframeEffectReadOnly::UpdateAnimationFlagsInFrame() is used
> > before frame construction. So we always early return here.
> 
> I thought moving the flags to frame means we set the flags when we
> initialize the frame, isn't it?
Both KeyframeEffectReadOnly and Frame are created after an Element was created, that's what we can guarantee.
What we can no guarantee is whether a frame is created ahead of KeyframeEffectReadOnly.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #24)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> > (In reply to C.J. Ku[:cjku](UTC+8) from comment #19)
> > > For those failure reftests, such as
> > > stacking-context-opacity-1-animation.html, I found the frames of
> > > mTarget->mElement was not created when UpdateAnimationFlagsInFrame() is
> > > called.
> > > That means KeyframeEffectReadOnly::UpdateAnimationFlagsInFrame() is used
> > > before frame construction. So we always early return here.
> > 
> > I thought moving the flags to frame means we set the flags when we
> > initialize the frame, isn't it?
> Both KeyframeEffectReadOnly and Frame are created after an Element was
> created, that's what we can guarantee.

There is a case that we have KeyframeEffectReadOnly but it's not associated to any elements.

Comment 26

[u459114]

yc, we need to move MayHaveAnimation/MayHaveOpcaityAnimation/MyHaveTransformAnimation

Comment 27

[u459114]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> There is a case that we have KeyframeEffectReadOnly but it's not associated
> to any elements.
If a KeyframeEffectReadOnly does not bind with an element, then there should no frame associates with this KeyframeEffectReadOnly too, right?

Comment 28

[u459114]

I think we may move GetBoolFlag(ElementHasAnimations) into Slots() as well, if it's false most of time.

Comment 29

[u459114]

The last try link, wait and see
https://treeherder.mozilla.org/#/jobs?repo=try&revision=137135044c07abaeffe5162d335256e87005ffca

Comment 30

[u459114]

Attachment: Carry mMayHaveOpacityAnim and mMayHaveTransformAnim in nsINode::mSlots.

Comment 31

[Ya-Chieh Wu (inactive)]

Attachment: Part I: Cache MayHaveTransformAnim in nsIFrame

this patch is a wip.

I am thinking that if we could duplicate |mMayHaveAnimations| in nsIFrame and keep mMayHaveOpacityAnim and mMayHaveTransformAnim in EffectSet. So we could still use |mMayHaveAnimations| to do early return.

And for resolving the issue of |GetEffectSet|[1] which cause the increase of memory, we probably can do *later getter* for them? For example, we pass a *EffectSet pointer* into |HasOpacity| and |IsTransformed|. And we only get the real EffectSet object when we need to get |EffectSet| in either |HasOpacity| and |IsTransformed|. In this way, we won't need to call |GetEffectSet| at the first but we could still share the |EffectSet object| if needed.



[1]http://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#3106

Comment 32

[Markus Stange [:mstange]]

Having an early return that only reads data from the nsIFrame sounds good. Accessing information from the nsIContent during display list building should be avoided, because we end up using only very little information from the 64 bytes of consecutive data that get read during such an access. In bug 1381157 comment 6 I got some good results from eliminating all accesses to the nsIContent for the common case.

Comment 33

[Ya-Chieh Wu (inactive)]

I have tried to cache the mMayHaveAnimation in |nsFrame::Init()| and |UpdateEffectSet()| but it doesn't really work. I still will miss some cases when |UpdateEffectSet()| can't find a right nsFrame which I originally thought the nsFrame would be constructed later so I would get to cache the mMayHaveAnimation in |nsFrame::Init()| but it doesn't work.

I think, as cj's patch, we could at least put the two flags into Element which we at least won't need to load |EffectSet| but we still need to look up GetContent(). Any thoughts?

Comment 34

[Markus Stange [:mstange]]

It would be nice to find out why the strategy with nsFrame::Init() doesn't work. It should be quite easy to track down with rr: In all places where MayHaveAnimation is queried, assert that the frame flag and the effect state match. Catch the the failing assertion in rr. Set a watchpoint on the effect state flag, and a breakpoint on nsFrame::Init for that frame, reverse-continue, and see where the watchpoint is hit. (And make sure it's hit before nsFrame::Init, which should be the case when the assertion is failing.)

Comment 35

[Hiroyuki Ikezoe (:hiro)]

(In reply to Ya-Chieh Wu from comment #33)
> I have tried to cache the mMayHaveAnimation in |nsFrame::Init()| and
> |UpdateEffectSet()| but it doesn't really work. I still will miss some cases
> when |UpdateEffectSet()| can't find a right nsFrame

How did you try to find the nsFrame?  If you used KeyframeReadOnly::GetAnimationFrame(), it uses Element::GetPrimaryFrame(), so if you called UpdateEffectSet() before the frame is associated with an element as the primary frame, it will fail.

Comment 36

[Ya-Chieh Wu (inactive)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #35)
> How did you try to find the nsFrame?   

I actually called KeyframeReadOnly::GetAnimationFrame() in UpdateEffectSet().

Comment 37

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Part-I-Cache-MayHaveOpacityAnimation-and-MayHaveTran.patch

this wip patch should be working. will have a followup to clean up unused EffectSet.

Comment 38

[Ya-Chieh Wu (inactive)]

btw, hiro remindered me that my old patch's naming is confusing by using mMayHaveAnimation where in fact, I was using it for MayHaveOpacityAnimation and MayHaveTransformAnimation so in this patch, I put back this two flags.

Comment 39

[Ya-Chieh Wu (inactive)]

try looks ok
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3784585d991decb15745ef7cf918711b9a7e836

Comment 40

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Bug-1381153-Part-1-Cache-MayHaveOpacityAnimation-and.patch

Comment 41

[Ya-Chieh Wu (inactive)]

Attachment: Part 2 : Use the flags we cache in nsIFrame

Comment 42

[Ya-Chieh Wu (inactive)]

try looks ok:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=168f64314c1f209b9ee3d009f7dd9a3d183d0913

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8911667 [details] [diff] [review]
0001-Bug-1381153-Part-1-Cache-MayHaveOpacityAnimation-and.patch

Review of attachment 8911667 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +646,5 @@
>        IncApproximateVisibleCount();
>      }
>    }
>    const nsStyleDisplay *disp = StyleDisplay();
> +  EffectSet* effectSet = EffectSet::GetEffectSet(this);

I am wondering this patch solves the issue that Markus mentioned in comment 0.
I think we need to use EffectSet::GetEffectSet(const Element*, CSSPseudoElementType) instead. As I told on IRC, we can get CSSPseudoElementType from style context.

Comment 44

[Ya-Chieh Wu (inactive)]

(In reply to Hiroyuki Ikezoe (:hiro) PTO on 28 Sep. from comment #43)
> ::: layout/generic/nsFrame.cpp
> @@ +646,5 @@
> >        IncApproximateVisibleCount();
> >      }
> >    }
> >    const nsStyleDisplay *disp = StyleDisplay();
> > +  EffectSet* effectSet = EffectSet::GetEffectSet(this);
> 
> I am wondering this patch solves the issue that Markus mentioned in comment
> 0.
> I think we need to use EffectSet::GetEffectSet(const Element*,
> CSSPseudoElementType) instead. As I told on IRC, we can get
> CSSPseudoElementType from style context.

we initialize the two flags to false and for example in |nsLayoutUtils::HasAnimationOfProperty| as below we do early return so if the frame doesn't have an animation, the EffectSet won't be loaded. I think this saves the size which was mentioned in Comment 1. 

--- a/layout/base/nsLayoutUtils.cpp
+++ b/layout/base/nsLayoutUtils.cpp

> -nsLayoutUtils::HasAnimationOfProperty(EffectSet* aEffectSet, 
> +nsLayoutUtils::HasAnimationOfProperty(const nsIFrame* aFrame,
>                                        nsCSSPropertyID aProperty)
>  {
> -  if (!aEffectSet || !MayHaveAnimationOfProperty(aEffectSet, aProperty)) {
> +  if (!MayHaveAnimationOfProperty(aFrame, aProperty)) {
>      return false;
>    }

I push try to see if there were any cases where the flags should be "true" but we missed to cache it and did early return. But as Comment 42, it looks fine which means that using |EffectSet* effectSet = EffectSet::GetEffectSet(this);| should be ok. If this convince you? thanks!

Comment 45

[Astley Chen (inactive)]

Markus, could you help to review the patches here? Thanks.

Comment 46

[Markus Stange [:mstange]]

Sorry that I still haven't gotten to this. I will try to review it tomorrow.

In the meantime, Hiro, can you comment on whether your concerns have been addressed?

Comment 47

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8911667 [details] [diff] [review]
0001-Bug-1381153-Part-1-Cache-MayHaveOpacityAnimation-and.patch

Review of attachment 8911667 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/KeyframeEffectReadOnly.cpp
@@ +1892,5 @@
>                                           mTarget->mPseudoType);
>    if (!effectSet) {
>      return;
>    }
> +  

Nit: unnecessary spaces.

::: layout/generic/nsFrame.cpp
@@ +645,5 @@
>        // Assume all frames in popups are visible.
>        IncApproximateVisibleCount();
>      }
>    }
>    const nsStyleDisplay *disp = StyleDisplay();

Nit: This should be after the following if block, i.e. should be close to if (disp->HasTransform(this) block.

@@ +646,5 @@
>        IncApproximateVisibleCount();
>      }
>    }
>    const nsStyleDisplay *disp = StyleDisplay();
> +  EffectSet* effectSet = EffectSet::GetEffectSet(this);

I am still wondering why we don't use EffectSet::GetEffectSet(Element*, CSSPseudoElementType) instead of EffectSet::GetEffectSet(nsIFrame*).　It saves the cost for looking-up Element and CSSPseudoElementType. 

Also animations run only on Element, so we can check mContent->IsElement() here and should check Element::MayHaveAnimations() as well.  So I think this part will be;

  if (mContent->IsElement() && mContent->AsElement()->MayHaveAnimations()) {
    EffectSet* effectSet =
      EfffectSet::GetEffectSet(mContent->AsElement(),
                               StyleContenxt()->GetPseudoType());
    if (effectSet) {
       ....
    }
  }

Comment 48

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #47)
> I am still wondering why we don't use EffectSet::GetEffectSet(Element*,
> CSSPseudoElementType) instead of EffectSet::GetEffectSet(nsIFrame*).　It
> saves the cost for looking-up Element and CSSPseudoElementType. 
> 
> Also animations run only on Element, so we can check mContent->IsElement()
> here and should check Element::MayHaveAnimations() as well.  So I think this
> part will be;

Oh, MayHaveAnimations() actually belongs to nsINode. So we can directly call mContent->MayHaveAnimations().

Comment 49

[Hiroyuki Ikezoe (:hiro)]

I am also wondering whether we can drop EffectSet::mMayHaveOpacityAnim and EffectSet::mMayHaveTransformAnim with these patches?

Comment 50

[Ya-Chieh Wu (inactive)]

 (In reply to Hiroyuki Ikezoe (:hiro) from comment #47)

> I am still wondering why we don't use EffectSet::GetEffectSet(Element*,
> CSSPseudoElementType) instead of EffectSet::GetEffectSet(nsIFrame*).　It
> saves the cost for looking-up Element and CSSPseudoElementType. 

Oh, I see. I get your point. I will make this change. thanks.

Comment 51

[Ya-Chieh Wu (inactive)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #49)
> I am also wondering whether we can drop EffectSet::mMayHaveOpacityAnim and
> EffectSet::mMayHaveTransformAnim with these patches?

I had tried to remove *EffectSet::mMayHaveOpacityAnim* and *EffectSet::mMayHaveTransformAnim*.
However, I found that when we want to update the status in |KeyframeEffectReadOnly::UpdateEffectSet|,
we might not have any frame which is associated with the element. I mean there is no guarantee that
the element has been associated with a frame  when we want to update the status. So I think that we
can still keep *EffectSet::mMayHaveOpacityAnim* and *EffectSet::mMayHaveTransformAnim* for us to update
the status and we transfer them into nsIFrame when we bind element to frame in nsIFrame:Init().

Comment 52

[Hiroyuki Ikezoe (:hiro)]

What I meant is that, if we drop the flag from EffectSet, we have to do HasAnimaitonOfProperty checks in nsIFrame::Init() instead of in UpdateEffectSet().  I think it should work.  Also note that KeyframeEffectReadOnly::UpdateEffectSet() is called whenever any animation properties change even if the target element does not have the primary frame (a typical case is that the element is display:none subtree).  Whereas nsIFrame::Init() is called on reframing. So it seems to me that nsIFrame::Init() is a cheaper place for the checks.

Comment 53

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #52)
> What I meant is that, if we drop the flag from EffectSet, we have to do
> HasAnimaitonOfProperty checks in nsIFrame::Init() instead of in
> UpdateEffectSet().  I think it should work.  Also note that
> KeyframeEffectReadOnly::UpdateEffectSet() is called whenever any animation
> properties change even if the target element does not have the primary frame
> (a typical case is that the element is display:none subtree).  Whereas
> nsIFrame::Init() is called on reframing. So it seems to me that
> nsIFrame::Init() is a cheaper place for the checks.

That's said, I don't have any evidences for this, so we should check it.

Comment 54

[Ya-Chieh Wu (inactive)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #53)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #52)
> > What I meant is that, if we drop the flag from EffectSet, we have to do
> > HasAnimaitonOfProperty checks in nsIFrame::Init() instead of in
> > UpdateEffectSet().  I think it should work.  Also note that
> > KeyframeEffectReadOnly::UpdateEffectSet() is called whenever any animation
> > properties change even if the target element does not have the primary frame
> > (a typical case is that the element is display:none subtree).  Whereas
> > nsIFrame::Init() is called on reframing. So it seems to me that
> > nsIFrame::Init() is a cheaper place for the checks.
> 
yes, we can do that but if I remember correctly, we were trying to avoid looking up 
*nsLayoutUtils::HasAnimationOfProperty* because it takes long.
So in bug 1205475, we add this two flgs, *EffectSet::mMayHaveOpacityAnim* and *EffectSet::mMayHaveTransformAnim*, 
for early return.

Comment 55

[Ya-Chieh Wu (inactive)]

I have rebased the patches to the new codebase. And as the changing of Bug 1406211, I have to add one more patch on these two old patches.

Comment 56

[Ya-Chieh Wu (inactive)]

Attachment: Part 1: Cache MayHaveOpacityAnimation and MayHaveTransformAnimation in nsIFrame.

update to new codebase

Comment 57

[Ya-Chieh Wu (inactive)]

Attachment: 0002-Bug-1381153-Part-2-look-up-MayHaveOpacityAnimation-a.patch

Comment 58

[Ya-Chieh Wu (inactive)]

Comment on attachment 8920499 [details] [diff] [review]
Part 1: Cache MayHaveOpacityAnimation and MayHaveTransformAnimation in nsIFrame.

Hiro:

I have updated the patch to not use |EffectSet::GetEffectSet(nsIFrame*)| directly. And the reason that I don't use |EffectSet::GetEffectSet(Element*,
 CSSPseudoElementType)| directly is because if a frame's |CSSPseudoElementType| is not before, after, notpseduo, it will hit assert.

Also, after the changing of Bug 1406211, I have this fail. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28c90dbf7cb104ef1a748e2f3b1e4dcaec2775fc
In this push, I add a check in |nsLayoutUtils::HasAnimationOfProperty| to make sure that we only use the cache data when the frame is a primary frame or we shouldn't use the cache values. It's unclear for me that why I break this test. Do you think it exists the timing issue for the test of |ib-split-sibling-opacity.html| or I just wrong in something? How do you think?

Comment 59

[Ya-Chieh Wu (inactive)]

Another try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7fd5b379497a83f0816ba2d53738ef49112dab3

and this time tc-R-e10s(R4) in Linux-x64-opt pass.

Comment 60

[Hiroyuki Ikezoe (:hiro)]

(In reply to Ya-Chieh Wu from comment #58)
> Comment on attachment 8920499 [details] [diff] [review]
> Part 1: Cache MayHaveOpacityAnimation and MayHaveTransformAnimation in
> nsIFrame.
> 
> Hiro:
> 
> I have updated the patch to not use |EffectSet::GetEffectSet(nsIFrame*)|
> directly. And the reason that I don't use |EffectSet::GetEffectSet(Element*,
>  CSSPseudoElementType)| directly is because if a frame's
> |CSSPseudoElementType| is not before, after, notpseduo, it will hit assert.

The assertion tells us what the right thing to do.  Animations on pseudo elements other than ::before or ::after never run, so we don't need to check animation existence for such pseudo elements, i.e. we don't need to call GetEffectSet() at all.

> Also, after the changing of Bug 1406211, I have this fail. 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=28c90dbf7cb104ef1a748e2f3b1e4dcaec2775fc
> In this push, I add a check in |nsLayoutUtils::HasAnimationOfProperty| to
> make sure that we only use the cache data when the frame is a primary frame
> or we shouldn't use the cache values. It's unclear for me that why I break
> this test. Do you think it exists the timing issue for the test of
> |ib-split-sibling-opacity.html| or I just wrong in something? How do you
> think?

I will check this failure later.

Comment 61

[Hiroyuki Ikezoe (:hiro)]

(In reply to Ya-Chieh Wu from comment #58)
> Also, after the changing of Bug 1406211, I have this fail. 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=28c90dbf7cb104ef1a748e2f3b1e4dcaec2775fc
> In this push, I add a check in |nsLayoutUtils::HasAnimationOfProperty| to
> make sure that we only use the cache data when the frame is a primary frame
> or we shouldn't use the cache values. It's unclear for me that why I break
> this test. Do you think it exists the timing issue for the test of
> |ib-split-sibling-opacity.html| or I just wrong in something? How do you
> think?

I guess you should set the new flags for the continuation frames as well.

Comment 62

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8920499 [details] [diff] [review]
Part 1: Cache MayHaveOpacityAnimation and MayHaveTransformAnimation in nsIFrame.

As per comment 60.

Comment 63

[Ya-Chieh Wu (inactive)]

Sorry that I might not clear on comment 58 but I've already only call |GetEffectSet| when the pseudo type is before or after or NotPseudo.

Comment 64

[Hiroyuki Ikezoe (:hiro)]

Np, it's my fault actually.

Comment 65

[Ya-Chieh Wu (inactive)]

Attachment: Bug 1381153 - Part 1: Cache MayHaveOpacityAnimation and MayHaveTransformAnimation in nsIFrame.

Comment 66

[Ya-Chieh Wu (inactive)]

Attachment: Bug 1381153 - Part 2: look up MayHaveOpacityAnimation and MayHaveTransformAnimation in nsIFrame.

Comment 67

[Ya-Chieh Wu (inactive)]

Hi mstange:

I would like to land Part 1 & Part 2 first since the bit gap in nsIFrame is shrinking quickly.

For part 1, I try to cache MayHaveTransformAnimation and MayHaveOpacityAnimation in nsIFrame.
(For further detail, I put my thoughts in the commit messages)

For part 2, I change the old way by looking up MayHaveTransformAnimation in EffectSet to nsFrame.
By doing so, we don't need to pass |EffectSet| all the way down to |HasAnimationOfProperty| which
looks like having some performance issues.

For MayHaveOpacityAnimation, I would like to create a follow-up bug to clean it up. 
In fact, I already pass all the test cases for MayHaveOpacityAnimation in nsFrame
but one reftest fails which is Bug 1406211 which has been landed recently and changed
some behavior of opacity animation. That's why I did not clean up MayHaveOpacityAnimation 
in part 2. I have to do further investigation for MayHaveOpacityAnimation but I think
MayHaveTransformAnimation in nsIFrame is ready to land.


Try looks ok for part 1 and part 2:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0bc0b069b494767fda59dd916614f1d343b0423

For part 3, I would like to create a follow-up bug to clean it up.

Comment 68

[Ya-Chieh Wu (inactive)]

Attachment: Bug 1381153 - Part 2: look up MayHaveTransformAnimation in nsIFrame.

Note: part 2 only clean up MayHaveTransformAnimation

Comment 69

[Ya-Chieh Wu (inactive)]

Hi mstange:

I put my comments on Comment 67 for you.
thanks!

Comment 70

[Markus Stange [:mstange]]

Comment on attachment 8921742 [details] [diff] [review]
Bug 1381153 - Part 1: Cache MayHaveOpacityAnimation and MayHaveTransformAnimation in nsIFrame.

Review of attachment 8921742 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +672,5 @@
> +    // The switch case below is the same as EffectSet::GetEffectSet(this);
> +    switch(pseudoType)
> +    {
> +      case CSSPseudoElementType::before:
> +        U_FALLTHROUGH;

I think this U_FALLTHROUGH is not needed because there's nothing else in this case branch. I could be  wrong though.

Comment 71

[Markus Stange [:mstange]]

Comment on attachment 8921750 [details] [diff] [review]
Bug 1381153 - Part 2: look up MayHaveTransformAnimation in nsIFrame.

Review of attachment 8921750 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

I'm really sorry that I took so extremely long to get to this review.

::: layout/base/nsLayoutUtils.cpp
@@ +474,5 @@
>    return true;
>  }
>  
> + static bool
> +MayHaveAnimationOfProperty(const nsIFrame* aFrame, nsCSSPropertyID aProperty)

The indentation in this function is a little off. Most lines have an extra space at the start which should be removed.

@@ +511,5 @@
>  nsLayoutUtils::HasAnimationOfProperty(const nsIFrame* aFrame,
>                                        nsCSSPropertyID aProperty)
>  {
> +  if (!MayHaveAnimationOfProperty(aFrame, aProperty)) {
> +     return false;

Please double-check the indentation in this method as well.

::: layout/generic/nsFrame.cpp
@@ +1562,5 @@
>  }
>  
>  bool
> +nsIFrame::Combines3DTransformWithAncestors(const nsStyleDisplay*
> +                                           aStyleDisplay) const

I think in this case there should be no line break, even though the line will be a bit longer than usual.

@@ +1585,4 @@
>  }
>  
>  bool
>  nsIFrame::HasPerspective(const nsStyleDisplay* aStyleDisplay, EffectSet* aEffectSet) const

Looks like this method no longer needs the aEffectSet parameter either. It can be removed.

Comment 72

[Markus Stange [:mstange]]

Thank you for doing this.

Comment 73

[Ya-Chieh Wu (inactive)]

Thanks for reviewing and I have addressed the comments.

try looks ok.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fe288b690e447eb64c02b80a1bb768970d95086&selectedJob=143591371

I will land these two patches tomorrow or the day after tomorrow since it's code freeze recently.

Comment 74

[Ya-Chieh Wu (inactive)]

Attachment: Bug 1381153 - Part 1: Cache MayHaveOpacityAnimation and MayHaveTransformAnimation in nsIFrame.

Comment 75

[Ya-Chieh Wu (inactive)]

Attachment: Bug 1381153 - Part 2: look up MayHaveTransformAnimation in nsIFrame.

Comment 76

[Ya-Chieh Wu (inactive)]

Attachment: Part 1: Cache MayHaveOpacityAnimation and MayHaveTransformAnimation in nsIFrame.

Comment 77

[Ya-Chieh Wu (inactive)]

Attachment: Part 2: look up MayHaveTransformAnimation in nsIFrame.

Comment 78

[Mats Palmgren (inactive)]

Comment on attachment 8927710 [details] [diff] [review]
Part 1: Cache MayHaveOpacityAnimation and MayHaveTransformAnimation in nsIFrame.

>+  if (mContent) {

Do we really need to execute this code block for all continuations?
I suspect we only need to do when aPrevInFlow is null and just
propagate the bits forward.

>+        U_FALLTHROUGH;

Please don't use U_FALLTHROUGH in Mozilla code, it comes from ICU:
https://searchfox.org/mozilla-central/rev/fe75164c55321e011d9d13b6d05c1e00d0a640be/intl/icu/source/common/unicode/platform.h#557
You should use MOZ_FALLTHROUGH (from mfbt/Attributes.h).

>+      case CSSPseudoElementType::before:
>+      case CSSPseudoElementType::after:
>+      case CSSPseudoElementType::NotPseudo:

Why are we excluding all other pseudos?  I see that we do the same in
EffectCompositor::GetAnimationElementAndPseudoForFrame but it seems
rather dubious to me.  (::placeholder for example can have opacity and
I assume it may be animated)

Anyway, we should call EffectCompositor::GetAnimationElementAndPseudoForFrame
here instead so we share the same logic.  Something like:
  content = EffectCompositor::GetAnimationElementAndPseudoForFrame(this);
  if (content && content->IsElement() && content->MayHaveAnimations()) {
    ...

Also, don't we need to update these frame bits on style changes?
E.g. if a frame is created for an element with default 'opacity' style,
and then the 'opacity' style changes; where do we update the frame bits
to reflect the new style?

Comment 79

[Mats Palmgren (inactive)]

Comment on attachment 8927711 [details] [diff] [review]
Part 2: look up MayHaveTransformAnimation in nsIFrame.

A few drive-by nits:

> +MayHaveAnimationOfProperty(const nsIFrame* aFrame, nsCSSPropertyID aProperty)
> +{
> + 
> +  if (aProperty == eCSSProperty_transform &&
> +     !aFrame->MayHaveTransformAnimation()) {
> +    return false;
> +  }
> +  if (aProperty == eCSSProperty_opacity &&
> +     !aFrame->MayHaveOpacityAnimation()) {
> +    return false;
> +  }
> + 
> +  return true;

This code is unnecessarily complex since the second 'if' is always mutually
exclusive with the first.  Also, shouldn't we assert that aProperty is
always one of these two values?  If so, I'd suggest using a switch instead:

switch (aProperty) {
  case eCSSProperty_transform:
    return aFrame->MayHaveTransformAnimation();
  case eCSSProperty_opacity:
    return aFrame->MayHaveOpacityAnimation();
  default:
    MOZ_ASSERT_UNREACHABLE("unexpected property");
    return false;
  }
}

> -  bool isTransformed = IsTransformed(disp, effectSet);
> -  bool hasPerspective = HasPerspective(effectSet);
> +  bool isTransformed = IsTransformed(disp);
> +  bool hasPerspective = HasPerspective();

I think you meant HasPerspective(disp).

Comment 80

[Mats Palmgren (inactive)]

(In reply to Mats Palmgren (:mats) from comment #78)
>   content = EffectCompositor::GetAnimationElementAndPseudoForFrame(this);
>   if (content && content->IsElement() && content->MayHaveAnimations()) {

It returns Maybe<NonOwningAnimationTarget>, so more like:
  auto target = EffectCompositor::GetAnimationElementAndPseudoForFrame(this);
  if (target.isSomething() && ...

Comment 81

[Ya-Chieh Wu (inactive)]

(In reply to Mats Palmgren (:mats) from comment #78) 
> >+      case CSSPseudoElementType::before:
> >+      case CSSPseudoElementType::after:
> >+      case CSSPseudoElementType::NotPseudo:
> 
> Why are we excluding all other pseudos?  I see that we do the same in
> EffectCompositor::GetAnimationElementAndPseudoForFrame but it seems
> rather dubious to me.  (::placeholder for example can have opacity and
> I assume it may be animated)
> 
> Anyway, we should call EffectCompositor::GetAnimationElementAndPseudoForFrame
> here instead so we share the same logic.  Something like:
>   content = EffectCompositor::GetAnimationElementAndPseudoForFrame(this);
>   if (content && content->IsElement() && content->MayHaveAnimations()) {
>     ...
> 
The reason I don't call |GetAnimationElementAndPseudoForFrame| directly is 
because in |GetAnimationElementAndPseudoForFrame|, it does |nsIContent* content = aFrame->GetContent();|
which doesn't look efficient. I think what we might be able to do is put these lines into a function.
However, the function will be only used here. So I don't think it is necessary to do that. How do you think?


> Also, don't we need to update these frame bits on style changes?
> E.g. if a frame is created for an element with default 'opacity' style,
> and then the 'opacity' style changes; where do we update the frame bits
> to reflect the new style?

(In reply to Mats Palmgren (:mats) from comment #78)
> Also, don't we need to update these frame bits on style changes?
> E.g. if a frame is created for an element with default 'opacity' style,
> and then the 'opacity' style changes; where do we update the frame bits
> to reflect the new style?

I think this will happen in |KeyframeEffectReadOnly::UpdateEffectSet|. no?

Comment 82

[Mats Palmgren (inactive)]

(In reply to Ya-Chieh Wu from comment #81)
> The reason I don't call |GetAnimationElementAndPseudoForFrame| directly is 
> because in |GetAnimationElementAndPseudoForFrame|, it does |nsIContent*
> content = aFrame->GetContent();|
> which doesn't look efficient. 

I'm pretty sure the compiler can inline that:
https://searchfox.org/mozilla-central/rev/fe75164c55321e011d9d13b6d05c1e00d0a640be/layout/generic/nsIFrame.h#758

> I think this will happen in |KeyframeEffectReadOnly::UpdateEffectSet|. no?

But UpdateEffectSet only updates one frame (GetAnimationFrame()),
the continuations will not be updated AFAICT.

Comment 83

[Ya-Chieh Wu (inactive)]

(In reply to Mats Palmgren (:mats) from comment #82)
> (In reply to Ya-Chieh Wu from comment #81)
> > The reason I don't call |GetAnimationElementAndPseudoForFrame| directly is 
> > because in |GetAnimationElementAndPseudoForFrame|, it does |nsIContent*
> > content = aFrame->GetContent();|
> > which doesn't look efficient. 
> 
> I'm pretty sure the compiler can inline that:
> https://searchfox.org/mozilla-central/rev/
> fe75164c55321e011d9d13b6d05c1e00d0a640be/layout/generic/nsIFrame.h#758
> 

ok, then I probably can just call |EffectSet::GetEffectSet(const nsIFrame* aFrame)|.

> > I think this will happen in |KeyframeEffectReadOnly::UpdateEffectSet|. no?
> 
> But UpdateEffectSet only updates one frame (GetAnimationFrame()),
> the continuations will not be updated AFAICT.

Yes, this can be an issue for |opacity|. And for |opacity|, I will create a follow-up to handle it.
I have some explains on Comment 67. For now, I only touch |transfrom|. 
I wonder if this address your concerns?

Comment 84

[Ya-Chieh Wu (inactive)]

Attachment: Part-2-look-up-MayHaveTransformAnimation.patch

Comment 85

[Ya-Chieh Wu (inactive)]

Attachment: Part-1-Cache-MayHaveOpacityAnimation-and.patch

mats,

In this patch, I only touch |nsFrame::init|. 

Based on the above comment suggested, I used |EffectSet* effectSet = EffectSet::GetEffectSet(this);| to get the |EffectSet| so that we can share the same logic in |EffectCompositor::GetAnimationElementAndPseudoForFrame|.(not duplicate the logic)  I also executed this part when |aPrevInFlow| is null. Otherwise we propagate the bits forward. How do you think about this?

And again, I only touch |transfrom| here. For |opacity|, we might need to come up where we can make sure that we also update the continue frame or maybe we have to direct the frame check to prime frame but this will not be handled here.

Comment 86

[Ya-Chieh Wu (inactive)]

Attachment: Part-1-Cache-MayHaveOpacityAnimation-and.patch

Hi mats, same comment as comment 85.

Triage a try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed57f7a8c21b00c4573a686071e95720f56718ee

Comment 87

[Mats Palmgren (inactive)]

Comment on attachment 8927749 [details] [diff] [review]
Part-1-Cache-MayHaveOpacityAnimation-and.patch

Looks OK, r=mats

Comment 88

[Markus Stange [:mstange]]

Thanks, Mats.
I keep forgetting that you're somebody that I can ask for help with patches that I'm not 100% comfortable reviewing.

Comment 89

[Astley Chen (inactive)]

(In reply to Ya-Chieh Wu from comment #83)
> Yes, this can be an issue for |opacity|. And for |opacity|, I will create a
> follow-up to handle it.

Do we have a follow-up bug to track now?

Comment 90

[Ya-Chieh Wu (inactive)]

Attachment: Part-1-Cache-MayHaveOpacityAnimation-and.patch

try looks ok:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=31c47a004243f7ea5956c3614a4792f64de04077&selectedJob=144228513

Comment 91

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/161226f14afd
Part 1: Cache MayHaveOpacityAnimation and MayHaveTransformAnimation in nsIFrame. r=mstange, r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/168ef20c4a31
Part 2: Look up MayHaveTransformAnimation in nsIFrame. r=mstange

Comment 92

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/161226f14afd
https://hg.mozilla.org/mozilla-central/rev/168ef20c4a31