nsIFrame::HasOpacityInternal takes a lot of time

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: mattwoodrow, Assigned: u459114)

Tracking

(Depends on 1 bug, Blocks 3 bugs, {perf})

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox55 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
To Peter for assignment
Assignee: matt.woodrow → howareyou322
Depends on: 1257732
Peter, is anyone working on this bug now ?
Flags: needinfo?(howareyou322)
I don't have resource to work on this bug right now. Feel free to take this bug.
Flags: needinfo?(howareyou322)
Assignee: howareyou322 → cku
(Assignee)

Comment 4

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
Add NS_FRAME_MAY_HAVE_OPACITY to element in EffectSet::AddEffect, and clear this flag in EffectSet::RemoveEffect
(Assignee)

Updated

2 years ago
Attachment #8867991 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8868448 - Flags: review?(matt.woodrow)
Attachment #8868096 - Flags: review?(matt.woodrow)
(Reporter)

Comment 18

2 years ago
mozreview-review
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?
Attachment #8868096 - Flags: review?(matt.woodrow) → review+
(Reporter)

Comment 19

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

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

Nice!
Attachment #8868448 - Flags: review?(matt.woodrow) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 23

2 years ago
Pushed by cku@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d7b25abd4866
https://hg.mozilla.org/mozilla-central/rev/cf4e37d17683
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 25

2 years ago
mozreview-review
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?
(Assignee)

Comment 26

2 years ago
(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

2 years ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7af9cc734f9e
(followup) Declare mMayHaveOpacityAnim and mMayHaveTransformAnim as bool. r=me
Depends on: 1371267
You need to log in before you can comment on or make changes to this bug.