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

Code documentation: task scheduling #37

Closed
hwchen opened this issue Apr 27, 2020 · 3 comments · Fixed by #67
Closed

Code documentation: task scheduling #37

hwchen opened this issue Apr 27, 2020 · 3 comments · Fixed by #67

Comments

@hwchen
Copy link
Contributor

hwchen commented Apr 27, 2020

I think this issue might be appropriate, as smol is also intended as a project to help people learn about async executors.

Overall, I found the code very easy to follow (thanks @stjepang !). The one place I was completely confused was how/where there was differentiation between awake and asleep tasks.

I finally semi-randomly read the docs for async_task::Task and realized that after running a task, its Task ref will be gone if Poll::Pending, but the schedule function would take a reference to an awoken task and reschedule it somehow. https://docs.rs/async-task/3.0.0/async_task/struct.Task.html . As soon as I read it I knew what was going on, but since I wasn't familiar with it beforehand it took a while to find this information.

I think I was confused because I expected the scheduling to be handled from the executor side, instead of the task side, if that makes any sense. (i.e. I was trying to figure out the queue in the executor where the asleep tasks were kept).

Anyways, I now understand that the schedule closure at https://github.com/stjepang/smol/blob/master/src/thread_local.rs#L81 is meant for rescheduling woken up tasks, and not just on the initial spawn. I guess to me schedules a runnable task was not enough information there, since I've been thinking in terms of waking.

Perhaps someone else would not have gotten stuck here, but I just wanted to share my experience in case it helps somebody else navigate the code.

Let me know if this feedback is helpful, and if it would be helpful for me to try to clarify in the code comments.

@ghost
Copy link

ghost commented Apr 27, 2020

Thank you for the feedback! Perhaps it'd be useful to define somewhere what words mean, i.e. what is a 'task' and 'future', or what does 'schedule' and 'wake' mean.

Is this a useful comment to you, did you find it (perhaps it was not easy to find)?
https://github.com/stjepang/smol/blob/5acde03d5a971054603dcbbc616963270e56cdd2/src/task.rs#L14-L22

@hwchen
Copy link
Contributor Author

hwchen commented Apr 27, 2020

Ah, I don't believe I looked that closely at that comment. My tunnel vision here was to look really hard at the reactor and executor modules, but not look as closely at the task module.

That comment is pretty helpful. I think that I didn't really understand what Runnable meant on first pass, I thought it just generically meant Task and didn't dig too much because I didn't expect it to be as important as the executor for my understanding.

Maybe additionally having something like "When a task is woken up, its Task reference is recreated and passed to the schedule function. In most executors, scheduling simply pushes the Task reference into a queue of runnable tasks." from the async_task docs would make it even clearer.

Perhaps in this case, it's not even about better definitions, but about providing breadcrumbs to the connections. While your comment definitely helps with my deeper understanding of Runnable, it may not have jumped out to me when trying to put the pieces together, unless I saw awake task ... schedule... executor... queue.

And perhaps it's also just a case of me having an incorrect model which made it hard for me to see what's in front of me. :)

So, it may be good to wait and see if others have the same issue I do before deciding whether to do anything.

@ghost
Copy link

ghost commented Apr 28, 2020

I see, thank you for the elaborate explanation of your thought process :)

I can try to improve the comments later, but if you'd like to help and add them yourself, go ahead! I believe you could even just click "edit" on GitHub, add more comments, and submit a PR.

@ghost ghost closed this as completed in #67 Apr 30, 2020
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

1 participant