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

(Do not merge) Introduce pins #20063

Closed
wants to merge 1 commit into from
Closed

(Do not merge) Introduce pins #20063

wants to merge 1 commit into from

Conversation

@nox
Copy link
Member

nox commented Feb 16, 2018

Just pushing for people to take a look at it.

Things I want to do before we consider merging it:

  • document the stuff;
  • bikeshed the macro syntax;
  • use said macro in more places;
  • support non-'static types;
  • protect code against dropping Pin values out of order;
  • introduce an UnpinnedDefault trait;
  • introduce a TracedVec type that doesn't implement drain, moving iterators, etc.

This change is Reviewable

@highfive
Copy link

highfive commented Feb 16, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/microtask.rs, components/script/script_runtime.rs, components/script/dom/bindings/mod.rs, components/script/dom/bindings/pin.rs, components/script/dom/macros.rs
  • @fitzgen: components/script/microtask.rs, components/script/script_runtime.rs, components/script/dom/bindings/mod.rs, components/script/dom/bindings/pin.rs, components/script/dom/macros.rs
  • @KiChjang: components/script/microtask.rs, components/script/script_runtime.rs, components/script/dom/bindings/mod.rs, components/script/dom/bindings/pin.rs, components/script/dom/macros.rs
@highfive
Copy link

highfive commented Feb 16, 2018

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
@nox
Copy link
Member Author

nox commented Feb 16, 2018

@nox
Copy link
Member Author

nox commented Feb 16, 2018

Note that this encoding allows Pin to be used trivially with Dom<T>: we can just implement Unpinned<&T> for Dom<T>.

@asajeffrey
Copy link
Member

asajeffrey commented Feb 16, 2018

Ooooh, nice! On first glance, it looks good. I was wondering why we have Unpinned? It looks like the only place it's used is in the macro, Can we do something like:

macro_rules! pinned {
   (let ref $name:ident: $ty:ty := $expr:expr) => {
        let mut $name = $crate::dom::bindings::pin::Pin::<$ty>::new();
        let $name = unsafe { $name.pin($expr) };
    };
}

and ditto for let mut ref by adding a pin_mut() method to Pin?

@nox
Copy link
Member Author

nox commented Feb 16, 2018

I was wondering why we have Unpinned?

Because owning a Dom<T> is unsafe (in the absence of the lint), so you can't just feed it directly to the Pin.

and ditto for let mut ref by adding a pin_mut() method to Pin?

Providing any mutable access is UB because it introduces aliasing when JS traces the pins.

@asajeffrey
Copy link
Member

asajeffrey commented Feb 16, 2018

Ah that makes sense.

The IRC conversation about this: https://mozilla.logbot.info/servo/20180216#c14315715

@nox
Copy link
Member Author

nox commented Feb 16, 2018

Note too that WebIDL unions can be trivially pinned with that encoding too:

#[derive(JSTraceable)]
pub enum DocumentOrBlob {
    Document(Dom<Document>),
    Blob(Dom<Blob>),
}

unsafe impl<'a> Unrooted<&'a Document> for DocumentOrBlob {
    unsafe fn unrooted(value: &'a Document) -> Self {
        DocumentOrBlob::Document(Unrooted::unrooted(value))
    }
}

unsafe impl<'a> Unrooted<&'a Blob> for DocumentOrBlob {
    unsafe fn unrooted(value: &'a Blob) -> Self {
        DocumentOrBlob::Blob(Unrooted::unrooted(value))
    }
}
@nox nox force-pushed the end-of-the-line branch 5 times, most recently from 1a22838 to dc0ae23 Feb 16, 2018
@nox nox force-pushed the end-of-the-line branch from dc0ae23 to cff8044 Feb 22, 2018
@bors-servo
Copy link
Contributor

bors-servo commented Feb 22, 2018

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

@nox nox force-pushed the end-of-the-line branch 2 times, most recently from d6fa6cc to 598cedc Feb 26, 2018
@nox nox force-pushed the end-of-the-line branch from 598cedc to 87d95c1 Feb 26, 2018
@nox
Copy link
Member Author

nox commented Feb 26, 2018

@asajeffrey I replaced some rooted_vec! uses by pinned!.

@bors-servo
Copy link
Contributor

bors-servo commented Mar 28, 2018

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

@jdm
Copy link
Member

jdm commented Jul 4, 2019

Closing due to inactivity.

@jdm jdm closed this Jul 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.