Skip to content

Add woken_while_running as another argument to the scheduling function #42

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

Merged
merged 7 commits into from
Mar 23, 2023

Conversation

js2xxx
Copy link
Contributor

@js2xxx js2xxx commented Mar 22, 2023

Currently, it has been found that the return value of the Runnable::run() function cannot be directly used in the scheduling function. As a result, the scheduler cannot determine whether the task is self-yielding or has been woken up by some external source when it is scheduled to run on some queues.

This situation can occur when a scheduler wants to set up some lifo_slot to allow tasks to preempt the normal execution order, but it receives a scheduling request from a self-yielding task that woke itself up while running. In this case, the scheduler cannot let the task preempt and cause a "false-yielding" result or some sort. Therefore, it needs to know whether the task self-yielded or not. However, with the current implementation, the scheduler can only acquire this information AFTER it schedules the task. This means that it needs to schedule the task for a second time, pushing it to the run queue, which not only takes much more time but also sadly moves the possible last preempted task into the run queue as well.

My pull request is to give the information to the scheduling function and let the scheduler determine in the first place, which in fact is inspired by Tokio's implementation. All the present APIs remain unchanged, and three new kinds of functions, namely spawn2, spawn_local2, and spawn_unchecked2, are added. The names of these functions can be changed if there is a better idea. Also, the corresponding tests have yet to be added.

/// A new API.
pub fn spawn2<F, S>(future: F, schedule: S) -> (Runnable, Task<F::Output>)
where
    F: Future + Send + 'static,
    F::Output: Send + 'static,
    S: Fn(Runnable, bool) + Send + Sync + 'static,
{
    unsafe { spawn_unchecked2(future, schedule) }
}

/// A present API, redirected to the corresponding new one.
pub unsafe fn spawn_unchecked<'a, F, Fut, S>(
    self,
    future: F,
    schedule: S,
) -> (Runnable<M>, Task<Fut::Output, M>)
where
    F: FnOnce(&'a M) -> Fut,
    Fut: Future + 'a,
    S: Fn(Runnable<M>),
    M: 'a,
{
    self.spawn_unchecked2(future, move |task, _| schedule(task))
}

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

I can't review this now, but it seems like having a spawn2 version of every function would be garish. Not only would it create two versions of every function, but it would set a precedent for creating a spawn3 family of functions if we needed to break again.

Here's an idea; what do you think of having a Schedule trait, like so:

pub trait Schedule<M> {
    fn schedule(&self, runnable: Runnable<M>, info: ScheduleInfo);
}

impl<M, F> Schedule<M> for F where F: Fn(Runnable<M>)
{
    ...
}

At this point, you'd change the function bound for schedule to implement Schedule instead of Fn(Runnable<M>). Then, for your use case, you would add:

pub struct WithInfo<F>(pub F);

impl<M, F> Schedule<M> for WithInfo<F> where F: Fn(Runnable<M>, ScheduleInfo) {
    ...
}

Finally, ScheduleInfo would be a non exhaustive structure with a bool as a field, so that we can add more in the future.

This ensures that semver isn't broken, since all we're doing is relaxing trait bounds.

@js2xxx
Copy link
Contributor Author

js2xxx commented Mar 23, 2023

Seems a good idea. I will try it.

@js2xxx js2xxx requested a review from notgull March 23, 2023 03:02
@js2xxx js2xxx requested a review from notgull March 23, 2023 11:14
@js2xxx js2xxx requested a review from notgull March 23, 2023 17:17
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Thanks!

@notgull notgull merged commit 00581f5 into smol-rs:master Mar 23, 2023
@js2xxx js2xxx deleted the sched2 branch March 24, 2023 01:19
@js2xxx
Copy link
Contributor Author

js2xxx commented Mar 24, 2023

You're welcome. :) BTW, when would you release a new version?

@notgull
Copy link
Member

notgull commented Mar 24, 2023

I'll release once I land #44

@notgull notgull mentioned this pull request Mar 24, 2023
notgull added a commit that referenced this pull request Mar 24, 2023
- Ensure that the allocation doesn't exceed `isize::MAX` (#32)
- Add `FallibleTask::is_finished()` (#34)
- Add a metadata generic parameter to tasks (#33)
- Add panic propagation to tasks (#37)
- Add a way to tell if the task was woken while running from the schedule function (#42)
@notgull
Copy link
Member

notgull commented Mar 24, 2023

Released in v4.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants