1504929 - Make RestyleManager::AddLayerChangesForAnimation more efficient

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

Then we can handle multiple CSS properties that generates the same display item and the same change hints.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Initially I was thinking that just nsCSSPropertyID is simply replaced with nsCSSPropertyIDSet in LayerAnimationInfo, but after some more thoughts, I came up with an idea to eliminate LayerAnimationinfo.sRecord in RestyleManager::AddLayerChangesForAnimation, that means the function would be more efficient.

With the current setup, we have many kinds of iterations in the function:

1. sRecords
 2. nsIFrame::DisplayItemData via AnimationInfo::GetGenerationFromFrame
 3. EffectSets on the nsIFrame via nsLayoutUtils::HasEffectiveAnimation
   4. KeyframeEffect::mProperties via KeyframeEffect::HasEffectiveAnimationOfProperty

With the new setup the iterations would be a single nsIFrame::DisplayItemData basically (or just display item types iteration for WebRender).

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I had been hit by an assertion [1] in PuppetWidget::GetLayerManager() which I am using to tell whether we are on WebRender or not.  In my understanding the assertion checks that it should never happen the case where the PuppetWidget has no LayerManager but the corresponding TabChild thinks it's already connected.  Unfortunately PuppetWidgets created by PuppetWidget::CreateChild has a TabChild which is the same as the parent's one.  So if we call PuppetWidget::GetLayerManager for a child PuppetWidget, the assertion fails.  That's exactly the case what I had been seeing.

So to prevent the assertion I added a PuppetWidget::HasLayerManager() check for the nsIWidget obtained by nsContentUtils::WidgetForContent().  Here is a link [2] to the code what I did.

Sotaro, does this way sound reasonable?

FWIW here is a link to the assertion failure [2], it happened on dom/xul/test/test_bug486990.xul.

[1] https://hg.mozilla.org/mozilla-central/file/12cc80a0e996/widget/PuppetWidget.cpp#l626
[2] https://hg.mozilla.org/try/rev/9b15a774b778ee2d838605583d76025582957cd8#l1.45
[3] https://treeherder.mozilla.org/logviewer.html#?job_id=210497613&repo=try&lineNumber=8644

Comment 3

[Sotaro Ikeda [:sotaro]]

Yea, it looks reasonable:)

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Thank you, Sotaro!  I will send a review request for the change to you.  I was thinking I can finish writing commit messages while I am waiting for your reply, but actually coulnd't. :)

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1504929 - Mark AnimationInfo::GetAnimationGeneration as a const function. r?birtles!

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1504929 - Stop iterating EffectSets and KeyframeEffect::mProperties for each CSS properties that can be animated on the compositor. r?birtles!

This change gets all effective CSS properties on an nsIFrame just once.

Note that LayerAnimationInfo::GetCSSPropertiesFor intentionally returns
nsCSSPropertyIDSet instead of nsCSSPropertyID since when we support individual
transform properties for the compositor the mapping between display item types
and nsCSSProperty has to be 1:N. E.g. all scale/translate/rotate properties are
mapped to transform display item.

Depends on D11424

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1504929 - Further optimizations for RestyleManager::AddLayerChangesForAnimations.. r?birtles!,sotaro!

This change eliminates
 - nsLayoutUtils::LastContinuationOrIBSplitSibling calls for each CSS
   properties on WebRender
 - iterating over each display item for each compositor runnable CSS properties
 - a bunch of stuff in the case where the layer manager has not yet created,
   i.e. the compositor thread is not ready to receive animations

Depends on D11425

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1504929 - Drop LayerAnimationInfo::sRecords. r?birtles!

Depends on D11426

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1504929 - Introduce LayerAnimationInfo::sDisplayItemTypes and iterate it instead of LayerAnimationInfo::sRecords. r?birtles!

Depends on D11425

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1504929 - Avoid hashmap lookups in nsLayoutUtils::HasEffectiveAnimation and EffectCompositor::FindAnimationsForCompositor. r?birtles!

Depends on D11427

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1504929 - Make `if` conditions in KeyframeEffect::GetEffectiveAnimationOfProperty negative. r?birtles!

I.e., continue the loop if the CSS property is not what we want and set the
result value only if the CSS property is effective.

This change makes the function match
what KeyframeEffect::GetEffectiveAnimationProperties does in the similar loop
so that we can unify the iteration into a single function in the next commit.

Depends on D11597

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1504929 - Factor out IsEffectiveProperty(). r?birtles!

Depends on D11598

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Here is a try with updated patches;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59c9b46c38f58c5bce020e9c7b9fa2681ee6f3be

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1504929 - Break RestyleManager::GetAnimationGenerationForFrame into EffectSet::GetEffect and EffectSet::GetAnimationGeneration in AddAnimationsForProperty. r?birtles!,sotaro!

Since we use the EffectSet later in the same function.

Depends on D11599

Comment 15

[Hiroyuki Ikezoe (:hiro)]

These patches makes some reftests perma-fail on MacOSX opt builds for WebRender. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a405a82216b72da350af4e9b8647191af5c8f44&selectedJob=211139518

We stuck at STATE_WAITING_TO_FIRE_INVALIDATE_EVENT state which is the initial state of reftest harness, that means we continuously received MozAfterPaint events.  We need the same workaround for bug 1489327 here as well.

Given that the failures happen only on MacOSX *opt* builds and doesn't happen on MacOSX debug builds, I am pretty sure that my patches make some performance improvement for WebRender.  Thus without the patches it's possible that there is a frame that we don't receive a MozAfterPaint, but with the patches we can keep painting on every frame.  A big performance improvement in the patches for WebRender is that before the patches we try to find corresponding layers for each compositor runnable CSS properties but now we don't do it anymore.

Anyway, I did a try run with the workaround, but still animate-backface-hidden.html fails[1] intermittently, but in the new failure case, we stuck at STATE_WAITING_TO_FINISH.  I know the reason, that's because transform animations with backface-visibility:hidden don't run on the compositor (bug 1186204), thus we keep painting on the main-thread unfortunately. 

Hey sotaro, is that OK to disable animate-backface-hidden.html on MacOSX opt builds for WebRender?  What I am worried is that once gfx team makes more performance improvements the test might start failing on other platforms.  And the most worrisome thing for me is that I don't have any ideas to solve the intermittent failure other than fixing bug 1186204 (i.e. making transform animations with backface-visibility run on the compositor).

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a64c8879f781deba12008243f8e7fb35cae6c2f&selectedJob=211182467

Comment 16

[Sotaro Ikeda [:sotaro]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
>
> Hey sotaro, is that OK to disable animate-backface-hidden.html on MacOSX opt
> builds for WebRender?  What I am worried is that once gfx team makes more
> performance improvements the test might start failing on other platforms. 
> And the most worrisome thing for me is that I don't have any ideas to solve
> the intermittent failure other than fixing bug 1186204 (i.e. making
> transform animations with backface-visibility run on the compositor).

Thanks for the detailed explanation! It is ok, but can you create a new bug for WebRender?

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Thank you, sotaro!  Filed bug 1506758 for that.  And I am going to send a review request to you for the workaround patch.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1504929 - Start animations once after a MozReftestInvalidate event is received. r?sotaro!

We need the same workaround for
https://bugzilla.mozilla.org/show_bug.cgi?id=1489327 .

And unfortunately we have to disable animate-backface-hidden.html on MacOSX opt
builds for WebRender because the test fails intermittently, we will enable it
in bug 1506758.

Depends on D11601

Comment 19

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a605d8acba001a1e056b9b180098fb80f58a877e

The test (and other two preserve-3d reftests) fail on MacOSX debug build too, but less often.  I am going to skip them on MacOSX. :/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98a44b1a44cd
Mark AnimationInfo::GetAnimationGeneration as a const function. r=birtles
https://hg.mozilla.org/integration/autoland/rev/4a805bf94a32
Stop iterating EffectSets and KeyframeEffect::mProperties for each CSS properties that can be animated on the compositor. r=birtles
https://hg.mozilla.org/integration/autoland/rev/c1bad90a5650
Introduce LayerAnimationInfo::sDisplayItemTypes and iterate it instead of LayerAnimationInfo::sRecords. r=birtles
https://hg.mozilla.org/integration/autoland/rev/f82c7330018b
Further optimizations for RestyleManager::AddLayerChangesForAnimations.. r=birtles,sotaro
https://hg.mozilla.org/integration/autoland/rev/33ac76a8930d
Drop LayerAnimationInfo::sRecords. r=birtles
https://hg.mozilla.org/integration/autoland/rev/160a7716c1b3
Avoid hashmap lookups in nsLayoutUtils::HasEffectiveAnimation and EffectCompositor::FindAnimationsForCompositor. r=birtles
https://hg.mozilla.org/integration/autoland/rev/d77f35fe6d73
Make `if` conditions in KeyframeEffect::GetEffectiveAnimationOfProperty negative. r=birtles
https://hg.mozilla.org/integration/autoland/rev/b59d17710f5c
Factor out IsEffectiveProperty(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/de569bb95b1c
Break RestyleManager::GetAnimationGenerationForFrame into EffectSet::GetEffect and EffectSet::GetAnimationGeneration in AddAnimationsForProperty. r=birtles,sotaro
https://hg.mozilla.org/integration/autoland/rev/c84f0ad36ce9
Start animations once after a MozReftestInvalidate event is received. r=sotaro

Comment 21

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/98a44b1a44cd
https://hg.mozilla.org/mozilla-central/rev/4a805bf94a32
https://hg.mozilla.org/mozilla-central/rev/c1bad90a5650
https://hg.mozilla.org/mozilla-central/rev/f82c7330018b
https://hg.mozilla.org/mozilla-central/rev/33ac76a8930d
https://hg.mozilla.org/mozilla-central/rev/160a7716c1b3
https://hg.mozilla.org/mozilla-central/rev/d77f35fe6d73
https://hg.mozilla.org/mozilla-central/rev/b59d17710f5c
https://hg.mozilla.org/mozilla-central/rev/de569bb95b1c
https://hg.mozilla.org/mozilla-central/rev/c84f0ad36ce9