Change an instance of .collect::<Vec<_>>().join("") to .collect::<String>()

[yoav-lavi]

Hey,
I've noticed that compiler/rustc_resolve/src/late/diagnostics.rs uses .collect::<Vec<_>>().join(""); over .collect::<String>();, the latter being shorter, clearer and more performant:

    criterion.bench_function("bench 1", |bencher| {
        bencher.iter(|| {
            std::iter::repeat("'a, ")
                .take(10)
                .collect::<Vec<_>>()
                .join("");
        })
    });

    criterion.bench_function("bench 2", |bencher| {
        bencher.iter(|| {
            std::iter::repeat("'a, ").take(10).collect::<String>();
        })
    });

bench 1                 time:   [88.355 ns 88.526 ns 88.704 ns]                         
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) low mild
  1 (1.00%) high mild
slope  [88.355 ns 88.704 ns] R^2            [0.9910173 0.9909753]
mean   [88.312 ns 88.623 ns] std. dev.      [640.43 ps 940.42 ps]
median [88.168 ns 88.576 ns] med. abs. dev. [587.33 ps 932.58 ps]

bench 2                 time:   [110.31 ns 110.50 ns 110.77 ns]                         
Found 5 outliers among 100 measurements (5.00%)
  5 (5.00%) high mild
slope  [110.31 ns 110.77 ns] R^2            [0.9907452 0.9903230]
mean   [110.85 ns 111.38 ns] std. dev.      [1.1039 ns 1.5752 ns]
median [110.25 ns 110.91 ns] med. abs. dev. [395.71 ps 1.1579 ns]

I'm currently working on a clippy PR to lint this issue but thought to create a manual PR for Deno

Thanks for your consideration!

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) soon.

Please see the contribution instructions for more information.

[the8472]

Merge branch 'master' of https://github.com/yoav-lavi/rust

This runs afoul of the no-merge policy, you'll have to rebase.

[yoav-lavi]

@the8472 Should be fine now

[jackh726]

bench 1                 time:   [88.355 ns 88.526 ns 88.704 ns]                         
bench 2                 time:   [110.31 ns 110.50 ns 110.77 ns]

Unless I'm reading this wrong, doesn't this suggest the opposite? That collecting into a vec and joining is faster?

[yoav-lavi]

@jackh726 you're right, I misread the results in this case.

All other instances of .collect::<Vec<_>>().join("") that I checked were slower.
I wonder why it's behaving differently in this case, will look into it. Closing for now.

Thanks!

[yoav-lavi]

For instance if you take a similar case that Deno had:

    criterion.bench_function("1", |bencher| {
        bencher.iter(|| {
            let url = "https://google.com".to_owned();
            let split_specifier = url.as_str().split(':');
            split_specifier.skip(1).collect::<String>();
        })
    });

    criterion.bench_function("2", |bencher| {
        bencher.iter(|| {
            let url = "https://google.com".to_owned();
            let split_specifier = url.as_str().split(':');
            split_specifier.skip(1).collect::<Vec<_>>().join("");
        })
    });

1                       time:   [61.874 ns 61.959 ns 62.071 ns]                   
Found 14 outliers among 100 measurements (14.00%)
4 (4.00%) high mild
10 (10.00%) high severe
slope  [61.874 ns 62.071 ns] R^2            [0.9864790 0.9862826]
mean   [61.979 ns 62.326 ns] std. dev.      [424.74 ps 1.3139 ns]
median [61.842 ns 61.906 ns] med. abs. dev. [131.58 ps 218.49 ps]

2                       time:   [93.029 ns 93.209 ns 93.443 ns]                   
Found 8 outliers among 100 measurements (8.00%)
4 (4.00%) high mild
4 (4.00%) high severe
slope  [93.029 ns 93.443 ns] R^2            [0.9892041 0.9888479]
mean   [93.153 ns 93.528 ns] std. dev.      [626.57 ps 1.2486 ns]
median [92.996 ns 93.290 ns] med. abs. dev. [362.85 ps 716.21 ps]

[yoav-lavi]

And my own benchmark:

fn criterion_benchmark(criterion: &mut Criterion) {
    criterion.bench_function("1", |bencher| {
        bencher.iter(|| {
            vec!["hello", "world"]
                .iter()
                .map(|item| item.to_uppercase())
                .collect::<Vec<String>>()
                .join("")
        })
    });
    criterion.bench_function("2", |bencher| {
        bencher.iter(|| {
            vec!["hello", "world"]
                .iter()
                .map(|item| item.to_uppercase())
                .collect::<String>()
        })
    });
}

1                       time:   [155.79 ns 155.97 ns 156.16 ns]                   
Found 4 outliers among 100 measurements (4.00%)
  2 (2.00%) high mild
  2 (2.00%) high severe
slope  [155.79 ns 156.16 ns] R^2            [0.9971885 0.9971738]
mean   [155.87 ns 156.58 ns] std. dev.      [672.36 ps 2.9345 ns]
median [156.12 ns 156.26 ns] med. abs. dev. [209.02 ps 650.40 ps]

2                       time:   [118.11 ns 118.20 ns 118.28 ns]                   
Found 8 outliers among 100 measurements (8.00%)
  8 (8.00%) high severe
slope  [118.11 ns 118.28 ns] R^2            [0.9987699 0.9987697]
mean   [118.31 ns 118.87 ns] std. dev.      [794.06 ps 2.0461 ns]
median [117.96 ns 118.43 ns] med. abs. dev. [154.11 ps 692.31 ps]

[paolobarbolini]

Compiler Explorer shows that although the generated assembly is simpler it doesn't unroll the loop as it does with .collect<Vec<_>>() https://rust.godbolt.org/z/WTxh83Y4z. This might be fixed by specialization!? (just guessing, I don't know too much about this stuff)