-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
runtime: steal tasks from the LIFO slot #7431
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
base: master
Are you sure you want to change the base?
Changes from 16 commits
1220253
75d8116
730a581
a5a656d
4c680ae
9ad2404
fe53128
86b16c4
9651b82
0497b31
cb27dda
3022aae
eba7e1d
f5d5766
adf146f
0889f08
aff05a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,13 +101,6 @@ struct Core { | |
/// Used to schedule bookkeeping tasks every so often. | ||
tick: u32, | ||
|
||
/// When a task is scheduled from a worker, it is stored in this slot. The | ||
/// worker will check this slot for a task **before** checking the run | ||
/// queue. This effectively results in the **last** scheduled task to be run | ||
/// next (LIFO). This is an optimization for improving locality which | ||
/// benefits message passing patterns and helps to reduce latency. | ||
lifo_slot: Option<Notified>, | ||
|
||
/// When `true`, locally scheduled tasks go to the LIFO slot. When `false`, | ||
/// they go to the back of the `run_queue`. | ||
lifo_enabled: bool, | ||
|
@@ -257,7 +250,6 @@ pub(super) fn create( | |
|
||
cores.push(Box::new(Core { | ||
tick: 0, | ||
lifo_slot: None, | ||
lifo_enabled: !config.disable_lifo_slot, | ||
run_queue, | ||
is_searching: false, | ||
|
@@ -405,7 +397,7 @@ where | |
// If we heavily call `spawn_blocking`, there might be no available thread to | ||
// run this core. Except for the task in the lifo_slot, all tasks can be | ||
// stolen, so we move the task out of the lifo_slot to the run_queue. | ||
if let Some(task) = core.lifo_slot.take() { | ||
if let Some(task) = core.run_queue.pop_lifo() { | ||
core.run_queue | ||
.push_back_or_overflow(task, &*cx.worker.handle, &mut core.stats); | ||
} | ||
|
@@ -620,7 +612,7 @@ impl Context { | |
}; | ||
|
||
// Check for a task in the LIFO slot | ||
let task = match core.lifo_slot.take() { | ||
let task = match core.run_queue.pop_lifo() { | ||
Some(task) => task, | ||
None => { | ||
self.reset_lifo_enabled(&mut core); | ||
|
@@ -858,7 +850,7 @@ impl Core { | |
} | ||
|
||
fn next_local_task(&mut self) -> Option<Notified> { | ||
self.lifo_slot.take().or_else(|| self.run_queue.pop()) | ||
self.run_queue.pop_lifo().or_else(|| self.run_queue.pop()) | ||
} | ||
|
||
/// Function responsible for stealing tasks from another worker | ||
|
@@ -914,7 +906,7 @@ impl Core { | |
} | ||
|
||
fn has_tasks(&self) -> bool { | ||
self.lifo_slot.is_some() || self.run_queue.has_tasks() | ||
self.run_queue.has_tasks() | ||
} | ||
|
||
fn should_notify_others(&self) -> bool { | ||
|
@@ -923,7 +915,7 @@ impl Core { | |
if self.is_searching { | ||
return false; | ||
} | ||
self.lifo_slot.is_some() as usize + self.run_queue.len() > 1 | ||
self.run_queue.len() > 1 | ||
} | ||
|
||
/// Prepares the worker state for parking. | ||
|
@@ -1090,17 +1082,16 @@ impl Handle { | |
true | ||
} else { | ||
// Push to the LIFO slot | ||
let prev = core.lifo_slot.take(); | ||
let ret = prev.is_some(); | ||
|
||
if let Some(prev) = prev { | ||
core.run_queue | ||
.push_back_or_overflow(prev, self, &mut core.stats); | ||
match core.run_queue.push_lifo(task) { | ||
Some(prev) => { | ||
// There was a previous task in the LIFO slot which needs | ||
// to be pushed to the back of the run queue. | ||
core.run_queue | ||
.push_back_or_overflow(prev, self, &mut core.stats); | ||
true | ||
} | ||
None => false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This boolean indicates whether other worker threads are notified that there is work available to steal. Since the lifo slot is now stealable, it should no longer be false in this scenario. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this something we could have caught with a loom test? Maybe put something in the lifo slot and then block the thread, concurrently with another thread doing nothing? |
||
} | ||
|
||
core.lifo_slot = Some(task); | ||
|
||
ret | ||
}; | ||
|
||
// Only notify if not currently parked. If `park` is `None`, then the | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
use crate::loom::sync::atomic::AtomicPtr; | ||
use crate::runtime::task::{Header, Notified, RawTask}; | ||
|
||
use std::marker::PhantomData; | ||
use std::ptr; | ||
use std::ptr::NonNull; | ||
use std::sync::atomic::Ordering::{AcqRel, Acquire}; | ||
|
||
/// An atomic cell which can contain a pointer to a [`Notified`] task. | ||
/// | ||
/// This is similar to the `crate::util::AtomicCell` type, but specialized to | ||
/// hold a task pointer --- this type "remembers" the task's scheduler generic | ||
/// when a task is stored in the cell, so that the pointer can be turned back | ||
/// into a [`Notified`] task with the correct generic type when it is retrieved. | ||
pub(crate) struct AtomicNotified<S: 'static> { | ||
task: AtomicPtr<Header>, | ||
_scheduler: PhantomData<S>, | ||
} | ||
|
||
impl<S: 'static> AtomicNotified<S> { | ||
pub(crate) fn empty() -> Self { | ||
Self { | ||
task: AtomicPtr::new(ptr::null_mut()), | ||
_scheduler: PhantomData, | ||
} | ||
} | ||
|
||
pub(crate) fn swap(&self, task: Option<Notified<S>>) -> Option<Notified<S>> { | ||
let new = task | ||
.map(|t| t.into_raw().header_ptr().as_ptr()) | ||
.unwrap_or_else(ptr::null_mut); | ||
let old = self.task.swap(new, AcqRel); | ||
NonNull::new(old).map(|ptr| unsafe { | ||
// Safety: since we only allow tasks with the same scheduler type to | ||
// be placed in this cell, we know that the pointed task's scheduler | ||
// type matches the type parameter S. | ||
Notified::from_raw(RawTask::from_raw(ptr)) | ||
}) | ||
} | ||
|
||
pub(crate) fn take(&self) -> Option<Notified<S>> { | ||
self.swap(None) | ||
} | ||
|
||
pub(crate) fn is_some(&self) -> bool { | ||
!self.task.load(Acquire).is_null() | ||
} | ||
} | ||
|
||
unsafe impl<S: Send> Send for AtomicNotified<S> {} | ||
unsafe impl<S: Send> Sync for AtomicNotified<S> {} | ||
|
||
impl<S> Drop for AtomicNotified<S> { | ||
fn drop(&mut self) { | ||
// Ensure the task reference is dropped if this cell is dropped. | ||
let _ = self.take(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reviewers: Rust has defined the casting from
bool
tou16
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also possible to use
u16::from(bool)
, if you care aboutcast_lossless
cleanliness.