[RFC] Add allocators for zero-copy conversion from Box<T> into Rc<T> #80273

mahkoh · 2020-12-21T22:26:13Z

The motivating example is

let mut vec = Vec::new_in(RcAlloc::new());
file.read_to_end(&mut vec)?;
let rc: Rc<[u8]> = vec.into_boxed_slice().into();

But note that read_to_end does not yet support allocators.

rust-highfive · 2020-12-21T22:26:16Z

r? @shepmaster

(rust-highfive has picked a reviewer for you, use r? to override)

mahkoh · 2020-12-22T18:45:15Z

I have a few more changes to StructAlloc

Replacing the type parameter by a const Layout parameter
Thoroughly documenting the safety

but it's essentially complete.

bors · 2020-12-29T03:32:26Z

☔ The latest upstream changes (presumably #80449) made this pull request unmergeable. Please resolve the merge conflicts.

shepmaster · 2021-01-01T20:09:22Z

r? @KodrAus

KodrAus · 2021-01-14T02:58:35Z

cc @rust-lang/wg-allocators I think I'll need a hand reviewing this allocator implementation

CAD97 · 2021-01-14T04:20:49Z

This is a really interesting use of the Allocator API. I need to think a bit more on the implications, but at first overview, the concept seems sound.

JohnCSimon · 2021-02-01T02:01:27Z

Ping from triage
@mahkoh - can you please address the merge conflicts? Thank you.

@rustbot label: -S-waiting-on-review +S-waiting-on-author

mahkoh · 2021-02-01T02:06:37Z

Ping from triage
@mahkoh - can you please address the merge conflicts? Thank you.

Once @CAD97 has performed his review. As I mentioned in my previous comment, there are still some very minor changes I'll make anyway.

KodrAus · 2021-02-10T01:01:54Z

r? @TimDiekmann

(@CAD97 is currently unavailable)

TimDiekmann · 2021-02-10T08:55:28Z

library/alloc/src/alloc/struct_alloc.rs

+    unsafe fn deallocate(&self, data_ptr: NonNull<u8>, data_layout: Layout) {
+        unsafe {
+            let struct_ptr = Self::data_ptr_to_struct_ptr(data_ptr, data_layout);
+            let struct_layout = Self::struct_layout(data_layout).unwrap();


Instead of unwrap we better use except here. When calling deallocate according to the safety section, this should never fail. However, if the method is called wrong by accident, the user should see a useful message.

TimDiekmann · 2021-02-10T09:14:56Z

library/alloc/src/alloc/struct_alloc.rs

+/// When this allocator creates an allocation for layout `layout`, the pointer can be
+/// offset by `-offsetof(Struct, data)` and the resulting pointer points is an allocation
+/// of `A` for `Layout::new::<Struct>()`.
+pub(crate) struct StructAlloc<T, A = Global>(A, PhantomData<*const T>);


This is basically a prefix-allocator, or more generally an affix-allocator. While the crate is a bit outdated due to personal time issues, you might want to take a look at alloc_compose::Affix.

TimDiekmann · 2021-02-10T09:16:15Z

library/alloc/src/alloc/struct_alloc.rs

+    }
+
+    /// Computes the layout of `Struct`.
+    fn struct_layout(data_layout: Layout) -> Result<Layout, AllocError> {


Do you have a specific reason, why you dont' use Layout::extend? See Affix::allocation_layout

At the time I wasn't aware of this function. Either way, this function is written to optimize performance given that we know that the layout of T will be known at compile time.

The compiler is able to optimize out Layout::extend, when T is known at compile time: https://godbolt.org/z/Tx5zv8

Compare https://godbolt.org/z/Mon6j1

In the Rc use case, data_layout is also known at compile time. Anyway, the allocator may be used in other places as well, so it would be better to use Layout::extend and optimize that function instead. I don't see a point in duplicating code logic, when there is a function for that exact use case.

In the Rc use case, data_layout is also known at compile time.

It is not. data_layout is the layout passed to the allocator.

so it would be better to use Layout::extend and optimize that function instead

I do not know if it is possible in general to optimize this function. There are different formulas to calculate padding etc., each optimized for different cases. If the compiler were able to tell the optimizer that the alignment is always a power of two, then those formulas might be optimized identically. But the compiler does not do this at this time. Note that in my optimized function I use two different formulas to minimize the number of runtime-only values.

It is not. data_layout is the layout passed to the allocator.

Yes, but the allocator is only called behind Rc, and the type of T in Rc<T> is known. Actually, I just noticed, StructAlloc::allocate is never called when constructing (A)Rc, which should be changed.

If the compiler were able to tell the optimizer that the alignment is always a power of two and > 0, then those formulas might be optimized identically.

I opened a PR a while ago: #75281 but it wasn't merged, maybe we should try it again.

Yes, but the allocator is only called behind Rc, and the type of T in Rc<T> is known.

T can be a DST.

I opened a PR a while ago: #75281 but it wasn't merged, maybe we should try it again.

I experimented with this a while ago and LLVM did not make use of such assertions.

TimDiekmann · 2021-02-10T09:19:09Z

library/alloc/src/alloc/struct_alloc.rs

+    unsafe fn struct_ptr_to_data_ptr(
+        struct_ptr: NonNull<[u8]>,
+        data_layout: Layout,
+    ) -> NonNull<[u8]> {


Is this working well on ZSTs? We might want a test for this.

TimDiekmann · 2021-02-10T10:21:42Z

While I don't know, how often the example in the OP is used, I really like the change to Rc and Arc, as it moves the logic to a better suited place. When the merge conflicts are solved, we should use a perf-run. Also it would be nice to compare the performance of Layout::extend and the manual implementation. For statically sized types, this should be optimized out.

KodrAus · 2021-02-10T21:39:37Z

@bors try @rust-timer queue

rust-timer · 2021-02-10T21:39:38Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-03-08T18:27:15Z

⌛ Testing commit bad1cda with merge d68e8dd19a7080dd5a26b849837fa1f04151c753...

m-ou-se · 2021-03-08T18:59:36Z

library/alloc/src/rc.rs

+/// let contents: Rc<[u32]> = contents.into_boxed_slice().into();
+/// ```
+#[derive(Debug)]
+#[unstable(feature = "struct_alloc", issue = "none")]


This is missing a tracking issue.

bors · 2021-03-08T22:42:15Z

💥 Test timed out

Dylan-DPC-zz · 2021-03-08T22:53:12Z

@bors retry

rust-log-analyzer · 2021-03-08T22:55:52Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

TimDiekmann · 2021-03-08T23:03:52Z

I'd say r- until a tracking issue is opened? cc @m-ou-se

Amanieu · 2021-03-08T23:22:47Z

@bors r-

crlf0710 · 2021-03-26T15:16:49Z

@mahkoh Ping from triage, would you mind creating a tracking issue as mentioned above? Or would you use some help?

BurntSushi · 2021-04-10T23:30:20Z

Moderation note: mahkoh is no longer participating in the Rust community. It looks like this PR is about ready to merge, so if someone wanted to pick it up where the OP left off, that would be very helpful. Otherwise, we should close this PR.

TimDiekmann · 2021-04-11T01:08:56Z

Thank you for the information!

I'm going to make some small changes and make a separate pull request. I'd leave this open until I open the PR. This will probably be in the next two weeks.

TimDiekmann · 2021-04-19T12:43:39Z

library/alloc/src/alloc/struct_alloc.rs

+            let old_struct_ptr = Self::data_ptr_to_struct_ptr(old_data_ptr, old_data_layout);
+            let new_struct_ptr =
+                self.0.$id(old_struct_ptr, old_struct_layout, new_struct_layout)?;
+            Ok(Self::struct_ptr_to_data_ptr(new_struct_ptr, new_data_layout))


This does not take into account, if the alignment changes. This may result in a different offset between the prefix and the actual data so storing the data may be required. As it's not needed for this use case, I'll skip this for now.

bors · 2021-05-06T07:21:48Z

☔ The latest upstream changes (presumably #84266) made this pull request unmergeable. Please resolve the merge conflicts.

Amanieu · 2021-05-06T13:30:28Z

Closing in favor of #84338

rust-highfive assigned shepmaster Dec 21, 2020

rust-highfive added the S-waiting-on-review label Dec 21, 2020

mahkoh force-pushed the zero-copy-rc branch from adf4df5 to 1892c40 Compare December 21, 2020 22:27

This comment has been minimized.

Sign in to view

mahkoh force-pushed the zero-copy-rc branch from 1892c40 to a89353c Compare December 21, 2020 23:17

This comment has been minimized.

Sign in to view

mahkoh force-pushed the zero-copy-rc branch 3 times, most recently from a46da63 to a136653 Compare December 22, 2020 00:14

rust-highfive assigned KodrAus and unassigned shepmaster Jan 1, 2021

rustbot added S-waiting-on-author and removed S-waiting-on-review labels Feb 1, 2021

TimDiekmann reviewed Feb 10, 2021

View reviewed changes

mahkoh force-pushed the zero-copy-rc branch 2 times, most recently from 9124de7 to 965a688 Compare February 10, 2021 17:45

This comment has been minimized.

Sign in to view

mahkoh force-pushed the zero-copy-rc branch from 965a688 to 5278a35 Compare February 10, 2021 18:01

bors added the S-waiting-on-bors label Mar 8, 2021

m-ou-se reviewed Mar 8, 2021

View reviewed changes

m-ou-se added the T-libs label Mar 8, 2021

bors added S-waiting-on-review and removed S-waiting-on-bors labels Mar 8, 2021

bors added S-waiting-on-bors and removed S-waiting-on-review labels Mar 8, 2021

bors added S-waiting-on-author and removed S-waiting-on-bors labels Mar 8, 2021

TimDiekmann reviewed Apr 19, 2021

View reviewed changes

TimDiekmann mentioned this pull request Apr 19, 2021

Refactor Rc and Arc to use a prefix allocator #84338

Closed

Amanieu closed this May 6, 2021

[RFC] Add allocators for zero-copy conversion from Box<T> into Rc<T> #80273

[RFC] Add allocators for zero-copy conversion from Box<T> into Rc<T> #80273

Conversation

mahkoh commented Dec 21, 2020

rust-highfive commented Dec 21, 2020

This comment has been minimized.

This comment has been minimized.

mahkoh commented Dec 22, 2020

bors commented Dec 29, 2020

shepmaster commented Jan 1, 2021

KodrAus commented Jan 14, 2021

CAD97 commented Jan 14, 2021

JohnCSimon commented Feb 1, 2021

mahkoh commented Feb 1, 2021 • edited Loading

KodrAus commented Feb 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimDiekmann Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimDiekmann Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahkoh Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimDiekmann commented Feb 10, 2021

This comment has been minimized.

KodrAus commented Feb 10, 2021

rust-timer commented Feb 10, 2021

bors commented Mar 8, 2021

Choose a reason for hiding this comment

bors commented Mar 8, 2021

Dylan-DPC-zz commented Mar 8, 2021

rust-log-analyzer commented Mar 8, 2021

TimDiekmann commented Mar 8, 2021

Amanieu commented Mar 8, 2021

crlf0710 commented Mar 26, 2021

BurntSushi commented Apr 10, 2021

TimDiekmann commented Apr 11, 2021

Choose a reason for hiding this comment

bors commented May 6, 2021

Amanieu commented May 6, 2021

mahkoh commented Feb 1, 2021 •

edited

Loading

TimDiekmann Feb 10, 2021 •

edited

Loading

TimDiekmann Feb 10, 2021 •

edited

Loading

mahkoh Feb 10, 2021 •

edited

Loading