Closed Bug 1205475 Opened 9 years ago Closed 7 years ago

nsIFrame::HasOpacityInternal takes a lot of time

Categories

(Core :: Web Painting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox43 --- affected
firefox55 --- fixed

People

(Reporter: mattwoodrow, Assigned: u459114)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: perf)

Attachments

(2 files, 1 obsolete file)

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)
Component: Layout → Layout: Web Painting
Keywords: perf
Assignee: howareyou322 → cku
The idea is set NS_FRAME_MAY_HAVE_OPACITY flag into frame right before animation start and remove it at the end of animation
Add NS_FRAME_MAY_HAVE_OPACITY to element in EffectSet::AddEffect, and clear this flag in EffectSet::RemoveEffect
Attachment #8867991 - Attachment is obsolete: true
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
Attachment #8868096 - Flags: review?(matt.woodrow)
Attachment #8868448 - Flags: review?(matt.woodrow)
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+
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+
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
https://hg.mozilla.org/mozilla-central/rev/d7b25abd4866
https://hg.mozilla.org/mozilla-central/rev/cf4e37d17683
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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?
(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.
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.

Attachment

General

Creator:
Created:
Updated:
Size: