Refactor sized Rc/Arc allocation/initialization#152600
Open
alexcrichton wants to merge 1 commit intorust-lang:mainfrom
Open
Refactor sized Rc/Arc allocation/initialization#152600alexcrichton wants to merge 1 commit intorust-lang:mainfrom
alexcrichton wants to merge 1 commit intorust-lang:mainfrom
Conversation
This commit refactors the `Rc` and `Arc` types in the standard library
to have a new narrow waist for all allocations of sized types.
Specifically a new `{Rc,Arc}::from_box_uninit_inner` function is added
which takes an entirely-uninitialized `Box` pointer, initializes the
reference counts, and then returns `Rc`/`Arc` where the payload is
`MaybeUninit<T>`. This is purely an internal refactor of functions at
this time and doesn't expose any new API surface area for either `Arc`
or `Rc`.
It's also worth noting that there remain two primary ways of allocating
`Rc`/`Arc`, like before. Notably allocating `Rc<[T]>` is not possible
with this helper because `MaybeUninit` does not support unsized types at
this time. It's hoped though that in the future if this is supported
that all allocations of `Rc`/`Arc` could be funneled through this
method.
While this refactor was done I went through a number of other
constructors/etc and simplified the implementations to use combinations
of other APIs of `Rc`/`Arc`, removing a number of `unsafe` blocks along
the way.
The overall goal of this refactor is to move towards [this discussion on
Zulip][Zulip]. Eventually the `from_box_uninit_inner` function would
perhaps be exposed as a `From` impl on `Rc` and `Arc`, or maybe a public
inherent function. This would require making the `{Rc,Arc}Inner` types
public, however, so this part is deferred for a future ACP. In the
meantime though I thought that this pattern of initialization was pretty
nifty and would be a good internal refactoring to do in the meantime
regardless.
[Zulip]: https://rust-lang.zulipchat.com/#narrow/channel/197181-t-libs.2Fwg-allocators/topic/Fallible.20allocation.20of.20.60Arc.3CT.3E.60.20w.2Fout.20all.20of.20.60allocator_api.60/near/569793763
rustbot
added
S-waiting-on-review
Collaborator
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Member
Note that this is an opposite direction from #132553 where those types disappear in favour of using a layout that's not expressible in a rust type today so that the counts are always at a consistent offset from the data so that type erasure can work better. |
Member
Author
|
Thanks for linking that! I wasn't previously aware of that, and yeah that would pretty much sink this endeavor. My understanding from reading through the various threads there is that there's no opposition beyond review bandwidth and scale of change there, in the sense that it's likely to happen. If that's true it's probably not worth landing this in the meantime as it would cause unnecessary churn. Assuming others agree I'd still like to land the refactor of |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit refactors the
RcandArctypes in the standard library to have a new narrow waist for all allocations of sized types. Specifically a new{Rc,Arc}::from_box_uninit_innerfunction is added which takes an entirely-uninitializedBoxpointer, initializes the reference counts, and then returnsRc/Arcwhere the payload isMaybeUninit<T>. This is purely an internal refactor of functions at this time and doesn't expose any new API surface area for eitherArcorRc.It's also worth noting that there remain two primary ways of allocating
Rc/Arc, like before. Notably allocatingRc<[T]>is not possible with this helper becauseMaybeUninitdoes not support unsized types at this time. It's hoped though that in the future if this is supported that all allocations ofRc/Arccould be funneled through this method.While this refactor was done I went through a number of other constructors/etc and simplified the implementations to use combinations of other APIs of
Rc/Arc, removing a number ofunsafeblocks along the way.The overall goal of this refactor is to move towards this discussion on Zulip. Eventually the
from_box_uninit_innerfunction would perhaps be exposed as aFromimpl onRcandArc, or maybe a public inherent function. This would require making the{Rc,Arc}Innertypes public, however, so this part is deferred for a future ACP. In the meantime though I thought that this pattern of initialization was pretty nifty and would be a good internal refactoring to do in the meantime regardless.