Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various improvements to Classes, oriented around reducing allocations #2870

Merged
merged 2 commits into from
Sep 21, 2022
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 67 additions & 22 deletions packages/yew/src/html/classes.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use std::borrow::{Borrow, Cow};
use std::hint::unreachable_unchecked;
use std::iter::FromIterator;
use std::rc::Rc;

Expand All @@ -16,8 +15,28 @@ pub struct Classes {
set: IndexSet<Cow<'static, str>>,
}

/// 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<Item = &'a str> + 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()
Lucretiel marked this conversation as resolved.
Show resolved Hide resolved
.map(|class| class.len())
.chain([first.len(), rest.size_hint().0])
.sum(),
Lucretiel marked this conversation as resolved.
Show resolved Hide resolved
);

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(),
Expand All @@ -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),
Expand All @@ -37,7 +57,11 @@ impl Classes {
/// If the provided class has already been added, this method will ignore it.
pub fn push<T: Into<Self>>(&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.
Expand All @@ -56,27 +80,30 @@ impl Classes {
}

/// Check the set contains a class.
#[inline]
pub fn contains<T: AsRef<str>>(&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()
}
}

impl IntoPropValue<AttrValue> 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)))),
}
}
}
Expand All @@ -93,18 +120,15 @@ impl IntoPropValue<Option<AttrValue>> for Classes {
}

impl IntoPropValue<Classes> for &'static str {
#[inline]
fn into_prop_value(self) -> Classes {
self.into()
}
}

impl<T: Into<Classes>> Extend<T> for Classes {
fn extend<I: IntoIterator<Item = T>>(&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))
}
}

Expand All @@ -120,18 +144,19 @@ impl IntoIterator for Classes {
type IntoIter = indexmap::set::IntoIter<Cow<'static, str>>;
type Item = Cow<'static, str>;

#[inline]
fn into_iter(self) -> Self::IntoIter {
self.set.into_iter()
}
}

impl ToString for Classes {
fn to_string(&self) -> String {
self.set
.iter()
.map(Borrow::borrow)
.collect::<Vec<_>>()
.join(" ")
let mut iter = self.set.iter().map(Cow::borrow);

iter.next()
.map(|first| build_string(first, iter))
.unwrap_or_default()
}
}

Expand All @@ -153,7 +178,18 @@ impl From<&'static str> for Classes {

impl From<String> for Classes {
fn from(t: String) -> Self {
Self::from(&t)
match t.contains(|c: char| c.is_whitespace()) && !t.is_empty() {
// 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),
}
Lucretiel marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -198,6 +234,8 @@ impl PartialEq for Classes {
}
}

impl Eq for Classes {}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -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())
}
}