Generalize `{Rc,Arc}::make_mut()` to unsized types. #116113

kpreid · 2023-09-23T23:45:23Z

{Rc,Arc}::make_mut() now accept any type implementing the new unstable trait core::clone::CloneToUninit.
CloneToUninit is implemented for T: Clone and for [T] where T: Clone.
CloneToUninit is a generalization of the existing internal trait alloc::alloc::WriteCloneIntoRaw.
New feature gate: clone_to_uninit

This allows performing make_mut() on Rc<[T]> and Arc<[T]>, which was not previously possible.

Previous PR description, now obsolete:

Add {Rc, Arc}::make_mut_slice()

These functions behave identically to make_mut(), but operate on Arc<[T]> instead of Arc<T>.

This allows performing the operation on slices, which was not previously possible because make_mut() requires T: Clone (and slices, being !Sized, do not and currently cannot implement Clone).

Feature gate: make_mut_slice

try-job: test-various

rustbot · 2023-09-23T23:45:30Z

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

kpreid · 2023-09-23T23:45:46Z

@rustbot label +T-libs-api

thomcc · 2023-09-24T00:22:47Z

This is insta-stable, so it needs libs-api FCP.

r? @rust-lang/libs-api

thomcc · 2023-09-24T00:23:28Z

Rerolling because @BurntSushi doesn't do libs reviews.

r? rust-lang/libs-api

dtolnay

How about adjusting the trait bound on make_mut instead? It can be implemented the current way for T: Clone and separately for [T].

impl<T: CanMakeMut, A: Allocator + Clone> Arc<T, A> {
    pub fn make_mut(this: &mut Self) -> &mut T { ... }
}

impl<T: Clone> CanMakeMut for T { ... }

impl<T: Clone> CanMakeMut for [T] { ... }

kpreid · 2023-09-24T00:46:57Z

This is insta-stable, so it needs libs-api FCP.

Is it? The current commit only adds unstable inherent methods. There's no trait implementation involved.

How about adjusting the trait bound on make_mut instead?

I like that idea much better since it doesn't add a new name. (Though it would be insta-stable.) I'm not familiar with how introducing a trait like CanMakeMut should work in std; should the trait be (perma?-)unstable or public-in-private sealed?

Actually, the trait could be of more broad use — it seems like it could be a general "clone ?Sized into uninit", which suggests it should be an unstable trait not tied to Rc/Arc in particular. So maybe it becomes core::mem::CloneUnsized or something like that? Is there any existing work on unsized rvalues that has such a trait?

thomcc · 2023-09-24T00:56:23Z

Oh, it's totally not insta-stable, my bad (sorry, been a draining couple of weeks for me).

Still, I'll leave the review to the assignee at this point.

dtolnay · 2023-09-24T00:57:41Z

The core::clone module has been looking pretty bare for a decade. 🥳

I would definitely be on board with a trait that allows cloning into a pre-allocated &mut MaybeUninit<Self>. I think it would need to be unsafe trait, right? The calling code needs to be able to do assume_init afterwards without the possibility that the trait impl didn't actually initialize the thing.

I am not familiar with the Rc and Arc code but if such a trait existed, would it be straightforward for their make_mut to allocate a value of size header + mem::size_of_val(&**self), cast the spot for the value to &mut MaybeUninit<T>, and call the CloneUnsized to get the value cloned into there?

library/alloc/src/sync.rs

kpreid · 2023-09-24T16:05:54Z

Observation: The new CloneToUninit trait turns out to be identical in signature to the internal trait alloc::alloc::WriteCloneIntoRaw, except for the ?Sizedness. That's a good sign, I think — I'm refining something that already proved useful rather than making up an ad-hoc solution.

kpreid · 2023-09-24T17:41:08Z

Observation: It's not possible to define these operations in terms of MaybeUninit<T> (as Rc currently does inside maek_mut(), and a previous comment suggested), because MaybeUninit<T> requires T: Sized, so you cannot have a MaybeUninit<T> where T might be [U]. I wonder if changing that would help all maybe-unsized code — but presumably it's difficult, and certainly out of scope for this PR.

kpreid · 2023-09-25T00:57:52Z

It was quite an adventure, but I believe I've got it all set now. I had to introduce additional helper types at both the CloneToUninit layer and the make_mut() layer in order to avoid leaks on panic.

Note that make_mut()'s enlarged receiver type bounds are insta-stable. Additionally, you can tell that CloneToUninit is specialized in the documentation, but I assume that's fine because it doesn't affect which programs will compile.

kpreid · 2023-09-25T01:03:40Z

Updated PR description with the new design based on CloneToUninit.

dtolnay · 2024-06-22T16:14:25Z

@bors r+

bors · 2024-06-22T16:14:28Z

📌 Commit 88c3db5 has been approved by dtolnay

It is now in the queue for this repository.

bors · 2024-06-22T16:35:32Z

⌛ Testing commit 88c3db5 with merge f944afe...

bors · 2024-06-22T18:47:03Z

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

rust-timer · 2024-06-22T20:02:14Z

Finished benchmarking commit (f944afe): comparison URL.

Overall result: ❌ regressions - 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.8%	[0.3%, 2.2%]	13
Regressions ❌ (secondary)	0.9%	[0.9%, 0.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[0.3%, 2.2%]	13

Max RSS (memory usage)

Results (primary -5.7%, secondary -2.2%)

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)	-5.7%	[-5.7%, -5.7%]	1
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	-5.7%	[-5.7%, -5.7%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (primary 0.1%, secondary 0.3%)

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.2%	[0.1%, 0.3%]	8
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.0%]	7
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.1%, 0.3%]	15

Bootstrap: 694.577s -> 694.429s (-0.02%)
Artifact size: 327.03 MiB -> 326.84 MiB (-0.06%)

Mark-Simulacrum · 2024-06-23T22:08:09Z

Regressions are mostly in doc benchmarks, seem likely to be just new docs due to extra stuff in the standard library.

rustbot assigned thomcc Sep 23, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 23, 2023

rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 23, 2023

This comment has been minimized.

Sign in to view

rustbot assigned BurntSushi and unassigned thomcc Sep 24, 2023

rustbot assigned dtolnay and unassigned BurntSushi Sep 24, 2023

dtolnay removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 24, 2023

dtolnay requested changes Sep 24, 2023

View reviewed changes

dtolnay reviewed Sep 24, 2023

View reviewed changes

library/alloc/src/sync.rs Outdated Show resolved Hide resolved

kpreid force-pushed the arcmut branch from b621d08 to 12c3efe Compare September 25, 2023 00:27

This comment has been minimized.

Sign in to view

kpreid force-pushed the arcmut branch from 12c3efe to 8ebd675 Compare September 25, 2023 00:56

kpreid changed the title ~~Add {Rc, Arc}::make_mut_slice().~~ Generalize {Rc,Arc}::make_mut() to unsized types. Sep 25, 2023

kpreid requested a review from dtolnay September 25, 2023 01:03

This comment has been minimized.

Sign in to view

kpreid force-pushed the arcmut branch from 8ebd675 to 8923cab Compare September 25, 2023 01:44

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 22, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 22, 2024

bors merged commit f944afe into rust-lang:master Jun 22, 2024

rustbot added this to the 1.81.0 milestone Jun 22, 2024

This was referenced Jun 22, 2024

Replace uses of MaybeUninit::uninit_array() with inline const blocks. #125082

Merged

speed up String::push and String::insert #124810

Merged

Add MAX_LEN_UTF8 and MAX_LEN_UTF16 Constants #120580

Merged

rustbot added the perf-regression Performance regression. label Jun 22, 2024

Enselic mentioned this pull request Jun 23, 2024

Bless nightly-2024-06-23 output cargo-public-api/cargo-public-api#585

Merged

Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Jun 23, 2024

kpreid deleted the arcmut branch June 24, 2024 17:39

apiraino removed the to-announce Announce this issue on triage meeting label Jul 4, 2024

celinval added a commit to celinval/rust-dev that referenced this pull request Jul 17, 2024

Uh oh!

Generalize {Rc,Arc}::make_mut() to unsized types. #116113

Generalize {Rc,Arc}::make_mut() to unsized types. #116113

Uh oh!

Conversation

kpreid commented Sep 23, 2023 • edited by workingjubilee Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Sep 23, 2023

Uh oh!

kpreid commented Sep 23, 2023

Uh oh!

This comment has been minimized.

thomcc commented Sep 24, 2023

Uh oh!

thomcc commented Sep 24, 2023

Uh oh!

dtolnay left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpreid commented Sep 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomcc commented Sep 24, 2023

Uh oh!

dtolnay commented Sep 24, 2023

Uh oh!

Uh oh!

kpreid commented Sep 24, 2023

Uh oh!

kpreid commented Sep 24, 2023

Uh oh!

This comment has been minimized.

kpreid commented Sep 25, 2023

Uh oh!

kpreid commented Sep 25, 2023

Uh oh!

This comment has been minimized.

dtolnay commented Jun 22, 2024

Uh oh!

bors commented Jun 22, 2024

Uh oh!

bors commented Jun 22, 2024

Uh oh!

bors commented Jun 22, 2024

Uh oh!

rust-timer commented Jun 22, 2024

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Mark-Simulacrum commented Jun 23, 2024

Uh oh!

Generalize `{Rc,Arc}::make_mut()` to unsized types. #116113

Generalize `{Rc,Arc}::make_mut()` to unsized types. #116113

kpreid commented Sep 23, 2023 •

edited by workingjubilee

Loading

dtolnay left a comment •

edited

Loading

kpreid commented Sep 24, 2023 •

edited

Loading