CloneToUninit impls

[GrigorenkoPV]

As per #126799.

Also implements it for Wtf8 and both versions of os_str::Slice.

Maybe it is worth to slap #[inline] on some of those impls.

r? @dtolnay

[workingjubilee]

Maybe it is worth to slap #[inline] on some of those impls.

Yes.

[GrigorenkoPV]

Maybe it is worth to slap #[inline] on some of those impls.

Yes.

Which ones though?

[workingjubilee]

probably at least str?

[GrigorenkoPV]

probably at least str?

Well, it goes kinda like this

Loading

graph LR;
    str-->u8["[u8]"]
    CStr-->u8
    Path-->OsStr
    OsStr-->Slice
    Slice-->|Windows/UEFI|Wtf8
    Wtf8-->u8
    Slice-->|Other platforms|u8

Each edge here is a trivial delegation¹, so I guess it's either all #[inline] or none #[inline]?

Footnotes

1. except for CStr -> u8 in future, maybe. Right now CStr is a fat [u8]² pointer with length, so delegation is trivial, but according to comments, there is a desire to make is a thin pointer (for ffi reasons), then the size would have to be calculated a-la strlen(3). ↩

2. actually [c_char], but that can be treated like [u8]. ↩

[workingjubilee]

LLVM's inlining flow reverses those arrows, so LLVM will decide whether to inline from e.g. [u8] to str first.

[GrigorenkoPV]

LLVM's inlining flow reverses those arrows, so LLVM will decide whether to inline from e.g. [u8] to str first.

So should I add #[inline] only for str, or for Wtf8, Path, OsStr, etc. too?

[workingjubilee]

Probably need to be added to [T] first.

[GrigorenkoPV]

I have pushed a commit that adds #[doc(hidden)] to <T: Copy> specializations (as per #126799 (comment)) and #[inline(always)] to all Unsized folks except for CStr.

I have a feeling I am wrong about some or all of those.

[workingjubilee]

inline(always) is generally overdoing it because it tends to prevent LLVM from backing out of the inlining if it's a bad idea.

[workingjubilee]

Hmm, I should have just referenced https://std-dev-guide.rust-lang.org/policy/inline.html from the beginning. I had forgotten it exists. Sorry about that.

[GrigorenkoPV]

I have force-pushed (twice).

First time changing #[inline(always)] to #[inline].

Second time is a bit more interesting. Aside from rebasing on top of master (and swapping places of short #[inline] and long #[cfg_attr(debug_assertions, track_caller)] to please OCD), I have went ahead and done what #126799 (comment) suggests, i.e. moving specialization to a private trait. This has an additional benefit of no longer needing any #[doc(hidden)] to shove implementation details under the rug.

[GrigorenkoPV]

Sorry, had a bit of a skill issue with rustdoc, here go two three more force-pushes

[dtolnay]

Thank you!

[dtolnay]

@rust-lang/libs-api:
@rfcbot fcp merge

This PR has the immediate, stable consequence that Rc::make_mut and Arc::make_mut become usable with Rc<str>, Rc<CStr>, Rc<OsStr>, Rc<Path> and Arc<_> of the same 4 dynamically sized types.

All of these types are real types that are constructible via a variety of From and Default impls.

impl Default for Rc<str>;
impl From<String> for Rc<str>;
impl From<&str> for Rc<str>;

impl Default for Rc<CStr>;
impl From<CString> for Rc<CStr>;
impl From<&CStr> for Rc<CStr>;

impl From<OsString> for Rc<OsStr>;
impl From<&OsStr> for Rc<OsStr>;

impl From<PathBuf> for Rc<Path>;
impl From<&Path> for Rc<Path>;

// and all the same for Arc

After using make_mut to obtain a &mut T, the caller can go on to use APIs such as Path::as_mut_os_str, OsStr::make_ascii_lowercase, str::as_mut_bytes, str::split_at_mut.

Separately, this PR unblocks an upcoming change to reconcile the set of types usable with Box::clone and Rc::make_mut and Arc::make_mut. It makes sense having all those 3 methods support the same set of types, and shouldn't require 3 separate trait impls for each of our dynamically sized types. All 3 do basically the same thing with their generic inner type: create an allocation of the right size and clone the existing dynamically sized value into it.

[rfcbot]

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[bors]

⌛ Testing commit 110c273 with merge ee2e592...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[GrigorenkoPV]

Retry?

[dtolnay]

@bors retry

[bors]

⌛ Testing commit 110c273 with merge c6f81a4...

[bors]

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing c6f81a4 to master...

[rust-timer]

Finished benchmarking commit (c6f81a4): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.9%	[0.2%, 1.7%]	2
Improvements ✅ (primary)	-0.5%	[-1.1%, -0.2%]	8
Improvements ✅ (secondary)	-2.6%	[-4.8%, -0.3%]	2
All ❌✅ (primary)	-0.5%	[-1.1%, -0.2%]	8

Max RSS (memory usage)

Results (primary -4.6%, secondary 2.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-4.6%	[-8.0%, -1.2%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-4.6%	[-8.0%, -1.2%]	2

Cycles

Results (primary -1.0%, secondary -1.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-1.9%	[-2.1%, -1.7%]	2
All ❌✅ (primary)	-1.0%	[-1.0%, -1.0%]	1

Binary size

Results (primary -0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.1%]	8
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.1%]	6
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.3%, 0.1%]	14

Bootstrap: 750.684s -> 749.433s (-0.17%)
Artifact size: 339.13 MiB -> 339.08 MiB (-0.01%)

[Mark-Simulacrum]

perf regression looks likely to be within noise thresholds, though driven by bimodality that our automatic noise removal doesn't cope well with.