Mark smaller CStr and CString functions as #[inline]

[alexbool]

No description provided.

[rust-highfive]

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

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[BurntSushi]

Seems reasonable to me! Thanks! @bors r+

[bors]

📌 Commit d131f86 has been approved by BurntSushi

[ollie27]

I did pretty much exactly this originally in #37622 but was told to remove it.

[BurntSushi]

@ollie27 Oh hmm, all right. I'll stop this and #42714 and ask @alexcrichton to take a look.

@alexbool Sorry about the mixup. Could you maybe say more about why you want these #[inline] annotations?

[BurntSushi]

@bors r-

[alexcrichton]

I feel the same as before. If we've got numbers then sounds good to me, otherwise let's hold off.

[alexbool]

@alexcrichton the motivation here is similar to #42714. To be completely honest, some methods I marked here as #[inline] maybe are a bit too much (like to_string_lossy ), but some others like AsRef conversions are meant to be free of charge, but actually convert to a method call, which is undesirable. Also to be mentioned, similar methods are inlined for String.
What is the policy with existing inlines in std? Are you maintaining status quo?
And is the overhead from these inlines such a problem?

[alexcrichton]

My thinking here is the same over there, it's very easy to go overboard with #[inline] which has negative consequences and as a result I just want us to be data driven. Do you have benchmarks or profiles which show benefits from these #[inline] annotations?

My worry is yes, that functions like to_string_lossy have no need being #[inline], and blanket applying #[inline] to "all the small functions" can harm downstream compile times negatively.

[alexbool]

@alexcrichton I discovered these while profiling an application which is heavy on C interop including string operations. I have seen some of these methods in the profile.
The methods I saw in profile and are somewhat critical to performance of my particular application:

• CStr::as_ptr
• CStr::to_bytes

Following methods are small enough in my opinion to be inlined, but are not critical for my particular application:

• CString::into_raw (essentially a pointer conversion)
• CString::as_bytes (this is only array slicing + bounds check)
• CString::as_bytes_with_nul (same as above)
• CString::as_c_str (only invoking AsRef conversion)
• <CString as From<Box<CStr>>>::from (only delegating to underlying method)
• <CString as Into<Box<CStr>>>::into (same as above)
• <CString as AsRef<CStr>>::as_ref (simple conversion)
• <CString as Deref<CStr>>::deref (same as above)
• CStr::from_bytes_with_nul_unchecked (one transmute)
• CStr::to_bytes (this is only array slicing + bounds check)
• CStr::to_bytes_with_nul (same as above)
• CStr::into_c_string (one transmute)

Other methods that I marked as inline in this PR seem like not so small.
So I propose that we leave methods listed above as inlined. If you feel that this is too risky, I am ready to only leave the first two methods that are valuable to me.

[alexcrichton]

Ok sounds reasonable to me, thanks for taking the time to type that out!

@bors: r+

[bors]

📌 Commit d131f86 has been approved by alexcrichton

[alexbool]

Oh wait @alexcrichton, I think you hurried, I haven't updated the PR yet to remove inlines on larger things like to_string_lossy!

[Mark-Simulacrum]

@bors r-

[alexbool]

Ready to go

[BurntSushi]

@bors r+

[bors]

📌 Commit bcb5b13 has been approved by BurntSushi

[bors]

⌛ Testing commit bcb5b13 with merge 29bce6e...

[alexcrichton]

Travis gave a clean bill of health but the appveyor webhook appears to have failed as the build never started. I'm going to unilaterally declare the risk of a windows-only regression here "low" and merge anyway! If this decision is in error I'll revert.

[alexbool]

Thanks for your time gentlemen!