Closed Bug 1504929 Opened Last year Closed 11 months ago

Make RestyleManager::AddLayerChangesForAnimation more efficient

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Depends on 1 open bug)

Details

Attachments

(10 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Then we can handle multiple CSS properties that generates the same display item and the same change hints.
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).
Assignee: nobody → hikezoe
Summary: Change nsCSSPropertyID in LayerAnimationInfo::Record to nsCSSPropertyIDSet → Make RestyleManager::AddLayerChangesForAnimation more efficient
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
Flags: needinfo?(sotaro.ikeda.g)
Yea, it looks reasonable:)
Flags: needinfo?(sotaro.ikeda.g)
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. :)
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
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
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
Attachment #9023843 - Attachment description: Bug 1504929 - Stop iterating EffectSets and CSS properties inside each loop in RestyleManager::AddLayerChangesForAnimations. r?birtles! → Bug 1504929 - Stop iterating EffectSets and KeyframeEffect::mProperties for each CSS properties that can be animated on the compositor. r?birtles!
Attachment #9023844 - Attachment description: Bug 1504929 - Reduce iteration numbers in RestyleManager::AddLayerChangesForAnimations. r?birtles!,sotaro! → Bug 1504929 - Further optimizations for RestyleManager::AddLayerChangesForAnimations.. r?birtles!,sotaro!
Attachment #9024335 - Attachment description: Bug 1504929 - Unify GetEffectiveAnimationProperties and GetEffectiveAnimationOfProperty. r?birtles! → Bug 1504929 - Factor out IsEffectiveProperty(). r?birtles!
Attachment #9024333 - Attachment description: Bug 1504929 - KeyframeEffect::GetEffectiveAnimationOfProperty also takes EffectSet&. r?birtles! → Bug 1504929 - Avoid hashmap lookups in nsLayoutUtils::HasEffectiveAnimation and EffectCompositor::FindAnimationsForCompositor. r?birtles!
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
Flags: needinfo?(sotaro.ikeda.g)
(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?
Flags: needinfo?(sotaro.ikeda.g)
Thank you, sotaro!  Filed bug 1506758 for that.  And I am going to send a review request to you for the workaround patch.
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
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. :/
Pushed by hikezoe@mozilla.com:
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
Depends on: 1507083
Depends on: 1507234
You need to log in before you can comment on or make changes to this bug.