From 6751946477b836745366ddfbf197d0a0adcd89d5 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Wed, 21 Sep 2022 00:41:10 -0400 Subject: [PATCH] Various improvements to Classes, oriented around reducing allocations (#2870) * Various improvements to Classes, oriented around reducing allocations - `push` no longer performs unnecessary allocations if `self` is empty. - Most constructors (FromIterator, From, Extend) are now oriented around push, to take advantage of this - `to_string` and `into_prop_value`: - No longer allocate an unnecessary `Vec`; they instead preallocate a string of the correct length and push into it directly. - No longer use a length check + `unsafe`; they instead match over a fallible .next() and proceed from there. - Additionally, `into_prop_value` no longer builds a `String` or `Rc` if `Classes` contains a single `&'static str` or is empty. - `From` no longer clones the string if it contains a single class. - `impl Eq for Classes` * Fix duplicated is_empty test --- packages/yew/src/html/classes.rs | 93 +++++++++++++++++++++++--------- 1 file changed, 69 insertions(+), 24 deletions(-) diff --git a/packages/yew/src/html/classes.rs b/packages/yew/src/html/classes.rs index 5311ca6e163..a2805be532b 100644 --- a/packages/yew/src/html/classes.rs +++ b/packages/yew/src/html/classes.rs @@ -1,5 +1,4 @@ use std::borrow::{Borrow, Cow}; -use std::hint::unreachable_unchecked; use std::iter::FromIterator; use std::rc::Rc; @@ -16,8 +15,28 @@ pub struct Classes { set: IndexSet>, } +/// helper method to efficiently turn a set of classes into a space-separated +/// string. Abstracts differences between ToString and IntoPropValue. The +/// `rest` iterator is cloned to pre-compute the length of the String; it +/// should be cheap to clone. +fn build_string<'a>(first: &'a str, rest: impl Iterator + Clone) -> String { + // The length of the string is known to be the length of all the + // components, plus one space for each element in `rest`. + let mut s = String::with_capacity( + rest.clone() + .map(|class| class.len()) + .chain([first.len(), rest.size_hint().0]) + .sum(), + ); + + s.push_str(first); + s.extend(rest.flat_map(|class| [" ", class])); + s +} + impl Classes { /// Creates an empty set of classes. (Does not allocate.) + #[inline] pub fn new() -> Self { Self { set: IndexSet::new(), @@ -26,6 +45,7 @@ impl Classes { /// Creates an empty set of classes with capacity for n elements. (Does not allocate if n is /// zero.) + #[inline] pub fn with_capacity(n: usize) -> Self { Self { set: IndexSet::with_capacity(n), @@ -37,7 +57,11 @@ impl Classes { /// If the provided class has already been added, this method will ignore it. pub fn push>(&mut self, class: T) { let classes_to_add: Self = class.into(); - self.set.extend(classes_to_add.set); + if self.is_empty() { + *self = classes_to_add + } else { + self.set.extend(classes_to_add.set) + } } /// Adds a class to a set. @@ -49,18 +73,20 @@ impl Classes { /// # Safety /// /// This function will not split the string into multiple classes. Please do not use it unless - /// you are absolutely certain that the string does not contain any whitespace. Using `push()` - /// is preferred. + /// you are absolutely certain that the string does not contain any whitespace and it is not + /// empty. Using `push()` is preferred. pub unsafe fn unchecked_push>>(&mut self, class: T) { self.set.insert(class.into()); } /// Check the set contains a class. + #[inline] pub fn contains>(&self, class: T) -> bool { self.set.contains(class.as_ref()) } /// Check the set is empty. + #[inline] pub fn is_empty(&self) -> bool { self.set.is_empty() } @@ -68,15 +94,16 @@ impl Classes { impl IntoPropValue for Classes { #[inline] - fn into_prop_value(mut self) -> AttrValue { - if self.set.len() == 1 { - match self.set.pop() { - Some(attr) => AttrValue::Rc(Rc::from(attr)), - // SAFETY: the collection is checked to be non-empty above - None => unsafe { unreachable_unchecked() }, - } - } else { - AttrValue::Rc(Rc::from(self.to_string())) + fn into_prop_value(self) -> AttrValue { + let mut classes = self.set.iter(); + + match classes.next() { + None => AttrValue::Static(""), + Some(class) if classes.len() == 0 => match *class { + Cow::Borrowed(class) => AttrValue::Static(class), + Cow::Owned(ref class) => AttrValue::Rc(Rc::from(class.as_str())), + }, + Some(first) => AttrValue::Rc(Rc::from(build_string(first, classes.map(Cow::borrow)))), } } } @@ -93,6 +120,7 @@ impl IntoPropValue> for Classes { } impl IntoPropValue for &'static str { + #[inline] fn into_prop_value(self) -> Classes { self.into() } @@ -100,11 +128,7 @@ impl IntoPropValue for &'static str { impl> Extend for Classes { fn extend>(&mut self, iter: I) { - let classes = iter - .into_iter() - .map(Into::into) - .flat_map(|classes| classes.set); - self.set.extend(classes); + iter.into_iter().for_each(|classes| self.push(classes)) } } @@ -120,6 +144,7 @@ impl IntoIterator for Classes { type IntoIter = indexmap::set::IntoIter>; type Item = Cow<'static, str>; + #[inline] fn into_iter(self) -> Self::IntoIter { self.set.into_iter() } @@ -127,11 +152,11 @@ impl IntoIterator for Classes { impl ToString for Classes { fn to_string(&self) -> String { - self.set - .iter() - .map(Borrow::borrow) - .collect::>() - .join(" ") + let mut iter = self.set.iter().map(Cow::borrow); + + iter.next() + .map(|first| build_string(first, iter)) + .unwrap_or_default() } } @@ -153,7 +178,18 @@ impl From<&'static str> for Classes { impl From for Classes { fn from(t: String) -> Self { - Self::from(&t) + match t.contains(|c: char| c.is_whitespace()) { + // If the string only contains a single class, we can just use it + // directly (rather than cloning it into a new string). Need to make + // sure it's not empty, though. + false => match t.is_empty() { + true => Self::new(), + false => Self { + set: IndexSet::from_iter([Cow::Owned(t)]), + }, + }, + true => Self::from(&t), + } } } @@ -198,6 +234,8 @@ impl PartialEq for Classes { } } +impl Eq for Classes {} + #[cfg(test)] mod tests { use super::*; @@ -285,4 +323,11 @@ mod tests { assert!(subject.contains("foo")); assert!(subject.contains("bar")); } + + #[test] + fn ignores_empty_string() { + let classes = String::from(""); + let subject = Classes::from(classes); + assert!(subject.is_empty()) + } }