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

Use of Pin for Heap<T> to indicate Immovability #376

Open
Redfire75369 opened this issue Aug 25, 2023 · 6 comments
Open

Use of Pin for Heap<T> to indicate Immovability #376

Redfire75369 opened this issue Aug 25, 2023 · 6 comments

Comments

@Redfire75369
Copy link
Contributor

Redfire75369 commented Aug 25, 2023

Currently, Heap<T> effectively duplicates pinning requirements with its own set of rules. This issue proposes a change to use Pin.

Although Pin<Box<Heap<T>>> is more nested than the current Box<Heap<T>>, it clearly signifies the aim of being immovable.
Not that much would change with regards to the public API. Perhaps Heap::boxed could be deprecated in favour of something like Heap::pinned, but this can be discussed later.

From my understanding, none of the methods of Heap<T> would need to change, but I may be mistaken.
I do not believe ptr needs to be public with the addition of Heap::new, but I may be mistaken about servo's use cases.

API Changes

The inline comments show which fields/methods are old, new or remain the same.

pub struct Heap<T: GCMethods + Copy> {
    ptr: UnsafeCell<T>,
    _phantom: PhantomPinned, // New ZST Field
}

impl<T: GCMethods + Copy> Heap<T> {
    pub unsafe fn new(v: T) -> Heap<T>; // New 
    
    pub fn boxed(v: T) -> Box<Heap<T>>; // Old
    pub fn boxed(v: T) -> Pin<Box<Heap<T>>>; // New

    pub fn set(&self, v: T); // Remains Same

    pub fn get(&self) -> T; // Remains Same

    pub unsafe fn get_unsafe(&self) -> *mut T; // Remains Same

    pub unsafe fn handle(&self) -> Handle<T>; // Remains Same
}
@jdm
Copy link
Member

jdm commented Aug 26, 2023

Can you provide any code samples of what using the pinned version looks like?

@Redfire75369
Copy link
Contributor Author

Redfire75369 commented Aug 26, 2023

Code Samples are difficult since most of will be identical with just a few functions changed. The best way would be to see what changes there are with servo itself.
@sagudev has made a branch of servo, servo:mozjs-pinned-heap. Some of the Heap API isn't quite as stated here, but its based on my branch mozjs:pin-heap.

In general, pinning can be quite annoying, but I believe it would be beneficial to express this constraint in the type system as opposed to having the possibility of breaking the invariant.

@sagudev
Copy link
Member

sagudev commented Aug 26, 2023

For reference mozilla has MOZ_NON_MEMMOVABLE.

@Redfire75369
Copy link
Contributor Author

Redfire75369 commented Aug 26, 2023

The API, as implemented, is as follows:

impl<T: GCMethods + Copy> Heap<T> {
    pub unsafe fn new(v: T) -> Heap<T>; // New, replaces `Heap::ptr` being a pub field.

    pub fn pinned(v: T) -> Pin<Box<Heap<T>>> // New
        where Heap<T>: Default;

    #[deprecated(note = "Use Heap::pinned instead")]
    pub fn boxed(v: T) -> Box<Heap<T>> // Old, but deprecated
        where Heap<T>: Default;

    pub fn set(self: Pin<&Self>, v: T); // Old, but breaking change with signature changed to `self: Pin<&Self>` instead of `&self`

    pub unsafe fn set_unsafe(&self, v: T); // New, doesn't require `Pin<&Self>`, but unsafe.

    pub fn get(&self) -> T;

    pub fn get_unsafe(&self) -> *mut T;

    pub unsafe fn handle(&self) -> Handle<T>;
}

@sagudev
Copy link
Member

sagudev commented Sep 3, 2023

This might help to solve: servo/servo#25726

@sagudev
Copy link
Member

sagudev commented Aug 12, 2024

NOTE-TO-SELF: In servo dom objects are usually already boxed (except if new_uninherited).

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

No branches or pull requests

3 participants