1205475 - nsIFrame::HasOpacityInternal takes a lot of time

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Checking for the presence of async animations on every frame when we try paint ends up taking a considerable amount of time.

We should add a NS_FRAME_MAY_HAVE_OPACITY state bit to avoid most of this work when we can.

Comment 1

[Jet Villegas (inactive)]

To Peter for assignment

Comment 2

[Astley Chen (inactive)]

Peter, is anyone working on this bug now ?

Comment 3

[Peter Chang[:pchang]]

I don't have resource to work on this bug right now. Feel free to take this bug.

Comment 4

[u459114]

The idea is set NS_FRAME_MAY_HAVE_OPACITY flag into frame right before animation start and remove it at the end of animation

Comment 5

[u459114]

Attachment: Bug 1205475 - Early return in nsLayoutUtils::HasAnimationOfProperty if there is no animation on the target frame.

Early return in nsLayoutUtils::HasAnimationOfProperty to save execution time of
the callers, which includes nsIFrame::HasAnimationOfTransform,
nsIFrame::HasOpacityInternal and nsIFrame::BuildDisplayListForStackingContext.

Review commit: https://reviewboard.mozilla.org/r/139530/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139530/

Comment 6

[u459114]

Comment on attachment 8867991 [details]
Bug 1205475 - Early return in nsLayoutUtils::HasAnimationOfProperty if there is no animation on the target frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139530/diff/1-2/

Comment 7

[u459114]

Add NS_FRAME_MAY_HAVE_OPACITY to element in EffectSet::AddEffect, and clear this flag in EffectSet::RemoveEffect

Comment 8

[u459114]

Attachment: Bug 1205475 - Part 1. Hold MAY_HAVE_OPACITY_ANIM/ MAY_HAVE_TRANSFOMR_ANIM information in EffectSet.

Review commit: https://reviewboard.mozilla.org/r/139710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139710/

Comment 9

[u459114]

Comment on attachment 8868096 [details]
Bug 1205475 - Part 1. Hold MAY_HAVE_OPACITY_ANIM/ MAY_HAVE_TRANSFOMR_ANIM information in EffectSet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139710/diff/1-2/

Comment 10

[u459114]

To bad, FRAME_STATE_BIT of nsFrame is full...BooleanFlag of nsINode is full too.. I need to find anohter place to put this flag

Comment 11

[u459114]

Attachment: Bug 1205475 - Part 2. Prevent multiple EffectSet property look up.

Functions like BuildDisplayListForStackingContext or BuildDisplayListForChild look
up EffectSet many times, which waste time.

We can look up EffectSet just once, and pass it to functions that need it.
With this patch, we reduce the total number of EffectSet::GetEffectSet calls in
nsIFrame.

Review commit: https://reviewboard.mozilla.org/r/140050/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140050/

Comment 12

[u459114]

Comment on attachment 8868096 [details]
Bug 1205475 - Part 1. Hold MAY_HAVE_OPACITY_ANIM/ MAY_HAVE_TRANSFOMR_ANIM information in EffectSet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139710/diff/2-3/

Comment 13

[u459114]

Comment on attachment 8868096 [details]
Bug 1205475 - Part 1. Hold MAY_HAVE_OPACITY_ANIM/ MAY_HAVE_TRANSFOMR_ANIM information in EffectSet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139710/diff/3-4/

Comment 14

[u459114]

Comment on attachment 8868448 [details]
Bug 1205475 - Part 2. Prevent multiple EffectSet property look up.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140050/diff/1-2/

Comment 15

[u459114]

Comment on attachment 8868096 [details]
Bug 1205475 - Part 1. Hold MAY_HAVE_OPACITY_ANIM/ MAY_HAVE_TRANSFOMR_ANIM information in EffectSet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139710/diff/4-5/

Comment 16

[u459114]

Comment on attachment 8868448 [details]
Bug 1205475 - Part 2. Prevent multiple EffectSet property look up.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140050/diff/2-3/

Comment 17

[u459114]

Comment on attachment 8868448 [details]
Bug 1205475 - Part 2. Prevent multiple EffectSet property look up.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140050/diff/3-4/

Comment 18

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8868096 [details]
Bug 1205475 - Part 1. Hold MAY_HAVE_OPACITY_ANIM/ MAY_HAVE_TRANSFOMR_ANIM information in EffectSet.

https://reviewboard.mozilla.org/r/139710/#review143756

::: layout/base/nsLayoutUtils.cpp:517
(Diff revision 5)
>      }
>    );
>  }
>  
> +static bool
> +CheckOpacityAndTransformAnim(EffectSet* effects, nsCSSPropertyID aProperty)

MayHaveAnimationOfProperty?

Comment 19

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8868448 [details]
Bug 1205475 - Part 2. Prevent multiple EffectSet property look up.

https://reviewboard.mozilla.org/r/140050/#review143758

Nice!

Comment 20

[u459114]

Comment on attachment 8868096 [details]
Bug 1205475 - Part 1. Hold MAY_HAVE_OPACITY_ANIM/ MAY_HAVE_TRANSFOMR_ANIM information in EffectSet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139710/diff/5-6/

Comment 21

[u459114]

Comment on attachment 8868448 [details]
Bug 1205475 - Part 2. Prevent multiple EffectSet property look up.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140050/diff/4-5/

Comment 22

[u459114]

Comment on attachment 8868448 [details]
Bug 1205475 - Part 2. Prevent multiple EffectSet property look up.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140050/diff/5-6/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7b25abd4866
Part 1. Hold MAY_HAVE_OPACITY_ANIM/ MAY_HAVE_TRANSFOMR_ANIM information in EffectSet. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/cf4e37d17683
Part 2. Prevent multiple EffectSet property look up. r=mattwoodrow

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d7b25abd4866
https://hg.mozilla.org/mozilla-central/rev/cf4e37d17683

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8868096 [details]
Bug 1205475 - Part 1. Hold MAY_HAVE_OPACITY_ANIM/ MAY_HAVE_TRANSFOMR_ANIM information in EffectSet.

https://reviewboard.mozilla.org/r/139710/#review144950

::: dom/animation/EffectSet.h:263
(Diff revision 6)
> +  uint32_t mMayHaveOpacityAnim;
> +  uint32_t mMayHaveTransformAnim;

Is there any strong reason why we use uint32_t instead of bool?

Comment 26

[u459114]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> Comment on attachment 8868096 [details]
> Bug 1205475 - Part 1. Hold MAY_HAVE_OPACITY_ANIM/ MAY_HAVE_TRANSFOMR_ANIM
> information in EffectSet.
> 
> https://reviewboard.mozilla.org/r/139710/#review144950
> 
> ::: dom/animation/EffectSet.h:263
> (Diff revision 6)
> > +  uint32_t mMayHaveOpacityAnim;
> > +  uint32_t mMayHaveTransformAnim;
> 
> Is there any strong reason why we use uint32_t instead of bool?

No, my bad. It should be bool. Thank you for catching this.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7af9cc734f9e
(followup) Declare mMayHaveOpacityAnim and mMayHaveTransformAnim as bool. r=me

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7af9cc734f9e