Remove probably unused impl ImplicitClone on Cow<str>

[cecton]

Description

This trait implies that cloning will be always cheap. In Cow<str> this might not be the case if the variant is Cow::Owned and the String is big. Unfortunately this is something verifiable only at runtime. Therefore it makes sense to remove ImplicitClone for Cow<str> as a way to guarantee the cheap cloning.

(Not sure if that's super clear xD is it? feel free to edit)

Link to related experiment examples: https://github.com/rustminded/yew-immutable/tree/main/examples

Checklist

• I have run cargo make pr-flow nahh
• I have reviewed my own code
• I have added tests not possible to write a test to ensure a trait is not implemented since all tests need to compile

[github-actions]

Visit the preview URL for this PR (updated for commit eeb09aa):

https://yew-rs-api--pr2447-cecton-remove-implic-m874ivwi.web.app

_{(expires Wed, 16 Feb 2022 14:19:29 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

[cecton]

@hamza1311 all checks passed ✔️

So... the last question remaining is: do we want to break the API of 0.20 for this? cc @siku2

For me it sounds really minor for this particular case but I don't mind to continue working on a fork of yew until I reach the conclusions of my experiment.

[siku2]

I absolutely don't mind the breaking change and I agree with your semantic definition of ImplicitClone. What I'm worried about is that it's a bit hypocritical to remove the implementation for Cow when the same argument can be applied to AttrValue as well.

[cecton]

@siku2 fair. This lacks of consistency.

... 🤔

Maybe I can just remove the variant Owned on AttrValue.

[cecton]

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

[cecton]

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)

[ranile]

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

[cecton]

[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.

[cecton]

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)

[ranile]

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

Nevermind, it's already removed.

[cecton]

Yup! ^_^'

[ranile]

Maybe I can just remove the variant Owned on AttrValue.

That was actually mentioned when I implemented AttrValue but I never with it. See #1994 (comment)