Fix #124275: Implemented Default for Arc<str>

[Billy-Sheppard]

With added implementations.

GOOD    Arc<CStr> 
BROKEN  Arc<OsStr> // removed
GOOD    Rc<str>
GOOD    Rc<CStr>
BROKEN  Rc<OsStr> // removed

GOOD    Rc<[T]> 
GOOD    Arc<[T]>

For discussion of #124367 (comment).

Key pain points currently:

I've had a guess at the best locations/feature attrs for them but they might not be correct.

However I'm unclear how to get the OsStr impl to compile, which file should they go in to avoid the error below? Is it possible, perhaps with some special std rust lib magic?

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[zachs18]

I think the stable attributes should probably be something like #[stable(feature = "more_rc_default_impls", since = "CURRENT_RUSTC_VERSION")], (literally CURRENT_RUSTC_VERSION, it will be replaced with 1.XX when 1.XX branches (e.g.)).

[Billy-Sheppard]

#124367 (comment)

As per this comment, I've removed the OsStr impls.

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

• 1a65532

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

• 7375074

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

• 09b1611

[Billy-Sheppard]

@rustbot review

[Billy-Sheppard]

This PR is now the main solution for #124275

[Mark-Simulacrum]

Hm, I see that there was an FCP in #124367 (comment) for these APIs, but I'm wondering if there was discussion already on whether these should return new Arc's or a shared Arc for the type (e.g., Arc with an empty string can share a single static, rather than having to allocate each time).

r? @dtolnay to make the decision whether to re-evaluate FCP or not (just random roll for libs-api)

[zachs18]

Regarding returning new vs shared Arcs, I think the docs could perhaps be updated to say something like

impl Default for (A)Rc<...> {
    /// Creates an empty ... inside an `(A)Rc`.
    ///
    /// This may or may not share an allocation with other `(A)Rc`s (on the same thread (for `Rc`)).
    fn default() -> Self { ... }
}

with the "may or may not" giving wiggle-room to add the "single shared allocation" semantics later (or to decide against it), but without blocking the impls on implementing those semantics now (It'd be doable for the concrete types Arc<str>/Arc<CStr>/etc, but it would be harder to do for Arc<[T]>¹ or any Rc<...>², and also harder to justify the cost).

Footnotes

1. I have some rough draft code that implements the shared allocation semantics for Arc<[T]>, but it wouldn't work in alloc because it uses RwLock which is from std. The concrete type impls could probably just use an AtomicPtr each though, which is available in core. ↩

2. The Rc impls in my code require thread_local!s, as I think any implementation would, so I don't see a way to include any of those in alloc. #[thread_local] is IIUC not available on every platform³. ↩

3. Though I guess if we have the "may or may not" verbiage, then it could be platform-dependent whether or not the single-shared-allocation semantics are implemented. ↩

[Mark-Simulacrum]

I would expect that a shared allocation would use something like the static empty groups in hashbrown. For str for example we'd just have the strong + weak count header in a static EMPTY: Rc/ArcInner<()> or similar, and return EMPTY.clone() from these functions. (Some care would need to be taken to ensure that std itself "owns" one of the reference counts at all times, but that seems easy enough.)

[zachs18]

I hadn't thought of that, but yeah having a static ArcInner<_> would be a much simpler solution than lazily-initailizing a shared pointer. to a dynamic allocation. That also makes it simpler to do for [T], since we can just have a shared static for each alignment up to some threshold, and do the current make-a-new-allocation-on-each-call for anything over-aligned.

@Billy-Sheppard I have a branch with these changes if you'd like to incorporate them into this PR, or perhaps just the first commit with the documentation, and I can submit the implementation as a later PR.

[Billy-Sheppard]

I forgot to push my amended message, unfortunately the broken merge will still show in the PR comments here (apologies for all those that got pinged, I hadn't realised the merge didn't grab the current upstream.

[Billy-Sheppard]

Ah yes I was wondering about that (attributing).

Good call

[dtolnay]

Thanks!

I would be interested to review a followup PR that makes these share the same static ArcInner, instead of 6 different effectively-identical ones. The representation I would expect is a 16-byte-aligned static with the following contents:

+---------------+---------------+-+-----------------------------+
| strong count  |  weak count   |0|          padding            |
+---------------+---------------+-+-----------------------------+

This works as the ArcInner<[T]> for any align_of::<T>() <= 16, and also str and CStr.

[dtolnay]

@bors r+

[bors]

📌 Commit f27d1e1 has been approved by dtolnay

It is now in the queue for this repository.

[bors]

⌛ Testing commit f27d1e1 with merge 496f731...

[bors]

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

[rust-timer]

Finished benchmarking commit (496f731): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

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

Max RSS (memory usage)

Results (primary -2.5%, secondary -2.6%)

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)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-2.6%	[-2.7%, -2.4%]	2
All ❌✅ (primary)	-2.5%	[-2.5%, -2.5%]	1

Cycles

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

Binary size

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

Bootstrap: 670.24s -> 668.657s (-0.24%)
Artifact size: 316.18 MiB -> 316.06 MiB (-0.04%)