-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Make Classes cheap to clone #3021
Make Classes cheap to clone #3021
Conversation
packages/yew/src/html/classes.rs
Outdated
// NOTE: this can be improved once Iterator::intersperse() becomes stable | ||
for class in rest { | ||
s.push_str(" "); | ||
s.push_str(class.as_str()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to change this because class.as_str()
borrows on class
and the compiler refused to yield the borrow value 😞 not a big deal
@@ -60,7 +67,7 @@ impl Classes { | |||
if self.is_empty() { | |||
*self = classes_to_add | |||
} else { | |||
self.set.extend(classes_to_add.set) | |||
Rc::make_mut(&mut self.set).extend(classes_to_add.set.iter().cloned()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of Rc::make_mut()
is important here as it will avoid cloning the set if it's not necessary
Visit the preview URL for this PR (updated for commit 4275574): https://yew-rs-api--pr3021-cecton-make-classes-1inatn7g.web.app (expires Wed, 14 Dec 2022 00:50:47 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
fn into_prop_value(self) -> AttrValue { | ||
let mut classes = self.set.iter(); | ||
let mut classes = self.set.iter().cloned(); | ||
|
||
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)))), | ||
Some(class) if classes.len() == 0 => class, | ||
Some(first) => build_attr_value(first, classes), | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the code has become simpler thanks to the cloning
fn into_iter(self) -> Self::IntoIter { | ||
self.set.into_iter() | ||
// NOTE: replace this by Rc::unwrap_or_clone() when it becomes stable | ||
Rc::try_unwrap(self.set) | ||
.unwrap_or_else(|rc| (*rc).clone()) | ||
.into_iter() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shamelessly copy-pasted from std. I will take no question
Benchmark - SSRYew Master
Pull Request
|
Size Comparison
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
Description
The goal of this PR is to make
Classes
cheap to clone. My motivation is toimprove the use of
Classes
in UI toolkits likeyewprint. In Yewprint, the user can pass
classes to a component like this:
The next thing Yew will do is to render the actual button to a child component
as you can see here:
This is inefficient at the moment because all the class values (can be
Cow::Owned
) are cloned unnecessarily.To make it more efficient, I'm proposing 2 major changes to the
Classes
object:
use AttrValue instead of Cow in Classes
Cow
is cheap to clone only when the value is a&'static str
. UsingAttrValue
here (aka IString) will allow owned string to be clonedefficiently, thanks to the use of
AttrValue::Rc
.wrap indexset into Rc
In most cases, classes will be added to the user classes before being
passed to the child component. But there are cases where classes are passed
as is to the child component. In that case, it is unnecessary to clone the
internal
IndexSet
itself.Checklist
I have added tests[edit: I have not added test because the API does notchange at all and the current tests are already covering all the use cases.]