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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 26, 2018
Merged

Add LocalTaskObj to core::task #51814

merged 4 commits into from
Jun 26, 2018

Conversation

MajorBreakfast
Copy link
Contributor

@MajorBreakfast MajorBreakfast commented Jun 26, 2018 •
edited
Loading

  • Splits libcore/task.rs into submodules
  • Adds LocalTaskObj and SpawnLocalObjError (-> Commit for this)

Note: To make reviewing easy, both actions have their own commit

r? @cramertj

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 26, 2018
@MajorBreakfast MajorBreakfast mentioned this pull request Jun 26, 2018
Merged
cramertj
cramertj reviewed Jun 26, 2018
View reviewed changes
src/libcore/task/mod.rs Outdated
pub use self::poll::Poll;

mod spawn_error;
pub use self::spawn_error::{SpawnErrorKind, SpawnObjError};
Copy link
Member

Choose a reason for hiding this comment

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

IMO the errors don't deserve their own file-- I'd move them in with the executor (since that's what uses them).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

cramertj
cramertj reviewed Jun 26, 2018
View reviewed changes
src/libcore/task/spawn_error.rs Outdated
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.

cramertj
cramertj reviewed Jun 26, 2018
View reviewed changes
src/libcore/task/spawn_error.rs Outdated
/// 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 •
edited
Loading

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.

https://github.com/rust-lang-nursery/futures-rs/pull/1046/files#diff-9c6376c5ce46d9e39358812a3269ab6bR259

Copy link
Member

@cramertj cramertj Jun 26, 2018 •
edited
Loading

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?

cramertj
cramertj reviewed Jun 26, 2018
View reviewed changes
src/libcore/task/task.rs Outdated
@@ -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 •
edited
Loading

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.

@MajorBreakfast
Copy link
Contributor Author

MajorBreakfast commented Jun 26, 2018 •
edited
Loading

I removed the conversions for SpawnErrorObj. If I you want me to set the fields instead of transmute, then there is no reason to put the conversions in libcore because this can be done anywhere because the fields are public.

@cramertj
Copy link
Member

cramertj commented Jun 26, 2018 •
edited
Loading

@MajorBreakfast The fields of TaskObj and LocalTaskObj aren't public, so if you want an unsafe conversion from LocalTaskObj -> TaskObj, it will need to be in core: https://github.com/rust-lang/rust/pull/51814/files#diff-c756fc15162cb40ebeb296a1699a84b5R24

Edit: oh, you meant for the errors. I see you kept https://github.com/rust-lang/rust/pull/51814/files#diff-c756fc15162cb40ebeb296a1699a84b5R44

@cramertj
Copy link
Member

r=me with travis passing. Thanks!

@cramertj
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 26, 2018

📌 Commit b39ea1d has been approved by cramertj

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2018
@bors
Copy link
Collaborator

bors commented Jun 26, 2018

⌛ Testing commit b39ea1d with merge 84804c3...

bors added a commit that referenced this pull request Jun 26, 2018
@bors
Auto merge of #51814 - MajorBreakfast:local-task-obj, r=cramertj
84804c3 
Add `LocalTaskObj` to `core::task`

- Splits `libcore/task.rs` into submodules
- Adds `LocalTaskObj` and `SpawnLocalObjError` (-> [Commit for this](433e6b3))

Note: To make reviewing easy, both actions have their own commit

r? @cramertj
@bors
Copy link
Collaborator

bors commented Jun 26, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: cramertj
Pushing 84804c3 to master...

@bors bors merged commit b39ea1d into rust-lang:master Jun 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cramertj cramertj cramertj left review comments

Assignees

@cramertj cramertj

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MajorBreakfast @cramertj @bors @rust-highfive