Closed Bug 1504929 Opened Last year Closed 11 months ago
Manager::Add Layer Changes For Animation more efficient
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  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  to the code what I did. Sotaro, does this way sound reasonable? FWIW here is a link to the assertion failure , it happened on dom/xul/test/test_bug486990.xul.  https://hg.mozilla.org/mozilla-central/file/12cc80a0e996/widget/PuppetWidget.cpp#l626  https://hg.mozilla.org/try/rev/9b15a774b778ee2d838605583d76025582957cd8#l1.45  https://treeherder.mozilla.org/logviewer.html#?job_id=210497613&repo=try&lineNumber=8644
Yea, it looks reasonable:)
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
Depends on D11426
Depends on D11425
Depends on D11427
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
Depends on D11598
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!
Here is a try with updated patches; https://treeherder.mozilla.org/#/jobs?repo=try&revision=59c9b46c38f58c5bce020e9c7b9fa2681ee6f3be
Since we use the EffectSet later in the same function. Depends on D11599
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 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).  https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a64c8879f781deba12008243f8e7fb35cae6c2f&selectedJob=211182467
(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?
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 firstname.lastname@example.org: 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
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
You need to log in before you can comment on or make changes to this bug.