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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ryoqun
Copy link
Collaborator

@ryoqun ryoqun commented Feb 12, 2025

Problem

While #4533 introduced (really!) minimal integration between banking stage and unified scheduler, the unified scheduler still needs several improvements for optimal block production.

Summary of Changes

As a first step, support pre-session buffering.

extracted from #3946 (well, with large code cleaning up!)

// Skip registering for block production. Both the replay stage and PohRecorder
// don't support _concurrent block production_ at all. It's strongly assumed that
// block is produced in singleton way and it's actually desired, while ignoring the
// opportunity cost of (hopefully rare!) fork switching...

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?

Copy link

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

}

pub fn is_preallocated(&self) -> bool {
self.bank.is_none()
}

pub fn mode(&self) -> SchedulingMode {
self.mode
}

pub fn bank(&self) -> &Arc<Bank> {

Choose a reason for hiding this comment

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

should this return Option similar to slot()?

}

#[must_use]
pub fn schedule_or_buffer_task(&mut self, task: Task, force_buffering: bool) -> Option<Task> {

Choose a reason for hiding this comment

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

The force_buffering argument is a bit confusing to me.
The transaction is pushed into the scheduling structure regardless of whether it is blocked or unblocked, right?

The force_buffering seems to just return Some regardless if the transaction is blocked or not?

(true, true)
}
};

Choose a reason for hiding this comment

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

I can't get to the line in review.
There's a series of comments on line 1332:

        // High-level flow of new tasks:
        // 1. the replay stage thread send a new task.
        // 2. the scheduler thread accepts the task.
        // 3. the scheduler thread dispatches the task after proper locking.
        // 4. the handler thread processes the dispatched task.
        // 5. the handler thread reply back to the scheduler thread as an executed task.
        // 6. the scheduler thread post-processes the executed task.

we should update this comment imo, but it doesn't necessarily have to be part of this PR - maybe not until the full implementation of production w/ all verification is in.

@apfitzge apfitzge self-requested a review February 13, 2025 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants