Make Rc<T>::deref zero-cost

[EFanZh]

View all comments

This PR makes Rc::deref zero-cost by changing the internal pointer to point directly to the value instead of to the allocation.

This PR is split from #132553, which will also make Arc::deref zero-cost.

Review status:

• 8d8878a Add RefCounts and RcLayout types
• abe9c27 Add allocation functions
• cfa6ea8 Add RefCounter trait
• 77755f0 Add RawWeak type
• 7fce1c4 Add RawWeak methods for sized values
• adddf22 Add RawWeak methods for slice values
• 99522fc Implement necessary traits for RawWeak
• 393e377 Add RawRc type
• 6cb537c Add RawRc methods for sized values
• 6b47771 Add RawRc methods for MaybeUninit<T> values
• 9184466 Add RawRc methods for slice values
• 33881a5 Add RawRc methods for dyn Any type
• 17e84c6 Implement necessary traits for RawRc
• 20e0aba Add RawUniqueRc type
• 82525d0 Add RawUniqueRc methods for sized values
• 8bc9dea Implement necessary traits for RawUniqueRc
• cee99e8 Implement alloc::rc::{Rc,Weak,UniqueRc} with alloc::raw_rc types
• 2676c63 (HEAD -> zero-cost-rc-deref) Add LLVM codegen tests for new Rc implementation

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[oli-obk]

@bors try @rust-timer queue

[bors]

⌛ Trying commit f5245ba with merge 8ef4a25...

[bors]

☀️ Try build successful - checks-actions
Build commit: 8ef4a25 (8ef4a25b05973cfbd577205c507a891d07f0ae5f)

[rust-timer]

Finished benchmarking commit (8ef4a25): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.2%, 0.7%]	4
Regressions ❌ (secondary)	1.1%	[0.2%, 2.5%]	9
Improvements ✅ (primary)	-1.0%	[-2.0%, -0.5%]	7
Improvements ✅ (secondary)	-1.3%	[-2.0%, -0.1%]	4
All ❌✅ (primary)	-0.5%	[-2.0%, 0.7%]	11

Max RSS (memory usage)

Results (primary -0.3%, secondary -0.0%)

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)	8.4%	[8.4%, 8.4%]	1
Regressions ❌ (secondary)	3.1%	[2.0%, 4.8%]	6
Improvements ✅ (primary)	-4.6%	[-7.1%, -2.1%]	2
Improvements ✅ (secondary)	-4.7%	[-6.4%, -2.9%]	4
All ❌✅ (primary)	-0.3%	[-7.1%, 8.4%]	3

Cycles

Results (primary -0.5%, 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.9%	[0.9%, 0.9%]	1
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	2
Improvements ✅ (primary)	-1.9%	[-1.9%, -1.9%]	1
Improvements ✅ (secondary)	-3.9%	[-9.3%, -1.5%]	4
All ❌✅ (primary)	-0.5%	[-1.9%, 0.9%]	2

Binary size

Results (primary 0.2%, secondary 1.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.3%	[0.0%, 1.8%]	29
Regressions ❌ (secondary)	1.8%	[0.0%, 6.4%]	9
Improvements ✅ (primary)	-0.3%	[-0.6%, -0.1%]	5
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.4%]	1
All ❌✅ (primary)	0.2%	[-0.6%, 1.8%]	34

Bootstrap: 775.728s -> 776.531s (0.10%)
Artifact size: 366.25 MiB -> 366.33 MiB (0.02%)

[apiraino]

Would anyone comment the perf. run? cc @EFanZh thanks!

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[tgross35]

Hey @EFanZh, we're sorry this has continued to drag on. The libs team is very interested in the change, it's just so expansive that finding somebody able to review it has been difficult when we were short on reviewer bandwidth. But, things have been improving! A few of us got together and did a group review, and I've collected the feedback here. So, thank you for persisting with this despite how long it has been open.

The biggest thing is: we decided that the downsides of making things generic over Rc and Arc outweigh its benefits. It is an interesting concept but the status quo of "method in rc/arc module"->implementation` turned into "method in rc/arc module"->generic call->trait calls->"trait impls in rc/arc module"—the logic is largely in the same place, but there's a lot more mental overhead to see what's going on. We're thinking about ability to review, but also future readability and maintainability.

Most of the issue there is with the RefCounter trait. It seems like there are some other things that may remain generic (more on that in the separate issues) but we'd prefer if that could be geared toward commonizing identical functionality such as allocation.

We put together a more concrete counterproposal, which I've broken down a bit in threaded comments. The specific files aren't meaningful, it's just for threads to allow separate followup discussion.

So, apologies again for the back and forth and delays here. The hope is that the new proposed changes reuse enough of the your logic that it will be reasonably easy to pretty much inline things that are currently dispatched to traits, while significantly shrinking the diff and making things easier to follow.

View changes since this review

[tgross35]

Ideally we would like things to be structured in a way that forces more encapsulation. There's a lot of pub(crate) that would be nice to reduce scope to pub(super) or completely private.

Would you be able to put together a separate PR to land before this one, restructuring things to the following?

// alloc

mod rcs {
    pub mod rc;
    mod arc;
    // to be added in this PR
    // mod common; 
}

mod sync {
    // Reexport to keep current public structure
    pub use crate::rcs::arc::...;
}

pub use rcs::rc;

That way things in the mod common can be made pub(super) rather than pub(crate) and still used with both Arc and Rc.

Note that if you do this, make it three commits that do the following:

1. Create a rcs module and move rc there.
2. Rename library/alloc/src/sync.rs to library/alloc/rcs/arc.rs. Just a mv, things won't build after this commit but that's fine.
3. Move the non-arc bits from arc.rs back to library/alloc/src/sync.rs.

That's a git trick to preserve its history and blame, since the vast majority of sync.rs is Arc. If you instead moved Arc from sync.rs to arc.rs in a single commit while sync.rs still exists, git would see it as a fresh addition with no history, and anybody with open PRs changing Arc wouldn't be able to automatically rebase..

[EFanZh]

Yes, I can submit a PR for it with the git trick in a few days.

[clarfonthey]

Feel free to explicitly request me (@clarfonthey), @joboet, or @tgross35 as a reviewer for those PRs since we were the ones doing the shared review. The syntax is r? username in the description. In general, happy to merge the more trivial, "easy" refactoring bits for this PR before merging the final version.

[tgross35]

Counterproposed module for items that are shared between Arc and Rc:

// alloc::rcs::common;

#[repr(align(...))]
#[repr(C)]
pub(crate) struct RefCounts<C /* no trait */> {
    pub(crate) weak: C,
    pub(crate) strong: C,
}

pub(crate) type RcRefcounts = RefCounts<Cell<usize>>;
pub(crate) type ArcRefcounts = RefCounts<Atomic<usize>>;

// through these types, it's safe to access both refcounts and T. Can have some
// minimal API to get a pointer to `T`.
pub(crate) struct RcValuePointer<T>(pub(crate) NonNull<T>);
pub(crate) struct ErasedRcValuePointer(pub(crate) NonNull<()>);

// Used as a template for the original allocation
#[repr(C)]
struct AllocLayout<C, T> {
    refcounts: RefCounts<C>,
    data: T
}

impl<C, A: Allocator> RefCounts<C, A> {x
    // The `_type` functions can be `#[inline]` and implemented in terms of `*_layout`.
    pub(crate) fn allocate_for_type<T>(alloc: A) -> RcValuePointer<T> {}
    pub(crate) fn allocate_for_layout(value_layout: Layout, alloc: A) -> ErasedRcValuePointer {}
    pub(crate) fn try_allocate_for_type<T>(alloc: A) -> Result<RcValuePointer<T>, AllocError> {}
    pub(crate) fn try_allocate_for_layout(value_layout: Layout, alloc: A) -> Result<ErasedRcValuePointer, AllocError> {}
    pub(crate) unsafe fn deallocate<T>(ptr: RcValuePointer<T>, alloc: A) {}
    pub(crate) unsafe fn get(ptr: RcValuePointer) -> NonNull<RefCounts<C>> {}
}

The main thing here is we'd like Rc and Arc to have a common structure on the heap, which means they should be able to share layout and inner allocation logic. We would like RcLayout to be removed and just inline its logic.

RcLayout and RcLayoutExt can also be removed in favor of allocating an AllocLayout. The existing version doesn't have any special handling for types that are right on the border of max possible layout but are pushed over the edge by refcounts, we just let the compiler handle that (https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=b301e48f15f6333cea76dc91cfb8f6cd).

[EFanZh]

• The current implementation uses a single RefCounts type for both Arc and Rc so they can share the exact same memory operation functions, do you prefer two set of memory operation functions for Arc and Rc?
• The current implementation employs memory layout strategy from [DRAFT] Rc: allow deduping both deref and clone across types #133061, which removes possible padding between reference counters and the contained value so that the offset between them can be a compile time constant independent of the type of the contained value. Operations like clone can use the same code for different types of contained value. This layout also make debugger pretty printers easier to write, do you prefer we dropping this strategy or keeping it?
• The current RcLayout is a checked Layout that includes the reference counter data. AllocLayout requires T, but many allocation functions can be done relying only on the allocation layout, not the concrete T type, I consider this a similar role as RcValuePointer.

[tgross35]

• The current implementation uses a single RefCounts type for both Arc and Rc so they can share the exact same memory operation functions, do you prefer two set of memory operation functions for Arc and Rc?

Yes please: we came to the conclusion that being able to follow the code outweighs deduplicating code of a similar (but not exact) shape. Or at least, we're more worried about bugs that would hide in the trait complexity when atomic ops get tricky than we are about bugs from mismatches between Arc and Rc.

In particular, the ops should largely remain in the Rc/Arc methods like they are prior to this PR, which will cut the diff down by a lot.

• The current implementation employs memory layout strategy from [DRAFT] Rc: allow deduping both deref and clone across types #133061, which removes possible padding between reference counters and the contained value so that the offset between them can be a compile time constant independent of the type of the contained value. Operations like clone can use the same code for different types of contained value. This layout also make debugger pretty printers easier to write, do you prefer we dropping this strategy or keeping it?

Thanks for pointing this out, that does make sense to keep then.

• The current RcLayout is a checked Layout that includes the reference counter data. AllocLayout requires T, but many allocation functions can be done relying only on the allocation layout, not the concrete T type, I consider this a similar role as RcValuePointer.

I don't think it's needed: a function like allocate_for_layout that returns an erased pointer can expect that Layout is for the data. It just needs to call something like this to get the layout it actually allocates

rust/library/alloc/src/rc.rs

Lines 292 to 302 in e5b9509

	fn rc_inner_layout_for_value_layout(layout: Layout) -> Layout {
	// Calculate layout using the given value layout.
	// Previously, layout was calculated on the expression
	// `&*(ptr as *const RcInner<T>)`, but this created a misaligned
	// reference (see #54908).
	Layout::new::<RcInner<()>>()
	.extend(layout)
	.unwrap_or_else(|_| panic!("capacity overflow"))
	.0
	.pad_to_align()
	}

.

I suppose there's no point to keeping AllocLayout, in any case.

[clarfonthey]

Right, essentially the proposal here was to inline all the various calls RcLayout uses to compute the correct layout directly into the allocation methods instead of keeping them separate. You can still separate out some logic if it's shared between these, but that can just be a private function in the same module, and doesn't need any extra types.

[clarfonthey]

Also, if I recall, when you first started working on this, const { … } block syntax wasn't available, so, maybe you can use that for computing layouts that depend solely on T to maybe help optimization a bit better, if you're still interested in that?

[EFanZh]

@clarfonthey: About the const block, I am not entire sure what you were referring to. Do you mean using it to replace the RcLayoutExt::RC_LAYOUT constant, the specialization for sized type or something else?

[tgross35]

I think you still need some specialization, but that can remain the preexisting RcFromSlice. const { ... } can likely replace the need for RC_LAYOUT though, since you can just call rc_inner_layout_for_value_layout in a const block.

Note that of course, there are probably some things we'll have to check back in on after seeing a new draft.

[tgross35]

Counterproposal for the pointer types:

// alloc::rcs::rc;

/* These change the pointer but otherwise remain unchanged */

pub struct Rc<T: ?Sized, A: Allocator = Global> {
    ptr: RcValuePointer<T>, // used to be `NonNull<RcInner<T>>`
    // Add comment: Required because `Drop` uses `#[may_dangle]`, see
    // https://doc.rust-lang.org/nomicon/phantom-data.html#an-exception-the-special-case-of-the-standard-library-and-its-unstable-may_dangle
    phantom: PhantomData<T>,
    alloc: A,
}

pub struct Weak<T: ?Sized, A: Allocator = Global> {
    ptr: RcValuePointer<T>, // used to be  `NonNull<RcInner<T>>`
    alloc: A,
}

pub struct UniqueRc<T: ?Sized, A: Allocator = Global> {
    ptr: RcValuePointer<T>, // used to be  `NonNull<RcInner<T>>`
    /// Define the ownership of `RcInner<T>` for drop-check
    _marker: PhantomData<T>,
    /// Invariance is necessary for soundness: once other `Weak`
    /// references exist, we already have a form of shared mutability!
    _marker2: PhantomData<*mut T>,
    alloc: A,
}

Since the important methods on RawRc, RawWeak, and RawUniqueRc mostly need to dispatch via the RefCounter trait which is going away, we would like to inline these. If there are repeated operations, they can be free functions in the common module (or in rc if they're not shared across rc/weak/unique but not useful for Arc).

The other thing with this is that after the change, we're likely going to move A into the RefCounts struct rather than as part of the pointer, so there won't really be anything duplicated.

(Note to others in the meeting: we didn't talk about keeping rcvaluepointer but after some thought, I considered it worth it for type safety (knowing it's safe to access a counters before T or not).)

[EFanZh]

Some non-trivial operations are similar between Arc and Rc except for reference counting methods, like make_mut and try_map, the current implementation generalizes these operations and uses RefCounter trait to inject reference counting specific operations. If we remove the trait, the non-reference counting logic have to be duplicated in Arc and Rc, what do you think we should do for these methods?

[tgross35]

That's fine and expected, it's the status quo. make_mut is pretty different anyway, RawRc just glues the two implementations together with MakeMutStrategy.

Maybe there's a case for deduplicating try_map and some other things since it's nearly the same in four places, but it's not worth doing that as part of this PR. Could use either callbacks or a macro for the body rather than the traits. too.

[EFanZh]

The current implementation implements (Unique)Rc with Weak, which makes certain drop guards easier to construct. Also I remember that someone once proposed the ability to borrow Weak out of Rc without actually doing reference count operations, although I can't find it now. Doing this will also making that proposal trivial to implement in case someone brings up it again. Do you want to keep this structure?

[clarfonthey]

One big point that came up in review is that the current approach basically replaces every single method on both types with a single function call, and since all of the methods have to be directly implemented by traits, this doesn't actually save anything extra besides just moving the code around. And besides, considering how there are only two of these types, and not any more, it's not really worth avoiding all that duplication if it massively increases the size of the code.

Maybe in the future, if we add more reference-counted types like ones that omit the option for weak references, it might be worth doing that generalization, but at least for now, it's not.

[EFanZh]

@clarfonthey, @tgross35: Can I have your opinions on implementing (Unique)Rc with Weak as described above?

[tgross35]

+1 to all that Fon said, but to frame it a different way: we'd like this PR to focus only on the layout changes. There's code that you may as well share to do that, like the allocation code. But let's keep any refactoring beyond that in followups to keep this PR easy to follow, and know exactly what is contributing to perf results we see.

[tgross35]

Sorry, didn't see your note to respond specifically before commenting. I'd say no, don't replace them with Weak. If there is shared logic for dropping then it can be a free function but there's no need to do it here. Especially since it might wind up needing to change again once the A moves into the allocation.

[tgross35]

Keep the main logic in the methods in the methods/functions in rc.rs. It will of course need to be updated for the new structure and to use what's in common, but it's going to make a world of difference in reviewability if the diff shows us the logic changes.

If there's value in splitting up weak/unique to different modules, that can be done as a followup.

[tgross35]

Prior to requested changes

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: c9e0a81 (c9e0a81eeece594c9f437495edde27f954a36c33)
Base parent: c64de18 (c64de18c55c70d62aa88a5722dea8f8e263bacf3)

[rust-timer]

Finished benchmarking commit (c9e0a81): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.5%, 0.7%]	2
Regressions ❌ (secondary)	1.1%	[0.3%, 3.9%]	13
Improvements ✅ (primary)	-0.5%	[-0.9%, -0.2%]	3
Improvements ✅ (secondary)	-1.1%	[-1.7%, -0.5%]	2
All ❌✅ (primary)	-0.1%	[-0.9%, 0.7%]	5

Max RSS (memory usage)

Results (primary -2.8%, secondary 2.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	5.6%	[1.8%, 8.6%]	3
Improvements ✅ (primary)	-4.1%	[-8.9%, -2.4%]	4
Improvements ✅ (secondary)	-1.9%	[-2.8%, -0.9%]	2
All ❌✅ (primary)	-2.8%	[-8.9%, 2.3%]	5

Cycles

Results (primary -2.7%, secondary 1.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.9%	[2.7%, 3.1%]	3
Improvements ✅ (primary)	-2.7%	[-2.7%, -2.7%]	1
Improvements ✅ (secondary)	-1.8%	[-2.5%, -1.0%]	2
All ❌✅ (primary)	-2.7%	[-2.7%, -2.7%]	1

Binary size

Results (primary -0.3%, secondary 1.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.2%, 0.5%]	4
Regressions ❌ (secondary)	7.3%	[7.0%, 8.3%]	4
Improvements ✅ (primary)	-0.7%	[-1.0%, -0.3%]	7
Improvements ✅ (secondary)	-5.0%	[-6.5%, -0.6%]	4
All ❌✅ (primary)	-0.3%	[-1.0%, 0.5%]	11

Bootstrap: 488.215s -> 487.686s (-0.11%)
Artifact size: 406.35 MiB -> 407.14 MiB (0.19%)

[EFanZh]

@tgross35: If there are later perf runs, I think they should be done on #132553, see #141348 (comment).

[tgross35]

Started one there too. But these are still relevant, no? The PR isn't unsharing any code so, unless I'm missing something, the regressions are real regressions that happen to be won back by the Arc changes. (Possibly related to inlining with the extra calls.)

[EFanZh]

@tgross35: I agree. Previously, if two perf run happen with a long interval in between, the results tend to drift, could be due to refactoring or toolchain updating in the middle. Usually certain calibrations (like inlining) needs to be done against the perf result, which I haven't done any on this round.

[tgross35]

Well, #132553 (comment) is also interesting. Don’t think it’s worth digging into until the next version though, I’m sure we can win back whatever regressed.