Bug 1550554 - Use an ArcSlice as the computed value representation of inherited list properties. r=heycam
☠☠ backed out by 283b94c196a1 ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 07 May 2019 18:46:22 +0200
changeset 474072 57f2362aa538b3374290652fe8605d05104c140f
parent 474071 45f171b26e95c6c7d12302743129ec5ccf5ac7e4
child 474073 2c31fe18eefd0a53bac3dec0c752fcc156d001e9
push id36022
push userncsoregi@mozilla.com
push dateThu, 16 May 2019 21:55:16 +0000
treeherdermozilla-central@96802be91766 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1550554
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1550554 - Use an ArcSlice as the computed value representation of inherited list properties. r=heycam This adds a bit of complexity, which I think will pay off in the end. Removals incoming. Differential Revision: https://phabricator.services.mozilla.com/D30544
servo/components/style/properties/gecko.mako.rs
servo/components/style/properties/helpers.mako.rs
servo/components/style/properties/longhands/inherited_svg.mako.rs
servo/components/style_traits/owned_slice.rs
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -3806,19 +3806,20 @@ fn static_assert() {
     pub fn copy_text_shadow_from(&mut self, other: &Self) {
         self.gecko.mTextShadow.copy_from(&other.gecko.mTextShadow);
     }
 
     pub fn reset_text_shadow(&mut self, other: &Self) {
         self.copy_text_shadow_from(other)
     }
 
+    // FIXME(emilio): Remove by sharing representation.
     pub fn clone_text_shadow(&self) -> longhands::text_shadow::computed_value::T {
-        let buf = self.gecko.mTextShadow.iter().map(|v| v.to_simple_shadow()).collect::<Vec<_>>();
-        longhands::text_shadow::computed_value::List(buf.into())
+        let iter = self.gecko.mTextShadow.iter().map(|v| v.to_simple_shadow());
+        longhands::text_shadow::computed_value::List(crate::ArcSlice::from_iter(iter))
     }
 
     fn clear_text_emphasis_style_if_string(&mut self) {
         if self.gecko.mTextEmphasisStyle == structs::NS_STYLE_TEXT_EMPHASIS_STYLE_STRING as u8 {
             self.gecko.mTextEmphasisStyleString.truncate();
             self.gecko.mTextEmphasisStyle = structs::NS_STYLE_TEXT_EMPHASIS_STYLE_NONE as u8;
         }
     }
@@ -4180,26 +4181,24 @@ clip-path
         SVGStrokeDashArray::Values(self.gecko.mStrokeDasharray.iter().cloned().collect())
     }
 
     #[allow(non_snake_case)]
     pub fn _moz_context_properties_count(&self) -> usize {
         self.gecko.mContextProps.len()
     }
 
+    // FIXME(emilio): Remove by sharing representation.
     #[allow(non_snake_case)]
-    pub fn _moz_context_properties_at(
-        &self,
-        index: usize,
-    ) -> longhands::_moz_context_properties::computed_value::single_value::T {
-        longhands::_moz_context_properties::computed_value::single_value::T(
-            CustomIdent(unsafe {
-                Atom::from_raw(self.gecko.mContextProps[index].mRawPtr)
-            })
-        )
+    pub fn clone__moz_context_properties(&self) -> longhands::_moz_context_properties::computed_value::T {
+        use crate::values::specified::svg::MozContextProperties;
+        let buf = self.gecko.mContextProps.iter().map(|v| {
+            MozContextProperties(CustomIdent(unsafe { Atom::from_raw(v.mRawPtr) }))
+        }).collect::<Vec<_>>();
+        longhands::_moz_context_properties::computed_value::List(crate::ArcSlice::from_iter(buf.into_iter()))
     }
 
     #[allow(non_snake_case)]
     pub fn set__moz_context_properties<I>(&mut self, v: I)
     where
         I: IntoIterator<Item = longhands::_moz_context_properties::computed_value::single_value::T>,
         I::IntoIter: ExactSizeIterator
     {
--- a/servo/components/style/properties/helpers.mako.rs
+++ b/servo/components/style/properties/helpers.mako.rs
@@ -75,16 +75,26 @@
 <%doc>
     To be used in cases where we have a grammar like "<thing> [ , <thing> ]*".
 
     Setting allow_empty to False allows for cases where the vector
     is empty. The grammar for these is usually "none | <thing> [ , <thing> ]*".
     We assume that the default/initial value is an empty vector for these.
     `initial_value` need not be defined for these.
 </%doc>
+
+// The setup here is roughly:
+//
+//  * UnderlyingList is the list that is stored in the computed value. This may
+//    be a shared ArcSlice if the property is inherited.
+//  * UnderlyingOwnedList is the list that is used for animation.
+//  * Specified values always use OwnedSlice, since it's more compact.
+//  * computed_value::List is just a convenient alias that you can use for the
+//    computed value list, since this is in the computed_value module.
+//
 <%def name="vector_longhand(name, animation_value_type=None,
                             vector_animation_type=None, allow_empty=False,
                             separator='Comma',
                             **kwargs)">
     <%call expr="longhand(name, animation_value_type=animation_value_type, vector=True,
                           **kwargs)">
         #[allow(unused_imports)]
         use smallvec::SmallVec;
@@ -106,98 +116,192 @@
             use crate::values::{computed, specified};
             #[allow(unused_imports)]
             use crate::values::{Auto, Either, None_};
             ${caller.body()}
         }
 
         /// The definition of the computed value for ${name}.
         pub mod computed_value {
+            #[allow(unused_imports)]
+            use crate::values::animated::ToAnimatedValue;
+            #[allow(unused_imports)]
+            use crate::values::resolved::ToResolvedValue;
             pub use super::single_value::computed_value as single_value;
             pub use self::single_value::T as SingleComputedValue;
             % if not allow_empty or allow_empty == "NotInitial":
             use smallvec::SmallVec;
             % endif
             use crate::values::computed::ComputedVecIter;
 
-            /// The generic type defining the value for this property.
+            <% is_shared_list = allow_empty and allow_empty != "NotInitial" and data.longhands_by_name[name].style_struct.inherited %>
+
+            // FIXME(emilio): Add an OwnedNonEmptySlice type, and figure out
+            // something for transition-name, which is the only remaining user
+            // of NotInitial.
+            pub type UnderlyingList<T> =
+                % if allow_empty and allow_empty != "NotInitial":
+                % if data.longhands_by_name[name].style_struct.inherited:
+                    crate::ArcSlice<T>;
+                % else:
+                    crate::OwnedSlice<T>;
+                % endif
+                % else:
+                    SmallVec<[T; 1]>;
+                % endif
+
+            pub type UnderlyingOwnedList<T> =
+                % if allow_empty and allow_empty != "NotInitial":
+                    crate::OwnedSlice<T>;
+                % else:
+                    SmallVec<[T; 1]>;
+                % endif
+
+
+            /// The generic type defining the animated and resolved values for
+            /// this property.
             ///
             /// Making this type generic allows the compiler to figure out the
             /// animated value for us, instead of having to implement it
             /// manually for every type we care about.
             % if separator == "Comma":
             #[css(comma)]
             % endif
             #[derive(
                 Clone,
                 Debug,
                 MallocSizeOf,
                 PartialEq,
                 ToAnimatedValue,
                 ToResolvedValue,
                 ToCss,
             )]
-            pub struct List<T>(
+            pub struct OwnedList<T>(
                 % if not allow_empty:
                 #[css(iterable)]
                 % else:
                 #[css(if_empty = "none", iterable)]
                 % endif
-                % if allow_empty and allow_empty != "NotInitial":
-                pub crate::OwnedSlice<T>,
+                pub UnderlyingOwnedList<T>,
+            );
+
+            /// The computed value for this property.
+            % if not is_shared_list:
+            pub type ComputedList = OwnedList<single_value::T>;
+            pub use self::OwnedList as List;
+            % else:
+            pub use self::ComputedList as List;
+
+            % if separator == "Comma":
+            #[css(comma)]
+            % endif
+            #[derive(
+                Clone,
+                Debug,
+                MallocSizeOf,
+                PartialEq,
+                ToCss,
+            )]
+            pub struct ComputedList(
+                % if not allow_empty:
+                #[css(iterable)]
                 % else:
-                // FIXME(emilio): Add an OwnedNonEmptySlice type, and figure out
-                // something for transition-name, which is the only remaining
-                // user of NotInitial.
-                pub SmallVec<[T; 1]>,
+                #[css(if_empty = "none", iterable)]
+                % endif
+                % if is_shared_list:
+                #[ignore_malloc_size_of = "Arc"]
                 % endif
+                pub UnderlyingList<single_value::T>,
             );
 
+            type ResolvedList = OwnedList<<single_value::T as ToResolvedValue>::ResolvedValue>;
+            impl ToResolvedValue for ComputedList {
+                type ResolvedValue = ResolvedList;
+
+                fn to_resolved_value(self, context: &crate::values::resolved::Context) -> Self::ResolvedValue {
+                    OwnedList(
+                        self.0
+                            .iter()
+                            .cloned()
+                            .map(|v| v.to_resolved_value(context))
+                            .collect()
+                    )
+                }
+
+                fn from_resolved_value(resolved: Self::ResolvedValue) -> Self {
+                    % if not is_shared_list:
+                    use std::iter::FromIterator;
+                    % endif
+                    let iter =
+                        resolved.0.into_iter().map(ToResolvedValue::from_resolved_value);
+                    ComputedList(UnderlyingList::from_iter(iter))
+                }
+            }
+            % endif
+
 
             % if vector_animation_type:
             % if not animation_value_type:
                 Sorry, this is stupid but needed for now.
             % endif
 
             use crate::properties::animated_properties::ListAnimation;
-            use crate::values::animated::{Animate, ToAnimatedValue, ToAnimatedZero, Procedure};
+            use crate::values::animated::{Animate, ToAnimatedZero, Procedure};
             use crate::values::distance::{SquaredDistance, ComputeSquaredDistance};
 
             // FIXME(emilio): For some reason rust thinks that this alias is
             // unused, even though it's clearly used below?
             #[allow(unused)]
-            type AnimatedList = <List<single_value::T> as ToAnimatedValue>::AnimatedValue;
+            type AnimatedList = OwnedList<<single_value::T as ToAnimatedValue>::AnimatedValue>;
+
+            % if is_shared_list:
+            impl ToAnimatedValue for ComputedList {
+                type AnimatedValue = AnimatedList;
+
+                fn to_animated_value(self) -> Self::AnimatedValue {
+                    OwnedList(
+                        self.0.iter().map(|v| v.clone().to_animated_value()).collect()
+                    )
+                }
+
+                fn from_animated_value(animated: Self::AnimatedValue) -> Self {
+                    let iter =
+                        animated.0.into_iter().map(ToAnimatedValue::from_animated_value);
+                    ComputedList(UnderlyingList::from_iter(iter))
+                }
+            }
+            % endif
 
             impl ToAnimatedZero for AnimatedList {
                 fn to_animated_zero(&self) -> Result<Self, ()> { Err(()) }
             }
 
             impl Animate for AnimatedList {
                 fn animate(
                     &self,
                     other: &Self,
                     procedure: Procedure,
                 ) -> Result<Self, ()> {
-                    Ok(List(
+                    Ok(OwnedList(
                         self.0.animate_${vector_animation_type}(&other.0, procedure)?
                     ))
                 }
             }
             impl ComputeSquaredDistance for AnimatedList {
                 fn compute_squared_distance(
                     &self,
                     other: &Self,
                 ) -> Result<SquaredDistance, ()> {
                     self.0.squared_distance_${vector_animation_type}(&other.0)
                 }
             }
             % endif
 
             /// The computed value, effectively a list of single values.
-            pub type T = List<single_value::T>;
+            pub use self::ComputedList as T;
 
             pub type Iter<'a, 'cx, 'cx_a> = ComputedVecIter<'a, 'cx, 'cx_a, super::single_value::SpecifiedValue>;
         }
 
         /// The specified value of ${name}.
         % if separator == "Comma":
         #[css(comma)]
         % endif
@@ -250,17 +354,22 @@
             }
         }
 
         impl ToComputedValue for SpecifiedValue {
             type ComputedValue = computed_value::T;
 
             #[inline]
             fn to_computed_value(&self, context: &Context) -> computed_value::T {
-                computed_value::List(self.0.iter().map(|i| i.to_computed_value(context)).collect())
+                % if not is_shared_list:
+                use std::iter::FromIterator;
+                % endif
+                computed_value::List(computed_value::UnderlyingList::from_iter(
+                    self.0.iter().map(|i| i.to_computed_value(context))
+                ))
             }
 
             #[inline]
             fn from_computed_value(computed: &computed_value::T) -> Self {
                 let iter = computed.0.iter().map(ToComputedValue::from_computed_value);
                 SpecifiedValue(iter.collect())
             }
         }
--- a/servo/components/style/properties/longhands/inherited_svg.mako.rs
+++ b/servo/components/style/properties/longhands/inherited_svg.mako.rs
@@ -191,14 +191,13 @@
     spec="https://www.w3.org/TR/SVG2/painting.html#PaintOrder",
 )}
 
 ${helpers.predefined_type(
     "-moz-context-properties",
     "MozContextProperties",
     initial_value=None,
     vector=True,
-    need_index=True,
     animation_value_type="none",
     products="gecko",
     spec="Nonstandard (https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-context-properties)",
     allow_empty=True,
 )}
--- a/servo/components/style_traits/owned_slice.rs
+++ b/servo/components/style_traits/owned_slice.rs
@@ -88,17 +88,17 @@ impl<T: Sized> OwnedSlice<T> {
     pub fn into_vec(self) -> Vec<T> {
         let ret = unsafe { Vec::from_raw_parts(self.ptr.as_ptr(), self.len, self.len) };
         mem::forget(self);
         ret
     }
 
     /// Iterate over all the elements in the slice taking ownership of them.
     #[inline]
-    pub fn into_iter(self) -> impl Iterator<Item = T> {
+    pub fn into_iter(self) -> impl Iterator<Item = T> + ExactSizeIterator {
         self.into_vec().into_iter()
     }
 
     /// Convert the regular slice into an owned slice.
     #[inline]
     pub fn from_slice(s: &[T]) -> Self
     where
         T: Clone,