From 484cf5771b372d611368652e3e25e25aa52dfb2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 22 Nov 2020 01:02:22 +0000 Subject: [PATCH] style: Avoid some allocations in selector serialization. The allocations in display_to_css_identifier show up in the profiles of bug 1675628. Differential Revision: https://phabricator.services.mozilla.com/D97856 --- components/selectors/parser.rs | 96 +++++++++++++++++----------------- components/selectors/tree.rs | 8 +-- 2 files changed, 51 insertions(+), 53 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 955a428429b..70f66419250 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -10,15 +10,15 @@ use crate::builder::{SelectorBuilder, SelectorFlags, SpecificityAndFlags}; use crate::context::QuirksMode; use crate::sink::Push; pub use crate::visitor::SelectorVisitor; -use cssparser::{parse_nth, serialize_identifier}; +use cssparser::parse_nth; use cssparser::{BasicParseError, BasicParseErrorKind, ParseError, ParseErrorKind}; use cssparser::{CowRcStr, Delimiter, SourceLocation}; -use cssparser::{CssStringWriter, Parser as CssParser, ToCss, Token}; +use cssparser::{Parser as CssParser, ToCss, Token}; use precomputed_hash::PrecomputedHash; use servo_arc::ThinArc; use smallvec::SmallVec; use std::borrow::{Borrow, Cow}; -use std::fmt::{self, Debug, Display, Write}; +use std::fmt::{self, Debug}; use std::iter::Rev; use std::slice; @@ -197,8 +197,6 @@ macro_rules! with_all_bounds { type ExtraMatchingData: Sized + Default + 'static; type AttrValue: $($InSelector)*; type Identifier: $($InSelector)*; - type ClassName: $($InSelector)*; - type PartName: $($InSelector)*; type LocalName: $($InSelector)* + Borrow; type NamespaceUrl: $($CommonBounds)* + Default + Borrow; type NamespacePrefix: $($InSelector)* + Default; @@ -218,7 +216,7 @@ macro_rules! with_all_bounds { macro_rules! with_bounds { ( [ $( $CommonBounds: tt )* ] [ $( $FromStr: tt )* ]) => { with_all_bounds! { - [$($CommonBounds)* + $($FromStr)* + Display] + [$($CommonBounds)* + $($FromStr)* + ToCss] [$($CommonBounds)*] [$($FromStr)*] } @@ -462,7 +460,6 @@ fn collect_ancestor_hashes( ) -> bool where Impl::Identifier: PrecomputedHash, - Impl::ClassName: PrecomputedHash, Impl::LocalName: PrecomputedHash, Impl::NamespaceUrl: PrecomputedHash, { @@ -514,7 +511,6 @@ impl AncestorHashes { pub fn new(selector: &Selector, quirks_mode: QuirksMode) -> Self where Impl::Identifier: PrecomputedHash, - Impl::ClassName: PrecomputedHash, Impl::LocalName: PrecomputedHash, Impl::NamespaceUrl: PrecomputedHash, { @@ -593,7 +589,7 @@ impl Selector { } #[inline] - pub fn parts(&self) -> Option<&[Impl::PartName]> { + pub fn parts(&self) -> Option<&[Impl::Identifier]> { if !self.is_part() { return None; } @@ -1012,7 +1008,7 @@ pub enum Component { LocalName(LocalName), ID(#[shmem(field_bound)] Impl::Identifier), - Class(#[shmem(field_bound)] Impl::ClassName), + Class(#[shmem(field_bound)] Impl::Identifier), AttributeInNoNamespaceExists { #[shmem(field_bound)] @@ -1061,7 +1057,7 @@ pub enum Component { Slotted(Selector), /// The `::part` pseudo-element. /// https://drafts.csswg.org/css-shadow-parts/#part - Part(#[shmem(field_bound)] Box<[Impl::PartName]>), + Part(#[shmem(field_bound)] Box<[Impl::Identifier]>), /// The `:host` pseudo-class: /// /// https://drafts.csswg.org/css-scoping/#host-selector @@ -1470,18 +1466,18 @@ impl ToCss for Component { if i != 0 { dest.write_char(' ')?; } - display_to_css_identifier(name, dest)?; + name.to_css(dest)?; } dest.write_char(')') }, PseudoElement(ref p) => p.to_css(dest), ID(ref s) => { dest.write_char('#')?; - display_to_css_identifier(s, dest) + s.to_css(dest) }, Class(ref s) => { dest.write_char('.')?; - display_to_css_identifier(s, dest) + s.to_css(dest) }, LocalName(ref s) => s.to_css(dest), ExplicitUniversalType => dest.write_char('*'), @@ -1490,13 +1486,13 @@ impl ToCss for Component { ExplicitNoNamespace => dest.write_char('|'), ExplicitAnyNamespace => dest.write_str("*|"), Namespace(ref prefix, _) => { - display_to_css_identifier(prefix, dest)?; + prefix.to_css(dest)?; dest.write_char('|') }, AttributeInNoNamespaceExists { ref local_name, .. } => { dest.write_char('[')?; - display_to_css_identifier(local_name, dest)?; + local_name.to_css(dest)?; dest.write_char(']') }, AttributeInNoNamespace { @@ -1507,10 +1503,10 @@ impl ToCss for Component { .. } => { dest.write_char('[')?; - display_to_css_identifier(local_name, dest)?; + local_name.to_css(dest)?; operator.to_css(dest)?; dest.write_char('"')?; - write!(CssStringWriter::new(dest), "{}", value)?; + value.to_css(dest)?; dest.write_char('"')?; match case_sensitivity { ParsedCaseSensitivity::CaseSensitive | @@ -1582,13 +1578,13 @@ impl ToCss for AttrSelectorWithOptionalNamespace { dest.write_char('[')?; match self.namespace { Some(NamespaceConstraint::Specific((ref prefix, _))) => { - display_to_css_identifier(prefix, dest)?; + prefix.to_css(dest)?; dest.write_char('|')? }, Some(NamespaceConstraint::Any) => dest.write_str("*|")?, None => {}, } - display_to_css_identifier(&self.local_name, dest)?; + self.local_name.to_css(dest)?; match self.operation { ParsedAttrSelectorOperation::Exists => {}, ParsedAttrSelectorOperation::WithValue { @@ -1598,7 +1594,7 @@ impl ToCss for AttrSelectorWithOptionalNamespace { } => { operator.to_css(dest)?; dest.write_char('"')?; - write!(CssStringWriter::new(dest), "{}", expected_value)?; + expected_value.to_css(dest)?; dest.write_char('"')?; match case_sensitivity { ParsedCaseSensitivity::CaseSensitive | @@ -1617,29 +1613,10 @@ impl ToCss for LocalName { where W: fmt::Write, { - display_to_css_identifier(&self.name, dest) + self.name.to_css(dest) } } -/// Serialize the output of Display as a CSS identifier -fn display_to_css_identifier(x: &T, dest: &mut W) -> fmt::Result { - // FIXME(SimonSapin): it is possible to avoid this heap allocation - // by creating a stream adapter like cssparser::CssStringWriter - // that holds and writes to `&mut W` and itself implements `fmt::Write`. - // - // I haven’t done this yet because it would require somewhat complex and fragile state machine - // to support in `fmt::Write::write_char` cases that, - // in `serialize_identifier` (which has the full value as a `&str` slice), - // can be expressed as - // `string.starts_with("--")`, `string == "-"`, `string.starts_with("-")`, etc. - // - // And I don’t even know if this would be a performance win: jemalloc is good at what it does - // and the state machine might be slower than `serialize_identifier` as currently written. - let string = x.to_string(); - - serialize_identifier(&string, dest) -} - /// Build up a Selector. /// selector : simple_selector_sequence [ combinator simple_selector_sequence ]* ; /// @@ -1810,7 +1787,7 @@ enum SimpleSelectorParseResult { SimpleSelector(Component), PseudoElement(Impl::PseudoElement), SlottedPseudo(Selector), - PartPseudo(Box<[Impl::PartName]>), + PartPseudo(Box<[Impl::Identifier]>), } #[derive(Debug)] @@ -2605,10 +2582,8 @@ pub mod tests { impl SelectorImpl for DummySelectorImpl { type ExtraMatchingData = (); - type AttrValue = DummyAtom; + type AttrValue = DummyAttrValue; type Identifier = DummyAtom; - type ClassName = DummyAtom; - type PartName = DummyAtom; type LocalName = DummyAtom; type NamespaceUrl = DummyAtom; type NamespacePrefix = DummyAtom; @@ -2618,12 +2593,35 @@ pub mod tests { type PseudoElement = PseudoElement; } + #[derive(Clone, Debug, Default, Eq, Hash, PartialEq)] + pub struct DummyAttrValue(String); + + impl ToCss for DummyAttrValue { + fn to_css(&self, dest: &mut W) -> fmt::Result + where + W: fmt::Write, + { + use std::fmt::Write; + + write!(cssparser::CssStringWriter::new(dest), "{}", &self.0) + } + } + + impl<'a> From<&'a str> for DummyAttrValue { + fn from(string: &'a str) -> Self { + Self(string.into()) + } + } + #[derive(Clone, Debug, Default, Eq, Hash, PartialEq)] pub struct DummyAtom(String); - impl fmt::Display for DummyAtom { - fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { - ::fmt(&self.0, fmt) + impl ToCss for DummyAtom { + fn to_css(&self, dest: &mut W) -> fmt::Result + where + W: fmt::Write, + { + serialize_identifier(&self.0, dest) } } @@ -3108,7 +3106,7 @@ pub mod tests { vec![Component::AttributeInNoNamespace { local_name: DummyAtom::from("attr"), operator: AttrSelectorOperator::DashMatch, - value: DummyAtom::from("foo"), + value: DummyAttrValue::from("foo"), never_matches: false, case_sensitivity: ParsedCaseSensitivity::CaseSensitive, }], diff --git a/components/selectors/tree.rs b/components/selectors/tree.rs index ac90fa1f00c..47cea73a208 100644 --- a/components/selectors/tree.rs +++ b/components/selectors/tree.rs @@ -113,7 +113,7 @@ pub trait Element: Sized + Clone + Debug { fn has_class( &self, - name: &::ClassName, + name: &::Identifier, case_sensitivity: CaseSensitivity, ) -> bool; @@ -121,10 +121,10 @@ pub trait Element: Sized + Clone + Debug { /// direction, that is, in an outer-tree -> inner-tree direction. fn imported_part( &self, - name: &::PartName, - ) -> Option<::PartName>; + name: &::Identifier, + ) -> Option<::Identifier>; - fn is_part(&self, name: &::PartName) -> bool; + fn is_part(&self, name: &::Identifier) -> bool; /// Returns whether this element matches `:empty`. ///