Repository navigation
Make Rc<T>::deref zero-cost - #141348
Make Rc<T>::deref zero-cost#141348EFanZh wants to merge 2 commits into
Rc<T>::deref zero-cost#141348Conversation
This comment has been minimized.
This comment has been minimized.
df34f84 to
d3a7429
Compare
This comment has been minimized.
This comment has been minimized.
bc84ec6 to
19fb34b
Compare
|
The Miri subtree was changed cc @rust-lang/miri |
19fb34b to
f5245ba
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Make `Rc<T>::deref` zero-cost This PR makes `Rc::deref` zero-cost by changing the internal pointer so that it points to the value directly instead of the allocation. This is split out from #132553, which will also make `Arc::deref` zero-cost.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
I have done an initial refactoring, notable points:
I'll update the PR based on the future review feedback. cc @clarfonthey, @joboet, @tgross35 |
This comment has been minimized.
This comment has been minimized.
| pub struct Rc< | ||
| T: ?Sized, | ||
| #[unstable(feature = "allocator_ext", issue = "163177", implied_by = "allocator_api")] A: Allocator = Global, | ||
| > { | ||
| ptr: NonNull<RcInner<T>>, | ||
| phantom: PhantomData<RcInner<T>>, | ||
| ptr: RcValuePointer<Header, T>, | ||
| phantom: PhantomData<T>, | ||
| alloc: A, | ||
| } |
There was a problem hiding this comment.
To avoid needing to specify Header everywhere, could you put these types/aliases in common/mod.rs?
// common/mod.rs
#[repr(align(...))]
#[repr(C)]
pub(crate) struct Header<C> {
pub(crate) weak: C,
pub(crate) strong: C,
}
pub(crate) type RcHeader = Header<Cell<usize>>;
// pub(crate) type ArcHeader = Header<Atomic<usize>>;| /// A `Layout` that describes a reference-counted allocation containing a header of type `H`. | ||
| pub(in crate::rcs) struct RcLayout<H>(Layout, PhantomData<H>); |
There was a problem hiding this comment.
I still think we can eliminate RcLayout, it pretty much exists only for use within rc_alloc.rs. With the design at #141348 (comment), the free functions in rc_alloc are instead implemented on RefCounts (renamed to Header here / after https://github.com/rust-lang/rust/pull/141348/changes#r4190716209), which can use Self to get the layout. You'll still need the layout computation functions but they can just be free functions in rc_alloc that are mostly private, similar to what exists on main.
UniqueRc is the only snag but rc_layout is only used for deallocation. It should be fine if that one case calls A::deallocate on its own without using the bits in common.
There was a problem hiding this comment.
Unlike RcValuePointer, RcLayout is indeed used as a safety contract. For allocation, we usually calculate the allocation layout from the value layout; then we only need a type-erased function that allocates from the allocation layout:
fn allocate<H, A>(alloc: &A, layout: RcLayout<H>) -> ErasedRcValuePointer<H> { ... }
We choose to return a pointer to the value instead of the allocation because this is usually what the caller wants. The caller usually won't need the allocation pointer until deallocation. Here RcLayout<H> is guaranteed to support the header H, so we can safely perform pointer arithmetic to get a pointer to the value.
If we drop the RcLayout type, we have to either mark the function unsafe or return a pointer to the allocation and let the caller calculate the pointer to the value. The first approach will introduce a lot of unsafe code and corresponding justifications at each call site; the second approach can cause code duplication at call sites for pointer calculations; and both require the caller to keep track of what kind of layout they are passing.
I also considered passing a value layout instead of the allocation layout, but that does not work very well because the underlying allocator API requires allocation layouts, so we have to obtain an allocation layout at some point; so functions that take allocation layouts arise naturally. Also, decoupling layout calculations from memory operations makes the code more flexible; an example is the cached layout in UniqueRcUninit. Although I am not sure whether the cache here has a performance benefit, at least we have a choice when we do need it.
There was a problem hiding this comment.
I also considered passing a value layout instead of the allocation layout, but that does not work very well because the underlying allocator API requires allocation layouts, so we have to obtain an allocation layout at some point; so functions that take allocation layouts arise naturally.
This is the preferred solution: nothing in rc.rs/arc.rs should care about the actual allocated layout, that can all be internal to the common module. Currently RcLayout is pretty viral.
Taking the API from the linked comment (with some fixes):
impl<C> Header<C> {x
// The `_type` functions can be `#[inline]` and implemented in terms of `*_layout`.
pub(crate) fn allocate_for_type<T, A: Allocator>(alloc: A) -> RcValuePointer<T> {}
pub(crate) fn allocate_for_layout<A: Allocator>(value_layout: Layout, alloc: A) -> ErasedRcValuePointer {}
pub(crate) fn try_allocate_for_type<T, A: Allocator>(alloc: A) -> Result<RcValuePointer<T>, AllocError> {}
pub(crate) fn try_allocate_for_layout<A: Allocator>(value_layout: Layout, alloc: A) -> Result<ErasedRcValuePointer, AllocError> {}
// ...
// I updated these here
pub(crate) unsafe fn deallocate_for_type<T, A: Allocator>(ptr: RcValuePointer<T>, alloc: A) {}
pub(crate) unsafe fn deallocate_for_layout<A: Allocator>(ptr: ErasedRcValuePointer, value_layout: Layout, alloc: A) {}
// Maybe this can actually return `&Header` since `RcValuePointer` has a validity invariant?
pub(crate) fn get(ptr: RcValuePointer) -> NonNull<Header<C>> {}
}All the Layouts there are for T: only try_allocate_for_layout, deallocate_for_layout, and get actually need to know about the true allocated Layout and they can do it internally. The first two can call a shared fn allocated_layout(value_layout: Layout).
value_layout is the layout of the entire data other than the header, e.g. for [T] it should be the layout for [T; N] that actually needs to get allocated. This is the layout that UniqueRcUninit can store.
| struct UniqueRcUninit<T: ?Sized, A: Allocator> { | ||
| ptr: NonNull<RcInner<T>>, | ||
| layout_for_value: Layout, | ||
| alloc: Option<A>, | ||
| ptr: RcValuePointer<Header, T>, | ||
| rc_layout: RcLayout<Header>, | ||
| alloc: A, | ||
| } |
There was a problem hiding this comment.
Why does UniqueRcUninit hold a layout at all? It's only used in drop, and I think that can be recomputed from T same as we do on UniqueRc. Perhaps it's related to allowing shared drop to reduce code size, but it doesn't seem worth optimizing this struct in this way but not the rest of the Arc/Rc types.
If you are interested, I think you could submit a separate PR changing this to match what is done in UniqueRc, which may make things a bit more simple for this PR.
There was a problem hiding this comment.
It is not necessary. I kept it to match the existing implementation. Changing it would have an unknown performance impact, so I prefer to leave it out of the scope of the current PR.
There was a problem hiding this comment.
I noticed you dropped the layout as part of this PR now—I don't think it will make all that much of a difference but your consideration about perf was reasonable. if you're interested, feel free to create a separate PR changing the existing implementation, which we can perf and merge separately.
There was a problem hiding this comment.
I removed the cached layout because:
- To my understanding, you prefer to not expose the allocation layout to
RcandArcimplementations. - Caching the value layout is not very useful because we have to compute the allocation layout again when dellocation happens.
So to my understanding, either we remove the cached layout, or we expose the allocation layout and cache this one instead.
I kept RcLayout because I found out that the "passing value layout" approach has another defect of not being able to compute the allocation layout at compile time. We could rely on compiler optimization, but it is not guaranteed. I also added a value_offset field to RcLayout because it is a by-product from computing allocation layout and is used whenever an RcLayout is used.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
There was a problem hiding this comment.
Many of the functions in this module are now called a single time in common/mod.rs. Can you inline any like this? The few remaining functions that are called >1 time can also just move into common/mod.rs.
There was a problem hiding this comment.
I checked each function in rc_alloc, only allocate_from_bytes and try_allocate_with are called a single time. allocate_from_bytes is introduced to erase the concrete value types, try_allocate_with is introduced to exist as a fallible counterpart of allocate_with.
| let allocation_result = alloc.allocate(rc_layout.layout()); | ||
|
|
||
| allocation_result.map(|allocation_ptr| { | ||
| // SAFETY: `allocation_ptr` is allocated with `rc_layout`, so we can safely acquire an | ||
| // pointer to the value. | ||
| ErasedRcValuePointer::new(unsafe { | ||
| allocation_ptr.cast().byte_add(rc_layout.value_offset()) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
| let allocation_result = alloc.allocate(rc_layout.layout()); | |
| allocation_result.map(|allocation_ptr| { | |
| // SAFETY: `allocation_ptr` is allocated with `rc_layout`, so we can safely acquire an | |
| // pointer to the value. | |
| ErasedRcValuePointer::new(unsafe { | |
| allocation_ptr.cast().byte_add(rc_layout.value_offset()) | |
| }) | |
| }) | |
| let ptr = alloc.allocate(rc_layout.layout())?; | |
| // SAFETY: `allocation_ptr` is allocated with `rc_layout`, so we can safely acquire an | |
| // pointer to the value. | |
| ErasedRcValuePointer::new(unsafe { | |
| ptr.cast().byte_add(rc_layout.value_offset()) | |
| }) |
? avoids some messier map calls. Applies a handful of places.
|
I'll have more feedback but let's see where this is on perf @bors try |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (2e6b3c1): 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 @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.2%, secondary 3.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -0.0%, secondary 5.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary 7.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 489.893s -> 489.388s (-0.10%) |
This comment has been minimized.
This comment has been minimized.
|
@tgross35: I added some |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (baeaed6): 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 @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.0%, secondary 2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 5.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary 6.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 489.893s -> 488.069s (-0.37%) |
|
Hopefully rust-lang/rustc-perf#2615 will give some better hints about exactly where things are slowing down. |
There was a problem hiding this comment.
So you can run perf yourself and experiment,
@bors delegate=try
I know the patterns in the current code have been optimized about as much as possible, any kind of perturbation may easily have negative effects on performance. The best thing to do is probably to slowly walk back changes to make the code's shape look more like what's currently there. If that means moving more out of common.rs and back into rc.rs then that's fine, more refactoring can happen after the two big PRs merge.
| pub fn new_uninit() -> Rc<mem::MaybeUninit<T>> { | ||
| // ignore-tidy-undocumented-unsafe | ||
| unsafe { | ||
| Rc::from_ptr(Rc::allocate_for_layout( | ||
| Layout::new::<T>(), | ||
| |layout| Global.allocate(layout), | ||
| <*mut u8>::cast, | ||
| )) | ||
| } | ||
| Self::new_uninit_in(Global) | ||
| } |
There was a problem hiding this comment.
Could you try adding this pattern back with allocate_for_layout and try_allocate_for_layout? With API in common:
fn rc_inner_layout_for_value_layout(layout: Layout) -> Layout;
impl RcHeader {
unsafe fn allocate_for_layout(
value_layout: Layout,
allocate: impl FnOnce(Layout) -> Result<NonNull<[u8]>, AllocError>,
mem_to_rc_inner: impl FnOnce(ErasedRcValuePointer) -> RcValuePointer<T>,
) -> RcValuePointer<T>;
}I think we may be spending more time/complexity doing layout calculation and allocation while the existing pattern works quite well.
There was a problem hiding this comment.
I inspected the generated assembly locally, I think the majority difference is caused by different inlining strategy and extra pointer offset operations. Most of the test cases use sized types, I ensured the layouts are computed at compile time, so the layout calculation are not likely the main cause.
I am now trying different optimization strategies to see if they improve the perf result.
|
✌️ @EFanZh, you can now perform try builds on this pull request! Warning You used the legacy format of the delegate command. You can now post |
|
@bors try @rust-timer queue |
|
Insufficient permissions to issue commands to rust-timer. |
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #164230) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
View all comments
This PR makes
Rc::derefzero-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::derefzero-cost.Review status:
RefCountsandRcLayouttypesRefCountertraitRawWeaktypeRawWeakmethods for sized valuesRawWeakmethods for slice valuesRawWeakRawRctypeRawRcmethods for sized valuesRawRcmethods forMaybeUninit<T>valuesRawRcmethods for slice valuesRawRcmethods fordyn AnytypeRawRcRawUniqueRctypeRawUniqueRcmethods for sized valuesRawUniqueRcalloc::rc::{Rc,Weak,UniqueRc}withalloc::raw_rctypesRcimplementation