-
Notifications
You must be signed in to change notification settings - Fork 347
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
Buffer pre-session tasks in BP unified scheduler #4949
base: master
Are you sure you want to change the base?
Changes from 1 commit
0367ff9
e1fd517
78f3739
7636f40
47d09bf
8697c30
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -238,40 +238,48 @@ pub type SchedulerId = u64; | |
#[derive(Clone, Debug)] | ||
pub struct SchedulingContext { | ||
mode: SchedulingMode, | ||
bank: Arc<Bank>, | ||
bank: Option<Arc<Bank>>, | ||
} | ||
|
||
impl SchedulingContext { | ||
pub fn new(bank: Arc<Bank>) -> Self { | ||
// mode will be configurable later | ||
pub fn for_preallocation() -> Self { | ||
Self { | ||
mode: SchedulingMode::BlockVerification, | ||
bank, | ||
mode: SchedulingMode::BlockProduction, | ||
bank: None, | ||
} | ||
} | ||
|
||
pub fn new_with_mode(mode: SchedulingMode, bank: Arc<Bank>) -> Self { | ||
Self { mode, bank } | ||
Self { | ||
mode, | ||
bank: Some(bank), | ||
} | ||
} | ||
|
||
#[cfg_attr(feature = "dev-context-only-utils", qualifiers(pub))] | ||
fn for_verification(bank: Arc<Bank>) -> Self { | ||
Self::new_with_mode(SchedulingMode::BlockVerification, bank) | ||
} | ||
|
||
#[cfg(feature = "dev-context-only-utils")] | ||
pub fn for_production(bank: Arc<Bank>) -> Self { | ||
Self { | ||
mode: SchedulingMode::BlockProduction, | ||
bank, | ||
} | ||
Self::new_with_mode(SchedulingMode::BlockProduction, bank) | ||
} | ||
|
||
pub fn is_preallocated(&self) -> bool { | ||
self.bank.is_none() | ||
} | ||
|
||
pub fn mode(&self) -> SchedulingMode { | ||
self.mode | ||
} | ||
|
||
pub fn bank(&self) -> &Arc<Bank> { | ||
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. should this return 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. done: 7636f40 |
||
&self.bank | ||
self.bank.as_ref().unwrap() | ||
} | ||
|
||
pub fn slot(&self) -> Slot { | ||
self.bank().slot() | ||
pub fn slot(&self) -> Option<Slot> { | ||
self.bank.as_ref().map(|bank| bank.slot()) | ||
} | ||
} | ||
|
||
|
@@ -570,7 +578,8 @@ impl BankWithSchedulerInner { | |
let pool = pool.clone(); | ||
drop(scheduler); | ||
|
||
let context = SchedulingContext::new(self.bank.clone()); | ||
// Schedulers can be stale only if its mode is block-verification. | ||
let context = SchedulingContext::for_verification(self.bank.clone()); | ||
let mut scheduler = self.scheduler.write().unwrap(); | ||
trace!("with_active_scheduler: {:?}", scheduler); | ||
scheduler.transition_from_stale_to_active(|pool, result_with_timings| { | ||
|
@@ -773,7 +782,7 @@ mod tests { | |
mock.expect_context() | ||
.times(1) | ||
.in_sequence(&mut seq.lock().unwrap()) | ||
.return_const(SchedulingContext::new(bank)); | ||
.return_const(SchedulingContext::for_verification(bank)); | ||
|
||
for wait_reason in is_dropped_flags { | ||
let seq_cloned = seq.clone(); | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,9 +103,10 @@ use { | |
solana_transaction::sanitized::SanitizedTransaction, | ||
static_assertions::const_assert_eq, | ||
std::{collections::VecDeque, mem, sync::Arc}, | ||
unwrap_none::UnwrapNone, | ||
}; | ||
|
||
#[derive(Clone, Copy, Debug)] | ||
#[derive(Clone, Copy, Debug, PartialEq)] | ||
pub enum SchedulingMode { | ||
BlockVerification, | ||
BlockProduction, | ||
|
@@ -668,11 +669,29 @@ impl SchedulingStateMachine { | |
/// indicating the scheduled task is blocked currently. | ||
/// | ||
/// Note that this function takes ownership of the task to allow for future optimizations. | ||
#[cfg(test)] | ||
#[must_use] | ||
pub fn schedule_task(&mut self, task: Task) -> Option<Task> { | ||
self.schedule_or_buffer_task(task, false) | ||
} | ||
|
||
pub fn buffer_task(&mut self, task: Task) { | ||
self.schedule_or_buffer_task(task, true).unwrap_none(); | ||
} | ||
|
||
#[must_use] | ||
pub fn schedule_or_buffer_task(&mut self, task: Task, force_buffering: bool) -> Option<Task> { | ||
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. The The 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. I think you missed the |
||
self.total_task_count.increment_self(); | ||
self.active_task_count.increment_self(); | ||
self.try_lock_usage_queues(task) | ||
self.try_lock_usage_queues(task).and_then(|task| { | ||
if !force_buffering { | ||
Some(task) | ||
} else { | ||
self.unblocked_task_count.increment_self(); | ||
self.unblocked_task_queue.push_back(task); | ||
None | ||
} | ||
}) | ||
} | ||
|
||
#[must_use] | ||
|
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.
@steviez do we have data on how often we switch forks during leader slot? I'm not aware of any, but I don't believe it's super uncommon for it to happen?
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.
I don't think we have any data on hand, any metrics to easily measure this unfortunately