Skip to content

Remove probably unused impl ImplicitClone on Cow<str> #2447

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.

Sign up for GitHub

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

Merged
ranile merged 4 commits into from
Feb 11, 2022
Merged

Remove probably unused impl ImplicitClone on Cow<str> #2447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
516abd5
Remove probably unused impl ImplicitClone on Cow<str>
cecton Feb 9, 2022
eb334c0
Remove unused import
cecton Feb 9, 2022
602957c
Remove variant Owned for AttrValue for consistency
cecton Feb 9, 2022
eeb09aa
Implement ImplicitClone on primitive types
cecton Feb 9, 2022
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
2 changes: 1 addition & 1 deletion packages/yew/src/html/classes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ impl IntoPropValue<AttrValue> for Classes {
None => unsafe { unreachable_unchecked() },
}
} else {
AttrValue::Owned(self.to_string())
Copy link
Copy Markdown
Contributor Author

@cecton cecton Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the end I think it is probably more optimal with an Rc here. I can imagine tons of classes being passed.

I don't know how big a string needs to be before it's more optimal to use Rc. 🤔

Besides, if I understood correctly, there is no cost of double indirection here since the String is actually transformed to Rc<str>.

Related conversation: https://twitter.com/Argorak/status/1491044312595185667?s=20&t=5e2MZV7sWpdCA3GlZpaQlA

Copy link
Copy Markdown
Contributor Author

@cecton cecton Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I'm really not looking at performance but more at correctness. The fact that ImplicitClone should guarantee - at compile time - that this will be cheap to clone sounds important to me. (Critics are welcome)

Copy link
Copy Markdown
Member

@ranile ranile Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial concern when not removing Owned was this unsolved question: which one of the is cheaper to clone (if there's a difference at all):

let s = String::new();
let rc = Rc::from("")'

This can be applied with small strings, which are not just empty strings

Copy link
Copy Markdown
Contributor Author

@cecton cecton Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

[package]
name = "tmp-rlrkag"
version = "0.1.0"
edition = "2021"

[dependencies]

[dev-dependencies]
criterion = "0.3"

[[bench]]
name = "my_benchmark"
harness = false

use std::rc::Rc;
use criterion::{black_box, criterion_group, criterion_main, Criterion};

fn rc_str(s: Rc<str>) -> Rc<str> {
    s.clone()
}

fn string(s: String) -> String {
    s.clone()
}

fn criterion_benchmark_rc_str(c: &mut Criterion) {
    let s = String::new();
    let rc = Rc::from(s);
    c.bench_function("Rc<str>", |b| b.iter(|| rc_str(black_box(Rc::clone(&rc)))));
}

fn criterion_benchmark_string(c: &mut Criterion) {
    let s = String::new();
    c.bench_function("String", |b| b.iter(|| string(black_box(s.clone()))));
}

criterion_group!(benches, criterion_benchmark_rc_str, criterion_benchmark_string);
criterion_main!(benches);

Results:

Rc<str>                 time:   [1.0763 ns 1.0769 ns 1.0774 ns]                     
Found 31 outliers among 100 measurements (31.00%)
  6 (6.00%) low severe
  8 (8.00%) low mild
  14 (14.00%) high mild
  3 (3.00%) high severe

String                  time:   [7.7115 ns 7.7901 ns 7.8681 ns]                    
Found 21 outliers among 100 measurements (21.00%)
  2 (2.00%) low severe
  7 (7.00%) low mild
  1 (1.00%) high mild
  11 (11.00%) high severe

It seems cloning the Rc is faster than the empty String.

Copy link
Copy Markdown
Contributor Author

@cecton cecton Feb 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also checked if reversing the order of the benchmarks change anything (in case of CPU warm up) but apparently it doesn't affect the performances:

String                  time:   [7.8812 ns 8.0260 ns 8.1961 ns]                    
                        change: [+2.5291% +4.2576% +6.1807%] (p = 0.00 < 0.05)
                        Performance has regressed.

Rc<str>                 time:   [1.0555 ns 1.0572 ns 1.0595 ns]                     
                        change: [-2.2550% -1.8980% -1.5580%] (p = 0.00 < 0.05)
                        Performance has improved.

(It says "regressed" / "increased" but that's probably because the measure is so small. It doesn't really improve or increased anything)

Copy link
Copy Markdown
Member

@ranile ranile Feb 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create an issue for removing AttrValue::Owned with these benchmarks?

Nevermind, it's already removed.

Copy link
Copy Markdown
Contributor Author

@cecton cecton Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! ^_^'

AttrValue::Rc(Rc::from(self.to_string()))
}
}
}
Expand Down
21 changes: 16 additions & 5 deletions packages/yew/src/html/conversion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,31 @@
use super::{Component, NodeRef, Scope};
use crate::virtual_dom::AttrValue;
use std::{borrow::Cow, rc::Rc};
use std::rc::Rc;

/// Marker trait for types that the [`html!`](macro@crate::html) macro may clone implicitly.
pub trait ImplicitClone: Clone {}

// this is only implemented because there's no way to avoid cloning this value
impl ImplicitClone for Cow<'static, str> {}

impl<T: ImplicitClone> ImplicitClone for Option<T> {}
impl<T> ImplicitClone for Rc<T> {}

impl ImplicitClone for NodeRef {}
impl<Comp: Component> ImplicitClone for Scope<Comp> {}
// TODO there are still a few missing

macro_rules! impl_implicit_clone {
($($ty:ty),+ $(,)?) => {
$(impl ImplicitClone for $ty {})*
};
}

#[rustfmt::skip]
impl_implicit_clone!(
u8, u16, u32, u64, u128,
i8, i16, i32, i64, i128,
f32, f64,
&'static str,
);

/// A trait similar to `Into<T>` which allows conversion to a value of a `Properties` struct.
pub trait IntoPropValue<T> {
/// Convert `self` to a value of a `Properties` struct.
Expand Down Expand Up @@ -85,7 +96,7 @@ macro_rules! impl_into_prop {
impl_into_prop!(|value: &'static str| -> String { value.to_owned() });

impl_into_prop!(|value: &'static str| -> AttrValue { AttrValue::Static(value) });
impl_into_prop!(|value: String| -> AttrValue { AttrValue::Owned(value) });
impl_into_prop!(|value: String| -> AttrValue { AttrValue::Rc(Rc::from(value)) });
impl_into_prop!(|value: Rc<str>| -> AttrValue { AttrValue::Rc(value) });

#[cfg(test)]
Expand Down
23 changes: 3 additions & 20 deletions packages/yew/src/virtual_dom/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,6 @@ use std::rc::Rc;
pub enum AttrValue {
/// String living for `'static`
Static(&'static str),
/// Owned string
Owned(String),
/// Reference counted string
Rc(Rc<str>),
}
Expand All @@ -64,7 +62,6 @@ impl Deref for AttrValue {
fn deref(&self) -> &Self::Target {
match self {
AttrValue::Static(s) => *s,
AttrValue::Owned(s) => s.as_str(),
AttrValue::Rc(s) => &*s,
}
}
Expand All @@ -78,7 +75,7 @@ impl From<&'static str> for AttrValue {

impl From<String> for AttrValue {
fn from(s: String) -> Self {
AttrValue::Owned(s)
AttrValue::Rc(Rc::from(s))
}
}

Expand All @@ -101,7 +98,6 @@ impl Clone for AttrValue {
fn clone(&self) -> Self {
match self {
AttrValue::Static(s) => AttrValue::Static(s),
AttrValue::Owned(s) => AttrValue::Owned(s.clone()),
AttrValue::Rc(s) => AttrValue::Rc(Rc::clone(s)),
}
}
Expand All @@ -117,7 +113,6 @@ impl fmt::Display for AttrValue {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
match self {
AttrValue::Static(s) => write!(f, "{}", s),
AttrValue::Owned(s) => write!(f, "{}", s),
AttrValue::Rc(s) => write!(f, "{}", s),
}
}
Expand All @@ -138,7 +133,6 @@ impl AttrValue {
pub fn into_string(self) -> String {
match self {
AttrValue::Static(s) => (*s).to_owned(),
AttrValue::Owned(s) => s,
AttrValue::Rc(mut rc) => {
if let Some(s) = Rc::get_mut(&mut rc) {
(*s).to_owned()
Expand All @@ -159,9 +153,6 @@ mod tests_attr_value {
let av = AttrValue::Static("str");
assert_eq!(av.into_string(), "str");

let av = AttrValue::Owned("String".to_string());
assert_eq!(av.into_string(), "String");

let av = AttrValue::Rc("Rc<str>".into());
assert_eq!(av.into_string(), "Rc<str>");
}
Expand All @@ -176,25 +167,17 @@ mod tests_attr_value {

let av = AttrValue::from(Cow::from("BorrowedCow"));
assert_eq!(av.into_string(), "BorrowedCow");

let av = AttrValue::from(Cow::from("OwnedCow".to_string()));
assert_eq!(av.into_string(), "OwnedCow");
}

#[test]
fn test_equality() {
// construct 3 AttrValue with same embedded value; expectation is that all are equal
let a = AttrValue::Owned("same".to_string());
let b = AttrValue::Static("same");
let c = AttrValue::Rc("same".into());
let a = AttrValue::Static("same");
let b = AttrValue::Rc("same".into());

assert_eq!(a, b);
assert_eq!(b, c);
assert_eq!(a, c);

assert_eq!(a, b);
assert_eq!(b, c);
assert_eq!(a, c);
}
}

Expand Down