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 task queue selection per agent rather than per scheduler #87

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

shaseley
Copy link
Collaborator

@shaseley shaseley commented Apr 8, 2024

The postTask spec allows the next Scheduler task to be the oldest, highest priority task from any of its task queues, enabling per-frame task prioritization decisions. But HTML task queues are per agent/event loop, and as such other scheduling decisions typically cannot take into account a task's frame. It's not clear there's a benefit to enabling some tasks to be per frame while others are per agent, and removing this simplifies things and makes overall prioritization easier to understand, which is what this change does.


Preview | Diff

@shaseley shaseley force-pushed the per-agent-enqueue-order branch from 1951637 to 5b9e02e Compare April 9, 2024 21:40
@shaseley shaseley requested review from clelland and mmocny April 11, 2024 02:46
@shaseley
Copy link
Collaborator Author

Friendly ping @clelland @mmocny. Not sure if adding as reviewers sent emails or not.

Copy link
Collaborator

@mmocny mmocny left a comment

Choose a reason for hiding this comment

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

LGTM.

I do have a question which I will follow-up with: If each task has a "next enqueue order" and a priority, what happens when you adjust a task priority with a TaskSignal? Would it be re-assigned a new enqueue order (i.e. go to the back of the new priority-level task queue), or would it be "merged into" according to its previous enqueue order?

This might be specified!

@@ -24,25 +24,33 @@ queue=].
With: For each [=event loop=], every [=task source=] that is not a [=scheduler task source=] must be
associated with a specific [=task queue=].

Add: An [=event loop=] has a numeric <dfn for="event loop">next enqueue order</dfn> which is
initialized to 1.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given the note about timestamps as an alternative (below), would this value be entirely optional?

Similarly, in the scheduler task definition, there is an "enqueue order" which is specifically initialized to 0, and then algorithms specifying how to assign and increment -- all these need to be timestamps in that case I think.

While this alternative is likely easily implied by the note below, just checking if you should make explicit reference to all the changes that would be made with that alternative or not.

I wonder if that would be easier if you just have an algorithm for "get next enqueue order value" or something like that. It could be implemented to increase a counter, or to return a unique+increasing timestamp.

Then, perhaps the rest of the references would not care if its a timestamp or number.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, yeah it's optional if you choose to use timestamps. I chose to use an enqueue order since it's easy to understand and matches Chrome's implementation. I also removed the note about using a timestamp since I'm not sure how much that adds -- it's clear in the spec where the value is set, and implementations in general can make different choices as long as the observable behavior is the same.

@shaseley shaseley force-pushed the per-agent-enqueue-order branch from 5b9e02e to 830630f Compare April 29, 2024 21:33
@shaseley shaseley merged commit 11268d8 into WICG:main Apr 29, 2024
2 checks passed
@shaseley shaseley deleted the per-agent-enqueue-order branch April 29, 2024 21:45
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.

2 participants