servo: Merge #14418 - layout: Fix some particularly bad cases of spurious reflows leading to script thread unresponsiveness (from pcwalton:infinite-reflows); r=notriddle
authorPatrick Walton <pcwalton@mimiga.net>
Thu, 01 Dec 2016 10:16:38 -0800
changeset 340263 6e75c58b3876e218c27529a3148aa6e7c908b8b8
parent 340262 2ca07971434160711fa690dfe90db46f52032d94
child 340264 347204d95d6ddac7c689b0cf912483c9a58328fe
push id31307
push usergszorc@mozilla.com
push dateSat, 04 Feb 2017 00:59:06 +0000
treeherdermozilla-central@94079d43835f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnotriddle
servo: Merge #14418 - layout: Fix some particularly bad cases of spurious reflows leading to script thread unresponsiveness (from pcwalton:infinite-reflows); r=notriddle See commits for details. This improves nytimes.com quite a bit. r? @notriddle Source-Repo: https://github.com/servo/servo Source-Revision: d2d62267a14891ecc37092a1a931862b9f784019
servo/components/compositing/compositor.rs
servo/components/layout/animation.rs
servo/components/layout_thread/lib.rs
servo/components/style/animation.rs
servo/components/style/matching.rs
servo/components/style/properties/helpers/animated_properties.mako.rs
--- a/servo/components/compositing/compositor.rs
+++ b/servo/components/compositing/compositor.rs
@@ -1186,20 +1186,23 @@ impl<Window: WindowMethods> IOCompositor
         let animation_callbacks_running = self.pipeline_details(pipeline_id).animation_callbacks_running;
         if animation_callbacks_running {
             let msg = ConstellationMsg::TickAnimation(pipeline_id, AnimationTickType::Script);
             if let Err(e) = self.constellation_chan.send(msg) {
                 warn!("Sending tick to constellation failed ({}).", e);
             }
         }
 
-        // We still need to tick layout unfortunately, see things like #12749.
-        let msg = ConstellationMsg::TickAnimation(pipeline_id, AnimationTickType::Layout);
-        if let Err(e) = self.constellation_chan.send(msg) {
-            warn!("Sending tick to constellation failed ({}).", e);
+        // We may need to tick animations in layout. (See #12749.)
+        let animations_running = self.pipeline_details(pipeline_id).animations_running;
+        if animations_running {
+            let msg = ConstellationMsg::TickAnimation(pipeline_id, AnimationTickType::Layout);
+            if let Err(e) = self.constellation_chan.send(msg) {
+                warn!("Sending tick to constellation failed ({}).", e);
+            }
         }
     }
 
     fn constrain_viewport(&mut self, pipeline_id: PipelineId, constraints: ViewportConstraints) {
         let is_root = self.root_pipeline.as_ref().map_or(false, |root_pipeline| {
             root_pipeline.id == pipeline_id
         });
 
--- a/servo/components/layout/animation.rs
+++ b/servo/components/layout/animation.rs
@@ -83,17 +83,18 @@ pub fn update_animation_state(constellat
 
             if still_running {
                 animations_still_running.push(running_animation);
                 continue
             }
 
             if let Animation::Transition(_, unsafe_node, _, ref frame, _) = running_animation {
                 script_chan.send(ConstellationControlMsg::TransitionEnd(unsafe_node,
-                                                                        frame.property_animation.property_name(),
+                                                                        frame.property_animation
+                                                                             .property_name(),
                                                                         frame.duration))
                            .unwrap();
             }
 
             expired_animations.entry(*key)
                               .or_insert_with(Vec::new)
                               .push(running_animation);
         }
@@ -108,26 +109,27 @@ pub fn update_animation_state(constellat
     for key in keys_to_remove {
         running_animations.remove(&key).unwrap();
     }
 
     // Add new running animations.
     for new_running_animation in new_running_animations {
         running_animations.entry(*new_running_animation.node())
                           .or_insert_with(Vec::new)
-                          .push(new_running_animation);
+                          .push(new_running_animation)
     }
 
     let animation_state = if running_animations.is_empty() {
         AnimationState::NoAnimationsPresent
     } else {
         AnimationState::AnimationsPresent
     };
 
-    constellation_chan.send(ConstellationMsg::ChangeRunningAnimationsState(pipeline_id, animation_state))
+    constellation_chan.send(ConstellationMsg::ChangeRunningAnimationsState(pipeline_id,
+                                                                           animation_state))
                       .unwrap();
 }
 
 /// Recalculates style for a set of animations. This does *not* run with the DOM
 /// lock held.
 // NB: This is specific for SelectorImpl, since the layout context and the
 // flows are SelectorImpl specific too. If that goes away at some point,
 // this should be made generic.
--- a/servo/components/layout_thread/lib.rs
+++ b/servo/components/layout_thread/lib.rs
@@ -1088,17 +1088,17 @@ impl LayoutThread {
         }
         if needs_reflow {
             if let Some(mut flow) = self.try_get_layout_root(element.as_node()) {
                 LayoutThread::reflow_all_nodes(FlowRef::deref_mut(&mut flow));
             }
         }
 
         let restyles = document.drain_pending_restyles();
-        debug!("Draining restyles: {}", restyles.len());
+        debug!("Draining restyles: {} (needs dirtying? {:?})", restyles.len(), needs_dirtying);
         if !needs_dirtying {
             for (el, restyle) in restyles {
                 // Propagate the descendant bit up the ancestors. Do this before
                 // the restyle calculation so that we can also do it for new
                 // unstyled nodes, which the descendants bit helps us find.
                 if let Some(parent) = el.parent_element() {
                     unsafe { parent.note_dirty_descendant() };
                 }
@@ -1279,16 +1279,20 @@ impl LayoutThread {
     }
 
     fn tick_all_animations<'a, 'b>(&mut self, possibly_locked_rw_data: &mut RwData<'a, 'b>) {
         let mut rw_data = possibly_locked_rw_data.lock();
         self.tick_animations(&mut rw_data);
     }
 
     fn tick_animations(&mut self, rw_data: &mut LayoutThreadData) {
+        if opts::get().relayout_event {
+            println!("**** pipeline={}\tForDisplay\tSpecial\tAnimationTick", self.id);
+        }
+
         let reflow_info = Reflow {
             goal: ReflowGoal::ForDisplay,
             page_clip_rect: max_rect(),
         };
 
         let mut layout_context = self.build_shared_layout_context(&*rw_data,
                                                                   false,
                                                                   reflow_info.goal);
--- a/servo/components/style/animation.rs
+++ b/servo/components/style/animation.rs
@@ -330,36 +330,54 @@ impl PropertyAnimation {
 
         self.property.update(style, progress);
     }
 
     #[inline]
     fn does_animate(&self) -> bool {
         self.property.does_animate() && self.duration != Time(0.0)
     }
+
+    #[inline]
+    pub fn has_the_same_end_value_as(&self, other: &PropertyAnimation) -> bool {
+        self.property.has_the_same_end_value_as(&other.property)
+    }
 }
 
 /// Inserts transitions into the queue of running animations as applicable for
 /// the given style difference. This is called from the layout worker threads.
 /// Returns true if any animations were kicked off and false otherwise.
 //
 // TODO(emilio): Take rid of this mutex splitting SharedLayoutContex into a
 // cloneable part and a non-cloneable part..
 pub fn start_transitions_if_applicable(new_animations_sender: &Sender<Animation>,
                                        opaque_node: OpaqueNode,
                                        unsafe_node: UnsafeNode,
                                        old_style: &ComputedValues,
                                        new_style: &mut Arc<ComputedValues>,
-                                       timer: &Timer)
+                                       timer: &Timer,
+                                       possibly_expired_animations: &[PropertyAnimation])
                                        -> bool {
     let mut had_animations = false;
     for i in 0..new_style.get_box().transition_property_count() {
         // Create any property animations, if applicable.
-        let property_animations = PropertyAnimation::from_transition(i, old_style, Arc::make_mut(new_style));
+        let property_animations = PropertyAnimation::from_transition(i,
+                                                                     old_style,
+                                                                     Arc::make_mut(new_style));
         for property_animation in property_animations {
+            // Per [1], don't trigger a new transition if the end state for that transition is
+            // the same as that of a transition that's already running on the same node.
+            //
+            // [1]: https://drafts.csswg.org/css-transitions/#starting
+            if possibly_expired_animations.iter().any(|animation| {
+                    animation.has_the_same_end_value_as(&property_animation)
+               }) {
+                continue
+            }
+
             // Set the property to the initial value.
             // NB: get_mut is guaranteed to succeed since we called make_mut()
             // above.
             property_animation.update(Arc::get_mut(new_style).unwrap(), 0.0);
 
             // Kick off the animation.
             let box_style = new_style.get_box();
             let now = timer.seconds();
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -2,17 +2,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 //! High-level interface to CSS selector matching.
 
 #![allow(unsafe_code)]
 
 use {Atom, LocalName};
-use animation;
+use animation::{self, Animation, PropertyAnimation};
 use atomic_refcell::AtomicRefMut;
 use cache::LRUCache;
 use cascade_info::CascadeInfo;
 use context::{SharedStyleContext, StyleContext};
 use data::{ComputedStyle, ElementData, ElementStyles, PseudoStyles};
 use dom::{TElement, TNode, TRestyleDamage, UnsafeNode};
 use properties::{CascadeFlags, ComputedValues, SHAREABLE, cascade};
 use properties::longhands::display::computed_value as display;
@@ -390,20 +390,20 @@ trait PrivateMatchMethods: TElement {
     ///
     /// Note that animations only apply to nodes or ::before or ::after
     /// pseudo-elements.
     fn cascade_node_pseudo_element<'a, Ctx>(&self,
                                             context: &Ctx,
                                             parent_style: Option<&Arc<ComputedValues>>,
                                             old_style: Option<&Arc<ComputedValues>>,
                                             rule_node: &StrongRuleNode,
+                                            possibly_expired_animations: &[PropertyAnimation],
                                             booleans: CascadeBooleans)
                                             -> Arc<ComputedValues>
-        where Ctx: StyleContext<'a>
-    {
+                                            where Ctx: StyleContext<'a> {
         let shared_context = context.shared_context();
         let mut cascade_info = CascadeInfo::new();
         let mut cascade_flags = CascadeFlags::empty();
         if booleans.shareable {
             cascade_flags.insert(SHAREABLE)
         }
 
         let this_style = match parent_style {
@@ -440,60 +440,61 @@ trait PrivateMatchMethods: TElement {
             // to its old value if it did trigger a transition.
             if let Some(ref style) = old_style {
                 animation::start_transitions_if_applicable(
                     new_animations_sender,
                     this_opaque,
                     self.as_node().to_unsafe(),
                     &**style,
                     &mut this_style,
-                    &shared_context.timer);
+                    &shared_context.timer,
+                    &possibly_expired_animations);
             }
         }
 
         this_style
     }
 
     fn update_animations_for_cascade(&self,
                                      context: &SharedStyleContext,
-                                     style: &mut Arc<ComputedValues>) -> bool {
+                                     style: &mut Arc<ComputedValues>,
+                                     possibly_expired_animations: &mut Vec<PropertyAnimation>) {
         // Finish any expired transitions.
         let this_opaque = self.as_node().opaque();
-        let had_animations_to_expire =
-            animation::complete_expired_transitions(this_opaque, style, context);
+        animation::complete_expired_transitions(this_opaque, style, context);
 
         // Merge any running transitions into the current style, and cancel them.
         let had_running_animations = context.running_animations
                                             .read()
                                             .get(&this_opaque)
                                             .is_some();
         if had_running_animations {
             let mut all_running_animations = context.running_animations.write();
-            for mut running_animation in all_running_animations.get_mut(&this_opaque).unwrap() {
+            for running_animation in all_running_animations.get_mut(&this_opaque).unwrap() {
                 // This shouldn't happen frequently, but under some
                 // circumstances mainly huge load or debug builds, the
                 // constellation might be delayed in sending the
                 // `TickAllAnimations` message to layout.
                 //
                 // Thus, we can't assume all the animations have been already
                 // updated by layout, because other restyle due to script might
                 // be triggered by layout before the animation tick.
                 //
                 // See #12171 and the associated PR for an example where this
                 // happened while debugging other release panic.
                 if !running_animation.is_expired() {
                     animation::update_style_for_animation(context,
                                                           running_animation,
                                                           style);
-                    running_animation.mark_as_expired();
+                    if let Animation::Transition(_, _, _, ref frame, _) = *running_animation {
+                        possibly_expired_animations.push(frame.property_animation.clone())
+                    }
                 }
             }
         }
-
-        had_animations_to_expire || had_running_animations
     }
 
     fn share_style_with_candidate_if_possible(&self,
                                               shared_context: &SharedStyleContext,
                                               candidate: &mut StyleSharingCandidate)
                                               -> Result<ComputedStyle, CacheMiss> {
         let candidate_element = unsafe {
             Self::ConcreteNode::from_unsafe(&candidate.node).as_element().unwrap()
@@ -724,69 +725,75 @@ pub trait MatchMethods : TElement {
                                     primary_is_shareable: bool)
         where Ctx: StyleContext<'a>
     {
         // Get our parent's style.
         let parent_data = parent.as_ref().map(|x| x.borrow_data().unwrap());
         let parent_style = parent_data.as_ref().map(|x| &x.current_styles().primary.values);
 
         let mut new_styles;
+        let mut possibly_expired_animations = vec![];
 
         let damage = {
             let (old_primary, old_pseudos) = match data.previous_styles_mut() {
                 None => (None, None),
                 Some(previous) => {
                     // Update animations before the cascade. This may modify the
                     // value of the old primary style.
                     self.update_animations_for_cascade(context.shared_context(),
-                                                       &mut previous.primary.values);
+                                                       &mut previous.primary.values,
+                                                       &mut possibly_expired_animations);
                     (Some(&previous.primary.values), Some(&mut previous.pseudos))
                 }
             };
 
             let new_style =
                 self.cascade_node_pseudo_element(context,
                                                  parent_style,
                                                  old_primary,
                                                  &primary_rule_node,
+                                                 &possibly_expired_animations,
                                                  CascadeBooleans {
                                                      shareable: primary_is_shareable,
                                                      animate: true,
                                                  });
 
             let primary = ComputedStyle::new(primary_rule_node, new_style);
             new_styles = ElementStyles::new(primary);
 
             let damage =
                 self.compute_damage_and_cascade_pseudos(old_primary,
                                                         old_pseudos,
                                                         &new_styles.primary.values,
                                                         &mut new_styles.pseudos,
                                                         context,
-                                                        pseudo_rule_nodes);
+                                                        pseudo_rule_nodes,
+                                                        &mut possibly_expired_animations);
 
             self.as_node().set_can_be_fragmented(parent.map_or(false, |p| {
                 p.as_node().can_be_fragmented() ||
                 parent_style.unwrap().is_multicol()
             }));
 
             damage
         };
 
         data.finish_styling(new_styles, damage);
     }
 
-    fn compute_damage_and_cascade_pseudos<'a, Ctx>(&self,
-                                                   old_primary: Option<&Arc<ComputedValues>>,
-                                                   mut old_pseudos: Option<&mut PseudoStyles>,
-                                                   new_primary: &Arc<ComputedValues>,
-                                                   new_pseudos: &mut PseudoStyles,
-                                                   context: &Ctx,
-                                                   mut pseudo_rule_nodes: PseudoRuleNodes)
-                                                   -> RestyleDamage
+    fn compute_damage_and_cascade_pseudos<'a, Ctx>(
+            &self,
+            old_primary: Option<&Arc<ComputedValues>>,
+            mut old_pseudos: Option<&mut PseudoStyles>,
+            new_primary: &Arc<ComputedValues>,
+            new_pseudos: &mut PseudoStyles,
+            context: &Ctx,
+            mut pseudo_rule_nodes: PseudoRuleNodes,
+            possibly_expired_animations: &mut Vec<PropertyAnimation>)
+            -> RestyleDamage
         where Ctx: StyleContext<'a>
     {
         // Here we optimise the case of the style changing but both the
         // previous and the new styles having display: none. In this
         // case, we can always optimize the traversal, regardless of the
         // restyle hint.
         let this_display = new_primary.get_box().clone_display();
         if this_display == display::T::none {
@@ -833,24 +840,28 @@ pub trait MatchMethods : TElement {
 
                 // We have declarations, so we need to cascade. Compute parameters.
                 let animate = <Self as MatchAttr>::Impl::pseudo_is_before_or_after(&pseudo);
                 if animate {
                     if let Some(ref mut old_pseudo_style) = maybe_old_pseudo_style {
                         // Update animations before the cascade. This may modify
                         // the value of old_pseudo_style.
                         self.update_animations_for_cascade(context.shared_context(),
-                                                           &mut old_pseudo_style.values);
+                                                           &mut old_pseudo_style.values,
+                                                           possibly_expired_animations);
                     }
                 }
 
                 let new_pseudo_values =
-                    self.cascade_node_pseudo_element(context, Some(new_primary),
-                                                     maybe_old_pseudo_style.as_ref().map(|s| &s.values),
+                    self.cascade_node_pseudo_element(context,
+                                                     Some(new_primary),
+                                                     maybe_old_pseudo_style.as_ref()
+                                                                           .map(|s| &s.values),
                                                      &new_rule_node,
+                                                     &possibly_expired_animations,
                                                      CascadeBooleans {
                                                          shareable: false,
                                                          animate: animate,
                                                      });
 
                 // Compute restyle damage unless we've already maxed it out.
                 if damage != rebuild_and_reflow {
                     damage = damage | match maybe_old_pseudo_style {
--- a/servo/components/style/properties/helpers/animated_properties.mako.rs
+++ b/servo/components/style/properties/helpers/animated_properties.mako.rs
@@ -119,16 +119,30 @@ impl AnimatedProperty {
             % for prop in data.longhands:
                 % if prop.animatable:
                     AnimatedProperty::${prop.camel_case}(ref from, ref to) => from != to,
                 % endif
             % endfor
         }
     }
 
+    pub fn has_the_same_end_value_as(&self, other: &AnimatedProperty) -> bool {
+        match (self, other) {
+            % for prop in data.longhands:
+                % if prop.animatable:
+                    (&AnimatedProperty::${prop.camel_case}(_, ref this_end_value),
+                     &AnimatedProperty::${prop.camel_case}(_, ref other_end_value)) => {
+                        this_end_value == other_end_value
+                    }
+                % endif
+            % endfor
+            _ => false,
+        }
+    }
+
     pub fn update(&self, style: &mut ComputedValues, progress: f64) {
         match *self {
             % for prop in data.longhands:
                 % if prop.animatable:
                     AnimatedProperty::${prop.camel_case}(ref from, ref to) => {
                         if let Ok(value) = from.interpolate(to, progress) {
                             style.mutate_${prop.style_struct.ident.strip("_")}().set_${prop.ident}(value);
                         }