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

Make prioritization stricter #71

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shaseley
Copy link
Collaborator

@shaseley shaseley commented Mar 29, 2023

(Work in progess)

  • Make ordering guaranteed between schedulers associated with the same event loop. All other scheduling decisions are per-agent (% timer ordering, which uses method context), so removing this simplifies things and reduces interop risk.
  • Make background priority tasks have lower event loop priority than tasks in the event loop's queues.
  • Make user-blocking tasks have a higher event loop priority than timers and postMessage (other scheduling mechanisms), and add a note about intent of user-blocking tasks.

Preview | Diff

@shaseley shaseley linked an issue Mar 29, 2023 that may be closed by this pull request
@shaseley shaseley marked this pull request as ready for review April 4, 2023 23:49
@shaseley
Copy link
Collaborator Author

shaseley commented Apr 5, 2023

Hey @domenic, mind taking a look? Thanks.

@shaseley shaseley requested a review from domenic April 5, 2023 02:08
1. If |scheduler queue| is not null and |queues| is not [=list/empty=], then:
1. If |scheduler queue|'s [=scheduler task queue/priority=] is {{TaskPriority/user-blocking}},
then [=set/remove=] from |queues| any [=task queue=] whose [=task source=] is in
«[=timer task source=], [=posted message task source=]».
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will delay behind all posted messages, not just same-Window ones. The below note doesn't seem to align with that.

If you wanted to restrict to same-window ones, you'd need to mark the queued tasks in some way, presumably at the time they're enqueued. I might try something like moneypatching https://html.spec.whatwg.org/multipage/web-messaging.html#posted-message-task-source to contain "If ..., the [task] queued in such a way is a same-window posted message task", and then checking that here.

1. Let |scheduler queue| be the result of [=selecting the next scheduler queue from all schedulers=]
given the [=event loop=].
1. If |scheduler queue| is not null and |queues| is not [=list/empty=], then:
1. If |scheduler queue|'s [=scheduler task queue/priority=] is {{TaskPriority/user-blocking}},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Quotes around strings like user-blocking, here and elsewhere.

then [=set/remove=] from |queues| any [=task queue=] whose [=task source=] is in
«[=timer task source=], [=posted message task source=]».
1. Otherwise if |scheduler queue|'s [=scheduler task queue/priority=] is
{{TaskPriority/background}}, then set |scheduler queue| to null.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add a note here: something like

This means we won't select |scheduler queue|, instead selecting from the non-empty |queues| set.

given the [=event loop=].
1. If |scheduler queue| is not null and |queues| is not [=list/empty=], then:
1. If |scheduler queue|'s [=scheduler task queue/priority=] is {{TaskPriority/user-blocking}},
then [=set/remove=] from |queues| any [=task queue=] whose [=task source=] is in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Task queues don't have associated task sources. Instead, "every task source must be associated with a specific task queue". So e.g. a given task queue could have some timer task source-tasks, some networking tasks, etc.

I think what you want to check here is the task source of the next runnable task in the task queue. That's kind of weird, but I'm not sure there's a better option...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ACK, ugh I knew that :(. I'm OOO next week, I'll work on cleaning it up after. Thanks for the review - much appreciated!

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.

Specify event loop integration / priority intent
2 participants