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.

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

task/future: support spawning locally #24

Merged
merged 7 commits into from
Jan 2, 2020
Merged

task/future: support spawning locally #24

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
031ffd1
task/future: support spawning locally
BusyJay Dec 31, 2019
3ef6e43
adjust test case
BusyJay Dec 31, 2019
7f37e1a
address comment
BusyJay Dec 31, 2019
7857d64
remove unnecessary clone
BusyJay Dec 31, 2019
35e8c6b
address comment
BusyJay Dec 31, 2019
86dd586
address comment
BusyJay Jan 1, 2020
311a4ad
assign remote during polling
BusyJay Jan 1, 2020
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
28 changes: 10 additions & 18 deletions src/task/future.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,23 +88,6 @@ unsafe fn waker(task: *const Task) -> Waker {
#[inline]
unsafe fn clone_raw(this: *const ()) -> RawWaker {
let task_cell = clone_task(this as *const Task);
let extras = { &mut *task_cell.0.extras.get() };
if extras.remote.is_none() {
LOCAL.with(|l| {
// In general cases, waker will be cloned before getting out of the
// thread pool scope. And `Runner` guarantees `LOCAL` is
// initialized whenever the future is polled in the scope.
if !l.get().is_null() {
extras.remote = Some((&*l.get()).remote());
} else {
// NOTE: Due to rust-lang/rust#66481, it's possible that
// context is shared between threads by reference, but it is
// such a tricky situation that let's not support it to make
// code clean.
panic!("waker should not be moved to other threads by reference");
}
})
}
RawWaker::new(
Arc::into_raw(task_cell.0) as *const (),
&RawWakerVTable::new(clone_raw, wake_raw, wake_ref_raw, drop_raw),
Expand Down Expand Up @@ -186,7 +169,7 @@ unsafe fn wake_task(task: Cow<'_, Arc<Task>>, reschedule: bool) {
(*task.as_ref().extras.get())
.remote
.as_ref()
.expect("waker should not be moved to other threads by reference")
.expect("remote should exist!!!")
.spawn(TaskCell(task.clone().into_owned()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I still don't understand why we need a clone here...

Copy link
Member Author

Choose a reason for hiding this comment

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

TaskCell can be dropped inside method spawn, which can make self invalid.

} else if reschedule {
// It's requested explicitly to schedule to global queue.
Expand Down Expand Up @@ -257,6 +240,15 @@ impl crate::pool::Runner for Runner {
task.status.store(COMPLETED, SeqCst);
return true;
}
let extras = { &mut *task.extras.get() };
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be executed before poll. Otherwise, before the first poll finished, waking a waker moved to other threads will panic.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use mut_extras here.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it won't. Waking a polling future will mark it NOTIFIED instead of actually waking it up.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can use mut_extras here

They are not the same extras. Besides, task_cell is moved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. I got it.

if extras.remote.is_none() {
// It's possible to avoid assigning remote in some cases, but it requires
// at least one atomic load to detect such situation. So here just assign
// it to make things simple.
LOCAL.with(|l| {
extras.remote = Some((&*l.get()).remote());
})
}
match task.status.compare_exchange(POLLING, IDLE, SeqCst, SeqCst) {
Ok(_) => return false,
Err(NOTIFIED) => {
Expand Down