Skip to content

Add LocalTaskObj to core::task #51814

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

Merged
merged 4 commits into from
Jun 26, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/liballoc/boxed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ use core::marker::{Unpin, Unsize};
use core::mem::{self, PinMut};
use core::ops::{CoerceUnsized, Deref, DerefMut, Generator, GeneratorState};
use core::ptr::{self, NonNull, Unique};
use core::task::{Context, Poll, UnsafeTask, TaskObj};
use core::task::{Context, Poll, UnsafeTask, TaskObj, LocalTaskObj};
use core::convert::From;

use raw_vec::RawVec;
Expand Down Expand Up @@ -933,7 +933,7 @@ impl<'a, F: ?Sized + Future> Future for PinBox<F> {
}

#[unstable(feature = "futures_api", issue = "50547")]
unsafe impl<F: Future<Output = ()> + Send + 'static> UnsafeTask for PinBox<F> {
unsafe impl<F: Future<Output = ()> + 'static> UnsafeTask for PinBox<F> {
fn into_raw(self) -> *mut () {
PinBox::into_raw(self) as *mut ()
}
Expand Down Expand Up @@ -962,3 +962,17 @@ impl<F: Future<Output = ()> + Send + 'static> From<Box<F>> for TaskObj {
TaskObj::new(PinBox::from(boxed))
}
}

#[unstable(feature = "futures_api", issue = "50547")]
impl<F: Future<Output = ()> + 'static> From<PinBox<F>> for LocalTaskObj {
fn from(boxed: PinBox<F>) -> Self {
LocalTaskObj::new(boxed)
}
}

#[unstable(feature = "futures_api", issue = "50547")]
impl<F: Future<Output = ()> + 'static> From<Box<F>> for LocalTaskObj {
fn from(boxed: Box<F>) -> Self {
LocalTaskObj::new(PinBox::from(boxed))
}
}
4 changes: 2 additions & 2 deletions src/libcore/task/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ mod poll;
pub use self::poll::Poll;

mod spawn_error;
pub use self::spawn_error::{SpawnErrorKind, SpawnObjError};
pub use self::spawn_error::{SpawnErrorKind, SpawnObjError, SpawnLocalObjError};

mod task;
pub use self::task::{TaskObj, UnsafeTask};
pub use self::task::{TaskObj, LocalTaskObj, UnsafeTask};

mod wake;
pub use self::wake::{Waker, LocalWaker, UnsafeWake};
33 changes: 32 additions & 1 deletion src/libcore/task/spawn_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
issue = "50547")]

use fmt;
use super::TaskObj;
use mem;
use super::{TaskObj, LocalTaskObj};

/// Provides the reason that an executor was unable to spawn.
pub struct SpawnErrorKind {
Expand Down Expand Up @@ -49,3 +50,33 @@ pub struct SpawnObjError {
/// The task for which spawning was attempted
pub task: TaskObj,
}

/// The result of a failed spawn
#[derive(Debug)]
pub struct SpawnLocalObjError {
/// The kind of error
pub kind: SpawnErrorKind,

/// The task for which spawning was attempted
pub task: LocalTaskObj,
}

impl SpawnLocalObjError {
/// Converts the `SpawnLocalObjError` into a `SpawnObjError`
/// To make this operation safe one has to ensure that the `UnsafeTask`
/// instance from which the `LocalTaskObj` stored inside was created
/// actually implements `Send`.
pub unsafe fn as_spawn_obj_error(self) -> SpawnObjError {
Copy link
Member

@cramertj cramertj Jun 26, 2018

Choose a reason for hiding this comment

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

What is this for? This seems like a pretty big footgun.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@cramertj cramertj Jun 26, 2018

Choose a reason for hiding this comment

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

You don't need anything unsafe to implement that. You can implement spawn_obj by inlining the body of spawn_local_obj and using into only if upgrade succeeds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean copy paste?

// Safety: Both structs have the same memory layout
mem::transmute::<SpawnLocalObjError, SpawnObjError>(self)
}
}

impl From<SpawnObjError> for SpawnLocalObjError {
fn from(error: SpawnObjError) -> SpawnLocalObjError {
unsafe {
// Safety: Both structs have the same memory layout
mem::transmute::<SpawnObjError, SpawnLocalObjError>(error)
Copy link
Member

Choose a reason for hiding this comment

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

No need to transmute here-- just move the fields over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will it have the same performance?

Copy link
Member

Choose a reason for hiding this comment

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

Yup.

}
}
}
71 changes: 68 additions & 3 deletions src/libcore/task/task.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

use fmt;
use future::Future;
use mem::PinMut;
use mem::{self, PinMut};
use super::{Context, Poll};

/// A custom trait object for polling tasks, roughly akin to
Expand All @@ -30,7 +30,7 @@ unsafe impl Send for TaskObj {}
impl TaskObj {
/// Create a `TaskObj` from a custom trait object representation.
#[inline]
pub fn new<T: UnsafeTask>(t: T) -> TaskObj {
pub fn new<T: UnsafeTask + Send>(t: T) -> TaskObj {
Copy link
Member

@cramertj cramertj Jun 26, 2018

Choose a reason for hiding this comment

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

Rather than repeating all the logic between TaskObj and LocalTaskObj, I'd make TaskObj into a single-field wrapper around LocalTaskObj that just adds Send-ability at the cost of requiring Send to construct (in new).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current way was inspired by the way it is done for LocalWaker & Waker. You're right. A single field struct is nicer

Copy link
Member

Choose a reason for hiding this comment

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

Waker and LocalWaker are a bit different because they actually call different functions, but it's possible they could be combined further as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok good point. Anyhow I think the current waker code is good. At closer inspection it really shows that a lot of considerations went into it.

TaskObj {
ptr: t.into_raw(),
poll_fn: T::poll,
Expand Down Expand Up @@ -65,6 +65,71 @@ impl Drop for TaskObj {
}
}

/// A custom trait object for polling tasks, roughly akin to
/// `Box<Future<Output = ()>>`.
/// Contrary to `TaskObj`, `LocalTaskObj` does not have a `Send` bound.
pub struct LocalTaskObj {
ptr: *mut (),
poll_fn: unsafe fn(*mut (), &mut Context) -> Poll<()>,
drop_fn: unsafe fn(*mut ()),
}

impl LocalTaskObj {
/// Create a `LocalTaskObj` from a custom trait object representation.
#[inline]
pub fn new<T: UnsafeTask>(t: T) -> LocalTaskObj {
LocalTaskObj {
ptr: t.into_raw(),
poll_fn: T::poll,
drop_fn: T::drop,
}
}

/// Converts the `LocalTaskObj` into a `TaskObj`
/// To make this operation safe one has to ensure that the `UnsafeTask`
/// instance from which this `LocalTaskObj` was created actually implements
/// `Send`.
pub unsafe fn as_task_obj(self) -> TaskObj {
// Safety: Both structs have the same memory layout
mem::transmute::<LocalTaskObj, TaskObj>(self)
}
}

impl fmt::Debug for LocalTaskObj {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.debug_struct("LocalTaskObj")
.finish()
}
}

impl From<TaskObj> for LocalTaskObj {
fn from(task: TaskObj) -> LocalTaskObj {
unsafe {
// Safety: Both structs have the same memory layout
mem::transmute::<TaskObj, LocalTaskObj>(task)
}
}
}

impl Future for LocalTaskObj {
type Output = ();

#[inline]
fn poll(self: PinMut<Self>, cx: &mut Context) -> Poll<()> {
unsafe {
(self.poll_fn)(self.ptr, cx)
}
}
}

impl Drop for LocalTaskObj {
fn drop(&mut self) {
unsafe {
(self.drop_fn)(self.ptr)
}
}
}

/// A custom implementation of a task trait object for `TaskObj`, providing
/// a hand-rolled vtable.
///
Expand All @@ -74,7 +139,7 @@ impl Drop for TaskObj {
/// The implementor must guarantee that it is safe to call `poll` repeatedly (in
/// a non-concurrent fashion) with the result of `into_raw` until `drop` is
/// called.
pub unsafe trait UnsafeTask: Send + 'static {
pub unsafe trait UnsafeTask: 'static {
/// Convert a owned instance into a (conceptually owned) void pointer.
fn into_raw(self) -> *mut ();

Expand Down