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

Add support for stable Rust #4

Merged
merged 1 commit into from Sep 7, 2018
Merged

Add support for stable Rust #4

merged 1 commit into from Sep 7, 2018

Conversation

ghost
Copy link

@ghost ghost commented Sep 6, 2018

A new flag nightly is added that enables nightly features (the most important ones being CoerceUnsized and Unsize). Otherwise, stable Rust is supported but SmallBox can be used with sized types only.

This PR is still a work in progress. More tests need to be ported and the documentation needs to be updated. I'm submitting the PR early to check whether this is a direction we'd like to take at all.

To provide some context, I need SmallBox in crossbeam-rs/crossbeam-channel#86

@andylokandy
Copy link
Owner

Thanks for the PR! This project has been not updated for a long while, so I think it's indeed a good time to review it as the nightly rust had been developed so much. Actually, I've done some refactory work but I didn't push them here. I'm gonna to merge this PR and do the update in one or two days. Again, thanks for bring this up!

@ghost
Copy link
Author

ghost commented Sep 6, 2018

Awesome, thank you! If you need help with anything or would like me to finish this PR, let me know. :)

@andylokandy
Copy link
Owner

Yup, if you have some work not done yet, you can keep submiting them here, I'll do the preliminary work first.

@andylokandy andylokandy merged commit c4263f5 into andylokandy:master Sep 7, 2018
@ghost ghost deleted the stable-support branch September 7, 2018 11:00
@andylokandy
Copy link
Owner

andylokandy commented Sep 8, 2018

@stjepang I have to make a decision on the new update and I want to hear your opinion. I'm going to implement a must-success resize for SmallBox, which will box up it's stack-stored content when it's resizing into a too small SmallBox. But I can't do it if I don't implement a IntermediateBox myself. It means that we would not be able to take the standard Box variant out of SmallBox anymore. Another option is to do nothing. Which one do you prefer?

@ghost
Copy link
Author

ghost commented Sep 8, 2018

But I can't do it if I don't implement a IntermediateBox myself.

So if we resize to a smaller space, we try to turn the StackBox into a Box.
If we resize to a larger space, we try to turn the Box into a StackBox.

Which direction of these two is problematic?

I don't understand why we need IntermediateBox and can't take the Box variant out of SmallBox, can you elaborate where the problem occurs?

@andylokandy
Copy link
Owner

andylokandy commented Sep 8, 2018

If we resize to a larger space, we try to turn the Box into a StackBox.

It's quite not efficient to unbox something that are already stored on heap. So SmallBox will keep things on heap once they are boxed up.

So if we resize to a smaller space, we try to turn the StackBox into a Box.

The first problem is that we can't get the content by value(similar to DeferMove) out of SmallBox due to DSTs, but the standard Box::new() or box syntax want to take the content by value and there is no other way to box something that are already stored on heap because Box doesn't expose it's inner layout(and the document suggests not to depend on it).

@ghost
Copy link
Author

ghost commented Sep 8, 2018

I think we could do something like this (this works even on stable Rust 1.28):

pub fn resize<ToSpace>(self) -> SmallBox<T, ToSpace> {
    use std::mem;
    use std::alloc::{alloc, Layout};

    match self {
        SmallBox::Stack(x) => match x.resize() {
            Ok(x) => SmallBox::Stack(x),
            Err(x) => {
                let size = mem::size_of_val::<T>(&*x);
                let align = mem::align_of_val::<T>(&*x);

                let boxed = unsafe {
                    let dest = alloc(Layout::from_size_align_unchecked(size, align));
                    (&*x as *const T as *const u8).copy_to_nonoverlapping(dest, size);

                    let mut ptr = &*x as *const T as *mut T;
                    *(&mut ptr as *mut _ as *mut usize) = dest as usize;
                    Box::from_raw(ptr)
                };

                mem::forget(x);
                SmallBox::Box(boxed)
            }
        },
        SmallBox::Box(x) => SmallBox::Box(x),
    }
}

This would be ideal if you don't mind upgrading to Rust 1.28. Crossbeam is still stuck on 1.26, so that might be a bit of a problem for me :/

@andylokandy
Copy link
Owner

From document:

Specifically, the Box destructor will call the destructor of T and free the allocated memory. Since the way Box allocates and releases memory is unspecified, the only valid pointer to pass to this function is the one taken from another Box via the Box::into_raw function.

I am not sure whether we can do this directly... Since the allocate/deallocate behavior is not defined in the source of std but in the compiler as special lang_item.

@ghost
Copy link
Author

ghost commented Sep 8, 2018

Ah, I missed that - you're right! Seems like we'll have to wait until we get alloca support...

In that case, I'd probably prefer not introducing IntermediateBox and would wait until the language gets features that enable us to implement this properly.

@andylokandy
Copy link
Owner

Alright, so we had better not to resize into smaller SmallBox.

@andylokandy
Copy link
Owner

And we can't get a Box from StackBox either.

@andylokandy
Copy link
Owner

andylokandy commented Sep 8, 2018

I came up with an idea just now: we support resize by IntermediateBox now and then add method to_box()(or Into<Box>), which will return a standard Box regardless of whether we have stores the content on heap or not, under nightly flag when unsized rvalue lands.

@andylokandy
Copy link
Owner

andylokandy commented Sep 8, 2018

Seems that I can start on this since the unsized ravlue is implemented!(rust-lang/rust#51131)

@ghost
Copy link
Author

ghost commented Sep 8, 2018

Also, we could simply require T: Sized if nightly is not enabled (at least until we can implement it in stable Rust) and use unsized rvalues otherwise.

@andylokandy
Copy link
Owner

Yup, that's what I am doing now.

@andylokandy
Copy link
Owner

@stjepang The stable part is ready, with updated document, but I encountered some problems on the nightly, and I would be too busy in the following two days to have time on it. The final version should be finished this week. If you need the stable version in a hurry, I can publish a version with dst stripped out now.

@ghost
Copy link
Author

ghost commented Sep 9, 2018

Nah, this week sounds good, take your time :) And thanks for the speedy updates, I really appreciate it!

@andylokandy
Copy link
Owner

@stjepang Version 0.5.0 is available on crates.io now!

@ghost
Copy link
Author

ghost commented Sep 11, 2018

Thank you!

@andylokandy
Copy link
Owner

andylokandy commented Oct 2, 2018

@stjepang smallbox now supports unsized type on stable rust! Let's try it out!

The align check is also fixed in #6.

@ghost
Copy link
Author

ghost commented Oct 2, 2018

This is a really neat trick to make unsized types work on stable Rust! I like it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant