Closed Bug 1344619 Opened 8 years ago Closed 8 years ago

stylo: Introduce pre-traversal step to update a bunch of stuff that we shouldn't update during the parallel traversal

Categories

(Core :: DOM: Animation, enhancement)

Product:
Core
Component:
DOM: Animation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(7 files)

Bug 1344619 - Part 1: Introduce WillCompose() to update various state that we need to update as the result of ComposeStyle().
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1344619 - Part 2: Introduce EffectCompositor::PreTraverse().
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1344619 - Part 3: Kick EffectCompositor::PreTraverse().
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1344619 - Part 4: Add ServoStyleSet::ResolveStyleLazily().
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1344619 - Part 5: Introduce EffectCompositor::PreTraverse() for an element.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1344619 - Part 6: Kick EffectCompositor::PreTraverse() for an element.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1344619 - Part 7: Drop EffectCompositor::ClearElementsToRestyle.
59 bytes, text/x-review-board-request
birtles
: review+
Details
1. EffectCompositor::WillComposeAnimations - Iterate over all elements in mElementsToRestyle with a 'true' flag (i.e. posted restyle) - Call WillComposeStyle on each animation in the element's EffectSet and have it update its mProgressOnLastCompose / mCurrentIterationOnLastCompose / mFinishedAtLastComposeStyle etc. state - Clear from mElementsToRestyle all the elements we visited (i.e. leave the ones with only a throttled restyle) (2. Do animation restyle) 3. Do parallel traversal 4. Generate/update/drop animations and add them to mElementsToRestyle If needed: 5. Do another pre-traversal step as with (1) 6. Do traversal where we apply animations
Summary: stylo: Introduce pre-traversal step to update a bunch of stuff that we should update during the parallel traversal → stylo: Introduce pre-traversal step to update a bunch of stuff that we shouldn't update during the parallel traversal
Blocks: 1339704
Initial try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c28541390476d5762e9ab42744a53befe150c6ef Let's see if what happens. I guess it will not affect for gecko backend and we don't run any valid test cases for stylo backend unfortunately.
Gosh! I did forget to drop ClearElementsToRestyle() in ServoRestyleManager::ProcessPendingRestyles().
Blocks: 1344662
The try looks good. The reduced number of mochitest failures is due to bug 1338921. These patch might increase frequency of the failure of background-position-after-finish.html, I am not sure but it will fix by bug 1341985.
I did confirm that calling MaybeUpdateCascadeResults() in the pre-traversal and passing mPropertiesForAnimationsLevel to ComposeStyle() fixes bug 1339704. So, yeah I am now convinced we can clear mElementsToRestyle in the pre-traversal. Even if we noticed later that we need mElementsToRestyle during parallel traversal, we need this pre-traversal step for other things (e.g. for bug 1344662).
Comment on attachment 8844234 [details] Bug 1344619 - Part 3: Kick EffectCompositor::PreTraverse(). https://reviewboard.mozilla.org/r/117740/#review119418 ::: layout/style/ServoStyleSet.cpp:202 (Diff revision 1) > + // XXX: I am not convinced this is a right place to do the pre-traversal. > + // This function might to be called multiple times during while loop in > + // ServoStyleSet::StyleDocument(), if we have lots of throttled animations, > + // we will iterate over all throttled animations again and again. > + mPresContext->EffectCompositor()->PreTraverse(); Cameron, any thought about this? Should we call this PreTraverse() in StyleDocument() , StyleNewSubtree() and StyleNewChildren() respectively?
Comment on attachment 8844234 [details] Bug 1344619 - Part 3: Kick EffectCompositor::PreTraverse(). https://reviewboard.mozilla.org/r/117740/#review119508 ::: layout/style/ServoStyleSet.cpp:202 (Diff revision 1) > + // XXX: I am not convinced this is a right place to do the pre-traversal. > + // This function might to be called multiple times during while loop in > + // ServoStyleSet::StyleDocument(), if we have lots of throttled animations, > + // we will iterate over all throttled animations again and again. > + mPresContext->EffectCompositor()->PreTraverse(); Yes, moving the PreTraverse() call up to those three functions sounds right to me. Is PreTraverse() cheap, if it has no work to do? I think StyleDocument() needs to be cheap if there are no restyles to process. By the way, I think the ResolveMappedAttrDeclarationBlocks() call that is currently at the top of PrepareAndTraverseSubtree() should also be hoisted up somewhere. So maybe you can add a ServoStyleSet::PreTraverse function that does that call too.
Attachment #8844234 - Flags: review?(cam) → review-
Comment on attachment 8844235 [details] Bug 1344619 - Part 4: Add ServoStyleSet::ResolveStyleLazily(). https://reviewboard.mozilla.org/r/117742/#review119510
Attachment #8844235 - Flags: review?(cam) → review+
Comment on attachment 8844237 [details] Bug 1344619 - Part 6: Kick EffectCompositor::PreTraverse() for an element. https://reviewboard.mozilla.org/r/117746/#review119514
Attachment #8844237 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #14) > ::: layout/style/ServoStyleSet.cpp:202 > (Diff revision 1) > > + // XXX: I am not convinced this is a right place to do the pre-traversal. > > + // This function might to be called multiple times during while loop in > > + // ServoStyleSet::StyleDocument(), if we have lots of throttled animations, > > + // we will iterate over all throttled animations again and again. > > + mPresContext->EffectCompositor()->PreTraverse(); > > Yes, moving the PreTraverse() call up to those three functions sounds right > to me. > > Is PreTraverse() cheap, if it has no work to do? I think StyleDocument() > needs to be cheap if there are no restyles to process. > > By the way, I think the ResolveMappedAttrDeclarationBlocks() call that is > currently at the top of PrepareAndTraverseSubtree() should also be hoisted > up somewhere. So maybe you can add a ServoStyleSet::PreTraverse function > that does that call too. Sounds really nice. Thank you.
Comment on attachment 8844234 [details] Bug 1344619 - Part 3: Kick EffectCompositor::PreTraverse(). https://reviewboard.mozilla.org/r/117740/#review119552
Attachment #8844234 - Flags: review?(cam) → review+
Comment on attachment 8844232 [details] Bug 1344619 - Part 1: Introduce WillCompose() to update various state that we need to update as the result of ComposeStyle(). https://reviewboard.mozilla.org/r/117736/#review119898 This looks good but I wonder why the entry point is on the effect. In Stylo do we need to call WillCompose on the effect instead of the animation? For the Gecko case, I just imagined we would do: animation->WillComposeStyle(); // calls mEffect->WillComposeStyle() animation->ComposeStyle(); It feels a bit odd for the effect to call its animation. Is there a reason for doing it this way? ::: dom/animation/Animation.h:317 (Diff revision 2) > * running on the compositor). > */ > bool CanThrottle() const; > + > + /** > + * Updates various state that we need to update as the results of s/various state/various bits of state/ ::: dom/animation/Animation.cpp:990 (Diff revision 2) > mHoldTime.SetValue((timeToUse.Value() - mStartTime.Value()) > .MultDouble(mPlaybackRate)); (I guess static analysis is not going to like this...) ::: dom/animation/KeyframeEffectReadOnly.h:238 (Diff revision 2) > + // Update various state that we need to update as the results of > + // ComposeStyle(). > + // We need to update those state outside ComposeStyle() because we should > + // avoid mutating any state in ComposeStyle() since it might be called during > + // parallel traversal. Update various bits of state related to running ComposeStyle(). We need to update this state outside ComposeStyle() ...
Comment on attachment 8844233 [details] Bug 1344619 - Part 2: Introduce EffectCompositor::PreTraverse(). https://reviewboard.mozilla.org/r/117738/#review119902 This looks good but one thing I don't understand is, suppose we have element A and it has posted a throttled restyle. Now, suppose script also alters element A's style attribute, for example. When we do the pre-traversal for element A, we won't remove it from the hashmap because it only has a throttled restyle. However, when the do the parallel traversal, presumably we'll update its animation rule when we apply the changes to its style attribute. So, at the end the traversal, element A is still in the hashmap but it has been restyled for animation. How should we update the hashmap in that case? ::: dom/animation/EffectCompositor.cpp:949 (Diff revision 2) > +{ > + MOZ_ASSERT(mPresContext->RestyleManager()->IsServo()); > + > + for (auto& elementsToRestyle : mElementsToRestyle) { > + for (auto iter = elementsToRestyle.Iter(); !iter.Done(); iter.Next()) { > + bool& postedRestyle = iter.Data(); Any need for this to be a reference? ::: dom/animation/EffectCompositor.cpp:958 (Diff revision 2) > + if (!effects) { > + // Drop the effect that has been destroyed. > + iter.Remove(); > + continue; > + } Does this ever happen? If so, I'm curious why. If not, perhaps we should add an assertion here. ::: dom/animation/EffectCompositor.cpp:965 (Diff revision 2) > + iter.Remove(); > + continue; > + } > + > + for (KeyframeEffectReadOnly* effect : *effects) { > + effect->WillComposeStyle(); (As per my comment on part 1, I wonder if this could just be effect->GetAnimation()->WillComposeStyle() much like we do in EffectCompositor::ComposeAnimationRule) ::: dom/animation/EffectCompositor.cpp:968 (Diff revision 2) > + > + for (KeyframeEffectReadOnly* effect : *effects) { > + effect->WillComposeStyle(); > + } > + > + iter.Remove(); Let's add a comment: // Remove the element from the list of elements to restyle since we are about to restyle it.
(In reply to Brian Birtles (:birtles) from comment #26) > This looks good but I wonder why the entry point is on the effect. In Stylo > do we need to call WillCompose on the effect instead of the animation? > > For the Gecko case, I just imagined we would do: > > animation->WillComposeStyle(); // calls mEffect->WillComposeStyle() > animation->ComposeStyle(); > > It feels a bit odd for the effect to call its animation. Is there a reason > for doing it this way? Ah, indeed. We should trigger Animation's method. > ::: dom/animation/Animation.cpp:990 > (Diff revision 2) > > mHoldTime.SetValue((timeToUse.Value() - mStartTime.Value()) > > .MultDouble(mPlaybackRate)); > > (I guess static analysis is not going to like this...) Good catch. You know how the analysis tool feels. ;-)
Comment on attachment 8844233 [details] Bug 1344619 - Part 2: Introduce EffectCompositor::PreTraverse(). https://reviewboard.mozilla.org/r/117738/#review119908 ::: dom/animation/EffectCompositor.h:236 (Diff revision 2) > + // Do a bunch of stuff that we should avoid doing during the parallel > + // traversal (E.g. changing member variables) for all elements that needs > + // to be composed styles. Nit: s/E.g./e.g./ Nit: s/all elements that needs to be composed styles/all elements that we expect to restyle on the next traversal/
(In reply to Brian Birtles (:birtles) from comment #27) > This looks good but one thing I don't understand is, suppose we have element > A and it has posted a throttled restyle. Now, suppose script also alters > element A's style attribute, for example. When we do the pre-traversal for > element A, we won't remove it from the hashmap because it only has a > throttled restyle. However, when the do the parallel traversal, presumably > we'll update its animation rule when we apply the changes to its style > attribute. So, at the end the traversal, element A is still in the hashmap > but it has been restyled for animation. How should we update the hashmap in > that case? Just a thought, perhaps we don't need to update the hashmap then. Once we implement transitions, if we ever have non-animation restyles mixed with animation restyles we'll end up doing an animation restyle first where we flush all throttled animations. So perhaps this won't actually be an issue.
(In reply to Brian Birtles (:birtles) from comment #27) > Comment on attachment 8844233 [details] > Bug 1344619 - Part 2: Introduce EffectCompositor::PreTraverse(). > > https://reviewboard.mozilla.org/r/117738/#review119902 > > This looks good but one thing I don't understand is, suppose we have element > A and it has posted a throttled restyle. Now, suppose script also alters > element A's style attribute, for example. When we do the pre-traversal for > element A, we won't remove it from the hashmap because it only has a > throttled restyle. However, when the do the parallel traversal, presumably > we'll update its animation rule when we apply the changes to its style > attribute. So, at the end the traversal, element A is still in the hashmap > but it has been restyled for animation. How should we update the hashmap in > that case? Interesting. The situation is somewhat related to how we handle eRestyle_CSSAnimations. Oh, while I am writing comment, you already commented. You know better than me about this. > ::: dom/animation/EffectCompositor.cpp:958 > (Diff revision 2) > > + if (!effects) { > > + // Drop the effect that has been destroyed. > > + iter.Remove(); > > + continue; > > + } > > Does this ever happen? If so, I'm curious why. If not, perhaps we should add > an assertion here. I haven't checked it but I guessed this is caused by play & cancel at once?
Comment on attachment 8844236 [details] Bug 1344619 - Part 5: Introduce EffectCompositor::PreTraverse() for an element. https://reviewboard.mozilla.org/r/117744/#review119906 This seems good to me but I'm a little unclear on where it is used / why it is needed. When does this method get called? ::: dom/animation/EffectCompositor.h:242 (Diff revision 2) > // Do a bunch of stuff that we should avoid doing during the parallel > // traversal (E.g. changing member variables) for all elements that needs > // to be composed styles. > void PreTraverse(); > > + // Similar to the above but only for the (pseudo-)element. Similar to the above but only for a single (pseudo-)element. ::: dom/animation/EffectCompositor.cpp:976 (Diff revision 2) > } > } > } > > +void > +EffectCompositor::PreTraverse(dom::Element* aElement, nsIAtom* aPseudoTagOrNull) I guess both versions of PreTraverse should assert that they are running on the main thread?
Comment on attachment 8844238 [details] Bug 1344619 - Part 7: Drop EffectCompositor::ClearElementsToRestyle. https://reviewboard.mozilla.org/r/117748/#review119912
Attachment #8844238 - Flags: review?(bbirtles) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #31) > > ::: dom/animation/EffectCompositor.cpp:958 > > (Diff revision 2) > > > + if (!effects) { > > > + // Drop the effect that has been destroyed. > > > + iter.Remove(); > > > + continue; > > > + } > > > > Does this ever happen? If so, I'm curious why. If not, perhaps we should add > > an assertion here. > > I haven't checked it but I guessed this is caused by play & cancel at once? Actually I saw a stack trace (in a security bug?) something like this, but I don't remember exactly.
Comment on attachment 8844232 [details] Bug 1344619 - Part 1: Introduce WillCompose() to update various state that we need to update as the result of ComposeStyle(). https://reviewboard.mozilla.org/r/117736/#review119914 Clearing review on this for now since it seems like you agree we should move the entry point to Animation. I'll double check the patch after that change has been made.
Attachment #8844232 - Flags: review?(bbirtles)
Comment on attachment 8844233 [details] Bug 1344619 - Part 2: Introduce EffectCompositor::PreTraverse(). https://reviewboard.mozilla.org/r/117738/#review119916 Making this r=me with comments addressed since, as per comment 30, I think I've persuaded myself this is ok (or will be ok once we add animation restyles). ::: dom/animation/EffectCompositor.cpp:959 (Diff revision 2) > + > + NonOwningAnimationTarget target = iter.Key(); > + EffectSet* effects = > + EffectSet::GetEffectSet(target.mElement, target.mPseudoType); > + if (!effects) { > + // Drop the effect that has been destroyed. Nit: s/the effect/EffectSets that have/
Attachment #8844233 - Flags: review?(bbirtles) → review+
Comment on attachment 8844236 [details] Bug 1344619 - Part 5: Introduce EffectCompositor::PreTraverse() for an element. https://reviewboard.mozilla.org/r/117744/#review119906 I think this is called when we use getComputedStyle() or some sort of things that cause style flush: var div = document.createElement('div'); document.body.appendChild(div); div.animate({ opacity: [0, 1] }, 1000); getComputedStyle(div).opacity;
Comment on attachment 8844232 [details] Bug 1344619 - Part 1: Introduce WillCompose() to update various state that we need to update as the result of ComposeStyle(). https://reviewboard.mozilla.org/r/117736/#review119924 ::: dom/animation/Animation.h:317 (Diff revision 3) > * running on the compositor). > */ > bool CanThrottle() const; > + > + /** > + * Updates various bits of state that we need to update as the results of s/results of/result of running/
Attachment #8844232 - Flags: review?(bbirtles) → review+
Comment on attachment 8844236 [details] Bug 1344619 - Part 5: Introduce EffectCompositor::PreTraverse() for an element. https://reviewboard.mozilla.org/r/117744/#review119926
Attachment #8844236 - Flags: review?(bbirtles) → review+
Comment on attachment 8844232 [details] Bug 1344619 - Part 1: Introduce WillCompose() to update various state that we need to update as the result of ComposeStyle(). https://reviewboard.mozilla.org/r/117736/#review119942 ::: dom/animation/EffectCompositor.cpp:489 (Diff revision 4) > // priority. > // stylo: we don't support animations on compositor now, so propertiesToSkip > // is an empty set. > const nsCSSPropertyIDSet propertiesToSkip; > for (KeyframeEffectReadOnly* effect : sortedEffectList) { > + effect->GetAnimation()->WillComposeStyle(); I made a horrible mistake here. This function is GetServoAnimationRule(). We have only two test cases to catch this error.
Comment on attachment 8844232 [details] Bug 1344619 - Part 1: Introduce WillCompose() to update various state that we need to update as the result of ComposeStyle(). https://reviewboard.mozilla.org/r/117736/#review119944 ::: dom/animation/Animation.h:318 (Diff revision 5) > */ > bool CanThrottle() const; > + > + /** > + * Updates various bits of state that we need to update as the result of > + * runinning ComposeStyle(). running
(In reply to Hiroyuki Ikezoe (:hiro) from comment #54) > Comment on attachment 8844232 [details] > Bug 1344619 - Part 1: Introduce WillCompose() to update various state that > we need to update as the result of ComposeStyle(). > > https://reviewboard.mozilla.org/r/117736/#review119942 > > ::: dom/animation/EffectCompositor.cpp:489 > (Diff revision 4) > > // priority. > > // stylo: we don't support animations on compositor now, so propertiesToSkip > > // is an empty set. > > const nsCSSPropertyIDSet propertiesToSkip; > > for (KeyframeEffectReadOnly* effect : sortedEffectList) { > > + effect->GetAnimation()->WillComposeStyle(); > > I made a horrible mistake here. This function is GetServoAnimationRule(). > We have only two test cases to catch this error. Oh, nice catch. That's a worry that only two tests failed! I don't like how MozReview makes the function name so small and makes it easy to miss this context (and really hard to copy and paste too: bug 1272215).
(In reply to Brian Birtles (:birtles) from comment #62) > Comment on attachment 8844232 [details] > Bug 1344619 - Part 1: Introduce WillCompose() to update various state that > we need to update as the result of ComposeStyle(). > > https://reviewboard.mozilla.org/r/117736/#review119944 > > ::: dom/animation/Animation.h:318 > (Diff revision 5) > > */ > > bool CanThrottle() const; > > + > > + /** > > + * Updates various bits of state that we need to update as the result of > > + * runinning ComposeStyle(). > > running Horrible!
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 7aa2c65281d6 -d 31179f02c5c3: rebasing 380074:7aa2c65281d6 "Bug 1344619 - Part 1: Introduce WillCompose() to update various state that we need to update as the result of ComposeStyle(). r=birtles" merging dom/animation/KeyframeEffectReadOnly.cpp merging dom/animation/KeyframeEffectReadOnly.h rebasing 380075:ef60069f18b0 "Bug 1344619 - Part 2: Introduce EffectCompositor::PreTraverse(). r=birtles" rebasing 380076:79a88457831d "Bug 1344619 - Part 3: Kick EffectCompositor::PreTraverse(). r=heycam" merging layout/style/ServoStyleSet.cpp merging layout/style/ServoStyleSet.h warning: conflicts while merging layout/style/ServoStyleSet.h! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Oh, dear MozReview. The conflict happens on autoland. Why don't you accept commits based on autoland?
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 282602b50aae -d 7b8d55ab2e91: rebasing 380081:282602b50aae "Bug 1344619 - Part 1: Introduce WillCompose() to update various state that we need to update as the result of ComposeStyle(). r=birtles" merging dom/animation/KeyframeEffectReadOnly.cpp merging dom/animation/KeyframeEffectReadOnly.h rebasing 380082:e9561e527e31 "Bug 1344619 - Part 2: Introduce EffectCompositor::PreTraverse(). r=birtles" rebasing 380083:a37f1c78aad5 "Bug 1344619 - Part 3: Kick EffectCompositor::PreTraverse(). r=heycam" merging layout/style/ServoStyleSet.cpp merging layout/style/ServoStyleSet.h warning: conflicts while merging layout/style/ServoStyleSet.h! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e2a70a374a1c Part 1: Introduce WillCompose() to update various state that we need to update as the result of ComposeStyle(). r=birtles https://hg.mozilla.org/integration/autoland/rev/a59b8f71768e Part 2: Introduce EffectCompositor::PreTraverse(). r=birtles https://hg.mozilla.org/integration/autoland/rev/2a4983f41dd0 Part 3: Kick EffectCompositor::PreTraverse(). r=heycam https://hg.mozilla.org/integration/autoland/rev/3f65c363a271 Part 4: Add ServoStyleSet::ResolveStyleLazily(). r=heycam https://hg.mozilla.org/integration/autoland/rev/826ddb7f87ce Part 5: Introduce EffectCompositor::PreTraverse() for an element. r=birtles https://hg.mozilla.org/integration/autoland/rev/eefdde69034c Part 6: Kick EffectCompositor::PreTraverse() for an element. r=heycam https://hg.mozilla.org/integration/autoland/rev/4efef0367990 Part 7: Drop EffectCompositor::ClearElementsToRestyle. r=birtles
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: