String cloning is not optimized the same way as String construction

[SadiinsoSnowfall]

I don't know if it is really worth looking into, but I think I found some weird behavior in the rust compiler:
Let's consider the two following functions (godbolt link):

pub fn nop(a: &String) {
    a.as_str().to_owned();
}

pub fn complex(a: &String) {
    a.to_owned();
}

They both allocate a String struct but do not use or return it. Rustc is able to optimize away the first one, but not the second one.
If we dig deeper, we can observe that the compiler is not able to optimize away the to_owned and clone method when called on a string. Let's look at the following functions (godbolt link):

pub fn a() {
    "ok".to_owned();
}

pub fn b() {
    "ok".to_owned().clone();
}

pub fn c() {
    "ok".to_owned().as_str().to_owned();
}

pub fn d() {
    "ok".to_owned().to_owned();
}

pub fn e() {
    String::from("ok");
}

The fonctions a, c and e are optimized away even on opt-level 2. Whereas the functions b and d are not and still produce calls to alloc, dealloc, drop_in_place, etc...

Looking at the stdlib code, <&str>::to_owned internally calls <&[u8]>::to_owned, which calls <&[u8]>::to_vec, which calls <&[u8]>::to_vec_in which ends up calling hack::to_vec.

String::to_string calls String::to_owned which calls String::clone which calls Vec<u8>::clone which calls <&[u8]>::to_vec_in with the only difference from the <&str>::to_owned path being that the possibly custom allocator of the Vec internal buffer is cloned and passed to <&[u8]>::to_vec_in in Vec<u8>::clone.

Is it possible that rustc is not able to correctly inline the allocator functions when the default allocator is not directly assumed ?

This was tested on Rustc 1.55 and `Rustc 1.57.0-nightly (b69fe57 2021-09-10)

This issue has been assigned to @notriddle via this comment.

[nagisa]

Related #24194

[SadiinsoSnowfall]

While trying to understand why this is, I found another strange behavior (godbolt link): cloning a Vec containing elements with more than 1 byte wide cannot be optimized away by the compiler. I think this is because of Layout::repeat (called by Layout::array, in turn called by RawVec::allocate_in) which cannot fail for u8 (because (std::mem::size_of::<u8>(), std::mem::align_of::<u8>()) == (1, 1)) but can fail for any larger type (even though it should not in this case because an already existing slice is passed to the function). And this (checking the layout validity) might prevent the compiler from completly optimizing the clone operation.

However, I do not think this is related because all the previous tests used u8 slices.

[the8472]

This looks like it's a matter of inlining. String::clone is neither a generic method nor #[inline] thus it's not eligible for cross-crate inlining, which means it won't be optimized out because the optimizer doesn't know it won't have side-effects without inlining.
It does get eliminated when building a bin with LTO.

[SadiinsoSnowfall]

Hum, yes, I think this is it. Trying not to use String::clone (godbolt link) gives something much closer to the expected result but not perfect. Note that the resulting assembly is way better in the current nightly than it is in the current stable version, 1.55.

If it does get eliminated during LTO, it is not a problem. Should I close this issue ?

[camelid]

@rustbot assign @notriddle

[notriddle]

@SadiinsoSnowfall is this example reduced from a realistic use case?

[SadiinsoSnowfall]

@notriddle No, I was just playing around with compiler explorer. As the8472 said, this can be optimized during LTO so it might be a non-issue (as shown in the bench run in your PR).
I am closing the issue.