-
Notifications
You must be signed in to change notification settings - Fork 47
Add specification for scheduler.yield() method #88
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
Conversation
66af95a
to
c33a507
Compare
@@ -207,6 +279,20 @@ A <dfn>task handle</dfn> is a [=struct=] with the following [=struct/items=]: | |||
[=task/runnable=]. | |||
</div> | |||
|
|||
<div algorithm> |
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 know if it's better or not, but this could also be a simple lookup from a table with 6 rows in it, rather than an algorithm.
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.
Yeah that's probably easier to read. I don't know how it should be formatted, however (using defaults for now).
spec/scheduling-tasks.md
Outdated
1. Otherwise if |priorityOption| is not null, then set |result|'s [=scheduling state/priority | ||
source=] to the result of [=creating a fixed priority unabortable task signal=] given | ||
|priorityOption|. | ||
1. Otherwise if |abortOption| is not null and [=implements=] the {{TaskSignal}} interface, then |
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.
Should this attempt to handle the case where |abortOption| is "inherit", and the interited abort source is a TaskSignal?
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.
In this algorithm, |abortOption| only applies to determining the abort component, in which case it's irrelevant.
If original signal should also be used for priority, then the caller will set the |priorityOption| to "inherit" (step 5 in the algo above). If the original signal was a TaskSignal, it gets set as both the abort and priority sources in the |inheritedState|, and it funnels into this check.
</div> | ||
|
||
<div algorithm> | ||
To <dfn>compute the scheduling state from options</dfn> given a {{Scheduler}} object |scheduler|, |
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.
Overall, I find this algorithm hard to follow; I've written and deleted at least three comments where I thought there were errors already :)
Something structured like this might be clearer:
- Let result be...
- Let inheritedState be...
- If abortOption is "inherit":
- if inheritedState is not null, then set result’s abort source to inheritedState’s abort source.
- Otherwise, set result’s abort source to abortOption.
- If priorityOption is "inherit":
- if inheritedState is not null, then set result’s priority source to inheritedState’s priority source.
- Otherwise, if priorityOption is not null, then set result’s priority source to the result of creating a fixed priority unabortable task signal given priorityOption.
- Otherwise, if abortOption is not null and implements the TaskSignal interface, then set result’s priority source to abortOption. (although I would have expected this to reference result's abort source, rather than abortOption here)
- If result’s priority source is null, then set result’s priority source to the result of creating a fixed priority unabortable task signal given "user-visible".
- Return result.
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.
Heh, I want back and forth on this several times when drafting this. IIUC this was mostly rewriting 3 & 5? Just making sure I didn't miss anything (changed those in latest version).
Re: using abortOption
in step 7 -- this handles the case where the arguments are of the form {signal: someSignal}
, where there's no inheritance and the signal could be either a TaskSignal or plain AbortSignal. If it's a TaskSignal, then that priority gets used; otherwise, it defaults to "user-visible" in step 8.
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.
Thanks -- I missed that possibility
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.
Thanks for the reviews! Really helpful feedback. I think I've addressed everything; PTAnL.
spec/scheduling-tasks.md
Outdated
|
||
The priority and abort properties of the continuation are determined in a similar way as | ||
{{Scheduler/postTask()}}, but can additionally be inherited from the originating task. If | ||
neither the {{SchedulerYieldOptions/signal}} nor the {{SchedulerYieldOptions/priority}} |
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 neither. signal
can also include priority (if it's a TaskSignal
), in which case you'd be overriding that with the inherited priority and would have no way to specify to use the signal's priority.
Making both required wouldn't work either -- the priority
option (which is there for convenience vs. always requiring a TaskSignal
) overrides the signal
's priority, which isn't typically what you'd want if you provide a TaskSignal
. (Note: this is how postTask()
works as well % inheritance).
spec/scheduling-tasks.md
Outdated
1. Otherwise if |priorityOption| is not null, then set |result|'s [=scheduling state/priority | ||
source=] to the result of [=creating a fixed priority unabortable task signal=] given | ||
|priorityOption|. | ||
1. Otherwise if |abortOption| is not null and [=implements=] the {{TaskSignal}} interface, then |
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.
In this algorithm, |abortOption| only applies to determining the abort component, in which case it's irrelevant.
If original signal should also be used for priority, then the caller will set the |priorityOption| to "inherit" (step 5 in the algo above). If the original signal was a TaskSignal, it gets set as both the abort and priority sources in the |inheritedState|, and it funnels into this check.
</div> | ||
|
||
<div algorithm> | ||
To <dfn>compute the scheduling state from options</dfn> given a {{Scheduler}} object |scheduler|, |
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.
Heh, I want back and forth on this several times when drafting this. IIUC this was mostly rewriting 3 & 5? Just making sure I didn't miss anything (changed those in latest version).
Re: using abortOption
in step 7 -- this handles the case where the arguments are of the form {signal: someSignal}
, where there's no inheritance and the signal could be either a TaskSignal or plain AbortSignal. If it's a TaskSignal, then that priority gets used; otherwise, it defaults to "user-visible" in step 8.
Implementation notes:
yield()
andpostTask()
:postTask()
andyield()
postTask()
andyield()
priority with notion of an "effective priority", which is used for picking the next task to run (yield()
effective priority for a given task priority is just higher thanpostTask()
equivalent).postTask()
callback and idle callbacksNote: things get a little awkward regarding continuation vs. task priority IDL since we can't yet inherit from enums (ContinuationPriority is TaskPriority + "inherit"). And we sometimes pass one type as another where they are compatible (i.e. same string values permitted).
Preview | Diff