Attempt to reuse Vec<T> backing storage for Rc/Arc<[T]>#104205
Attempt to reuse Vec<T> backing storage for Rc/Arc<[T]>#104205bors merged 2 commits intorust-lang:masterfrom
Vec<T> backing storage for Rc/Arc<[T]>#104205Conversation
|
r? @thomcc (rustbot has picked a reviewer for you, use r? to override) |
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
|
Cool idea! However, this is unfortunately unsound, since the |
|
This might not actually be very useful, since |
|
Many allocators have some slack, a shrink that only shrinks by a few bytes may not require any reallocation at all. And for large allocations it may be implemented as |
|
This was pretty subtle and tricky to review. It's disappointing that this kind of thing is hard with our allocator APIs... But it looks fine to me. @bors r+ rollup=never |
|
📌 Commit 868c59fdacf6177170d686fdc4ed0a36c7469eef has been approved by It is now in the queue for this repository. |
|
This could use a test that verifies that the allocation gets reused. That'll also allow miri to look at this optimization. |
|
Say, could the same optimization be applied to |
|
Added a test for this behaviour, but I suspect not all platforms will implement |
Co-authored-by: joboet <jonas.boettiger@icloud.com>
|
Also added the optimization to |
|
Definitely uitests are the wrong place for this. Just put it in https://github.com/rust-lang/rust/tree/master/library/alloc/tests somewhere. |
|
@rustbot ready |
|
Looks fine @bors r+ |
|
Need to update title and description, as it now affects Arc too? |
|
@bors p=1 going to close the tree for non-nevers for a while so they can drain out |
Vec<T> backing storage for Rc<[T]>Vec<T> backing storage for Rc/Arc<[T]>
|
☀️ Test successful - checks-actions |
| } | ||
|
|
||
| #[test] | ||
| fn arc_from_vec_opt() { |
Good thing you did that, Miri found a bug. :) Though I think only the test is buggy, not the implementation. |
Revert Vec/Rc storage reuse opt Remove the optimization for using storage added by rust-lang#104205. The perf wins were pretty small, and it relies on non-guarenteed behaviour. On platforms that don't implement shrinking in place, the performance will be significantly worse. While it could be gated to platforms that do this (such as GNU), I don't think it's worth the overhead of maintaining it for very small gains. (rust-lang#104565, rust-lang#104563) cc `@RalfJung` `@matthiaskrgr` Fixes rust-lang#104565 Fixes rust-lang#104563
Attempt to reuse `Vec<T>` backing storage for `Rc/Arc<[T]>` If a `Vec<T>` has sufficient capacity to store the inner `RcBox<[T]>`, we can just reuse the existing allocation and shift the elements up, instead of making a new allocation.
Revert Vec/Rc storage reuse opt Remove the optimization for using storage added by rust-lang#104205. The perf wins were pretty small, and it relies on non-guarenteed behaviour. On platforms that don't implement shrinking in place, the performance will be significantly worse. While it could be gated to platforms that do this (such as GNU), I don't think it's worth the overhead of maintaining it for very small gains. (rust-lang#104565, rust-lang#104563) cc `@RalfJung` `@matthiaskrgr` Fixes rust-lang#104565 Fixes rust-lang#104563
If a
Vec<T>has sufficient capacity to store the innerRcBox<[T]>, we can just reuse the existing allocation and shift the elements up, instead of making a new allocation.