Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Support DSTs #209

Closed
wants to merge 8 commits into from
Closed

Conversation

jeehoonkang
Copy link
Contributor

NOTE: this is a copy of crossbeam-rs/crossbeam-epoch#87

Currently, crossbeam-epoch doesn't support dynamically-sized types (DST): https://github.com/crossbeam-rs/rfcs/blob/master/text/2017-05-02-atomic-api.md#fat-pointers-and-dsts

This PR adds the support for DST by generalizing Box<T>, which was used as the underlying storage type for atomic pointers, to the Storage<T> trait. For more details, see the comment on the Storage trait. As an example, I added support for dynamically-sized arrays. Other kinds of DSTs can be supported by implementing Storage<T>, even in the client code.

This PR was motivated by the fact that there's an unnecessary indirection in crossbeam-deque when accessing its underlying buffer. As a proof of concept, I removed that indirection on top of PR in my local development: concurrent-circbuf.

This is a breaking change, but I tried to keep it as small as possible. As far as I understand, the only breaking change is renaming Atomic::from(*const T) into Atomic::from_raw(*const T) and Shared::from(*const T) into Shared::from_raw(*const T). (Question: I ended up changing the underlying types. Is it a breaking change?)

This is a WIP. Probably we want to discuss this PR's design and even write up an RFC. I just share it for starting a discussion. Thanks!

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to write a long answer, but then realized that maaaaybe there's a cleaner way to support DSTs without changing the public API or introducing any new types at all. We'd only generalize struct Atomic<T> to struct Atomic<T: ?Sized> and that's it. Same with Shared and Owned. I'll give it some more thought and then reply with another comment.

By the way, there are two important RFCs to watch:

I expect in 2019 we'll get new Rust features (unsized rvalues and possibly a way to get metadata from a DST pointer) that will simplify DSTs for us a whole lot. Whatever we do today to support DSTs should be compatible with the upcoming language-level features. Just something to think consider.

crossbeam-epoch/src/storage/array_storage.rs Outdated Show resolved Hide resolved
@jeehoonkang
Copy link
Contributor Author

@stjepang I'm looking forward to reading your long answer :)

Changing Atomic<T> to Atomic<T: ?Sized> is the first thing I tried, but I couldn't manage to get it done. The main reason is that Atomic<T> should really be a word---in order to support atomic operations---but Box<T> does not fit in a word when T is not Sized. In order to make Atomic<T> a word, the metadata (e.g. size, vtable, ...) should reside in the memory allocation, as discussed in https://github.com/crossbeam-rs/rfcs/blob/master/text/2017-05-02-atomic-api.md#fat-pointers-and-dsts. Do you think we can work-around this issue?

@ghost ghost removed the discuss label Nov 8, 2018
@ghost
Copy link

ghost commented Nov 8, 2018

The main reason is that Atomic<T> should really be a word---in order to support atomic operations---but Box<T> does not fit in a word when T is not Sized.

Right. So perhaps we could do the following.

If T is a sized type, allocate the value on the heap as usual.

If T is unsized, allocate on the heap space for (&T, T). Then we write the value into the second field, and a pointer to it into the first field. It could be implemented roughly like this:

impl<T: ?Sized> Owned<T> {
    fn new(value: T) -> Owned<T> { // Assumes feature `unsized_locals` is available.
        unsafe {
            if is_sized::<T>() {
                // TODO: Allocate as usual.
            } else {
                let (layout, offset) = Layout::<&T>::new().extend(Layout::for_value(&value)).unwrap();
                let dest = Layout::alloc(layout);

                // Make a fat pointer to `value`.
                let mut ptr = &value as *const T;
                // Change the address field in `ptr` to point to `dest.add(offset)`.
                let ptr_ptr = &mut ptr as *mut _ as *mut usize;
                ptr_ptr.write(dest.add(offset) as usize);

                // TODO: Write `value` into `dest.add(offset)`.
                // TODO: Write `ptr` into `dest`.

                Owned {
                    data: dest as usize,
                    _marker: PhantomData,
                }
            }
        }
    }
}

Now, in order to dereference an Owned<T>, we check if T is sized. If it is sized, we dereference as usual. If it is unsized, we load the *const T stored at address self.data and just dereference it.

The smallbox crate uses similar trickery here.

@jeehoonkang
Copy link
Contributor Author

I think that's a great idea. Do you know if it's implementable in today's stable? If so, I'll try to get it done.

@ghost
Copy link

ghost commented Dec 11, 2018

Do you know if it's implementable in today's stable?

I believe it is. Just a tip: you might need this function when implementing DST support:
rust-lang/rust#55785

@jeehoonkang
Copy link
Contributor Author

@stjepang I think we cannot implement is_sized::<T>() in today's stable, and in fact, also in today's nightly. My Google search didn't give me any useful references...

@ghost
Copy link

ghost commented Dec 23, 2018

@jeehoonkang You could hack it with size_of::<&T>() == size_of::<usize>(). :)

@jeehoonkang
Copy link
Contributor Author

jeehoonkang commented Dec 23, 2018

@stjepang Thanks for for hack :)

I tried to dig a little bit more, and here is my initial thought:

  • I think it's not impossible to implement your idea in Nightly. It's definitely impossible in today's stable because it requires several Nightly features.

  • It'll be tricky to implement Owned::into_box(). We have to use some tricky specialization for sized types in order to use Box::new() for sized types.

    Now I wonder if box x can be deprecated in the Rust compiler source code, because it can be replaced with the Layout API. If so, into_box() can easily be implemented.

  • I'm not sure your ptr_ptr trick has a defined meaning in Rust. Is it guaranteed that the pointer part is always at the beginning of fat pointers?

@ghost
Copy link

ghost commented Dec 24, 2018

I think it's not impossible to implement your idea in Nightly. It's definitely impossible in today's stable because it requires several Nightly features.

True. We'd have to wait until unsized locals get stabilized.

It'll be tricky to implement Owned::into_box(). We have to use some tricky specialization for sized types in order to use Box::new() for sized types.

I'd be okay with supporting into_box() only when T: Sized until we get language-level support for that.

I'm not sure your ptr_ptr trick has a defined meaning in Rust. Is it guaranteed that the pointer part is always at the beginning of fat pointers?

It isn't, the layout of DSTs hasn't been stabilized. Although, I believe it's very unlikely to change in the future - it'll probably always be an address followed by some metadata (array length or virtual table).

@jeehoonkang
Copy link
Contributor Author

jeehoonkang commented Dec 25, 2018

@stjepang I implemented a prototype here: https://github.com/jeehoonkang/crossbeam/tree/dst

  • I'm pretty sure it'll work, but it's not tested at all. Needs test code to be merged upstream.

  • Needs 4 nightly features: unsized_locals, alloc_layout_extra, specialization, forget_unsized.

  • It's tricky to get it compiled in stable Rust. In order to do so, I need to duplicate 13 functions for stable Rust, because we have to opt-out the support for unsized types in stable Rust. For example:

     /// Returns a bitmask containing the unused least significant bits of an aligned pointer to `T`.
     #[inline]
     #[cfg(feature = "nightly")]
     fn low_bits<T: ?Sized>() -> usize {
         (1 << T::align_of().trailing_zeros()) - 1
     }
    
     /// Returns a bitmask containing the unused least significant bits of an aligned pointer to `T`.
     #[inline]
     #[cfg(not(feature = "nightly"))]
     fn low_bits<T>() -> usize {
         (1 << T::align_of().trailing_zeros()) - 1
     }

    But it'll be very verbose and I don't like to go across that bridge... I'd rather wait for the required nightly features to be stabilized.

    Here are the 13 functions that need to be duplicated:

    low_bits
    Atomic::new
    fmt::Pointer for Atomic
    From<*const T> for Atomic
    Owned::new
    Owned::from_raw
    Drop for Owned
    Borrow for Owned
    BorrowMut for Owned
    AsRef for Owned
    AsMut for Owned
    Shared::as_raw
    From<*const T> for Shared
    

@jeehoonkang
Copy link
Contributor Author

Now I think introducing the storage trait (this PR) is better than utilizing unsized types (https://github.com/jeehoonkang/crossbeam/tree/dst) in supporting dynamically sized types in Crossbeam, even after all necessary features for unsized types are stabilized.

First, storage trait is more general. In theory, it can support not only dynamically sized types but also allocations in different regions (e.g. those for CPU memory and those for GPU memory) and other use cases. I believe it's better to treat allocation and storage as a first-class citizen.

Second, the implementation based on the storage trait will be simpler. In essence, somewhere we need to apply different logics to sized and unsized types. This PR does that using a trait, which is exactly designed for this purpose, while the implementation based on unsized types will do that using specialization, which is non-parametric and likely introduces more cognitive loads.

For those reasons, I'd like to pursue this PR further. @stjepang, what do you think?

@jeehoonkang jeehoonkang force-pushed the storage branch 2 times, most recently from 8529558 to cd4dcf5 Compare April 24, 2019 05:12
@jeehoonkang
Copy link
Contributor Author

@stjepang any opinions on this issue?

@ghost
Copy link

ghost commented May 11, 2019

I'm still unsure whether adding this feature is necessary since the drawbacks currently outweight the benefits. The balance in my mind goes like this:

Benefits:

  • Better performance (fewer allocations and less indirection). But we still don't know how much it is improved. I guess a proof that this PR brings us the desired benefits would be to make a simple benchmark: an example that uses Atomic<Array<T>> (like in the deque) and then another one without the extra indirection. The second version needs to be meaningfully faster.

Drawbacks:

  • More complicated types and increased API surface.
  • More code to maintain.
  • There are not many places that really need this (the skiplist uses custom allocation, while the performance benefits in deque are dubious, and we could use custom allocation there too). If we used custom allocation (like in the skiplist) in all the currently known use cases this PR is trying to support, the total amount of code would be less that this PR itself!

I'm especially hesitant because this crate is not very easy to use already and would therefore try to avoid adding more complexity to it, if possible.

Feature unsized_locals got good progress recently and it seems the lang team is pretty confident we want to stabilize it eventually: rust-lang/rust#48055 So it seems there might be an easier solution in the future.

@jeehoonkang
Copy link
Contributor Author

Superseded by #452

bors bot added a commit that referenced this pull request May 25, 2020
452: Support dynamically sized types r=jeehoonkang a=jeehoonkang

It supersedes #209 for supporting dynamically sized types (DST) in Crossbeam.  It's much cleaner and less intrusive than the previous attempts in that it doesn't introduce another `Atomic` type.  `Atomic<T>` is still there without significant changes.

The key idea is (1) instead of requiring the condition on `T: Sized`, (2) requiring `T: Pointable` that means an object of type `T` can be pointed to by a single word.  For instance, `Atomic` becomes: `pub struct Atomic<T: ?Sized + Pointable> { ... }`.

It is breaking the backward compatibility in that it ~~(1) increases the minimum required Rust version to 1.36 (for `MaybeUninit`) and (2)~~ now `const fn Atomic::null()` is Nightly only.

Co-authored-by: Jeehoon Kang <jeehoon.kang@kaist.ac.kr>
@jeehoonkang jeehoonkang deleted the storage branch January 15, 2021 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

1 participant