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

Add priority queueing for Mutex and Semaphore #75

Merged
merged 20 commits into from
Mar 11, 2024

Conversation

dmurvihill
Copy link
Contributor

This change required a significant restructuring of the scheduler, which was pretty straightforward thanks to the high quality of the test suite. I caught at least one bug that definitely would have made it into the PR if I hadn't started from 100% code coverage.

Notes:

  • Previously, lighter items were always scheduled first. Now, weight is no longer a factor in scheduling order. Items are scheduled first in priority order and then in FIFO order at the same priority. This also fixes a potential issue where a steady stream of many small tasks could crowd out a more important larger task.
  • The queue is no longer broken up by weight; everything is in one big queue in priority order. The amount of time spent on shifting the queue around will increase for consumers that queue a large number of tasks at a few weights. For consumers that create a large number of weights or a large number of semaphores, we will save time on instantiating lists.
  • The priority queue is implemented as an array. It might be better to use a heap-based queue, or it could just waste time on overhead. Without profiling some real users, I couldn't guess which.

Dolan Murvihill and others added 14 commits January 22, 2024 14:35
Adds a second optional argument `nice` to the interface definitions for `acquire` and related functions. This is a non-compiling change created for discussion purposes. Not to be merged until the feature is complete.

Some functions now have two optional arguments, making the interface a
bit clunky. Consider altering it to accept a configuration object next
time a breaking change is released.
The previous commit made all tasks of the same weight execute in order
of niceness, but lighter items were still executed first. That obviously
defeats the purpose of having a priority parameter.

Now, the least nice items are scheduled first. At the same priority
level, execution order is now FIFO. Weight is no longer a factor in
execution order.

The execution queue is now implemented using a single JavaScript array,
rather than an array of arrays by weight. Performance will be noticeably
worse when scheduling a very (very) large number of tasks, especially
when they have diverse weights, but probably better with a large number
of semaphores with few tasks due to fewer array allocations. If there are
users for whom this is a concern, a good heap-based priority queue could
be added as a dependency.
Copy link
Owner

@DirtyHairy DirtyHairy left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long for the review. I like what you have done. Treating weights in FIFO order is a good thing I think, and I think the performance hit from using a single queue is acceptable --- I don't think weights are commonly used anyway.

I have left a comment on what I think is a worthwhile optimisation, and I think I have identified a bug 😏

queueEntry.resolve([previousValue, this._newReleaser(previousWeight)]);
private _dispatchQueue(): void {
this._drainUnlockWaiters();
while (this._queue.length > 0 && this._queue[0].weight <= this._value) {
Copy link
Owner

Choose a reason for hiding this comment

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

This loop will stop scanning the queue if the next item has a weight that exceeds the current value, even if there are other items further up in the queue that could be scheduled. You need to keep scanning through the whole queue.

Copy link
Contributor Author

@dmurvihill dmurvihill Feb 1, 2024

Choose a reason for hiding this comment

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

That would allow light low-priority items to crowd out heavier high-priority items. The queue could not guarantee eventual completion of a high-priority item.

Copy link
Owner

Choose a reason for hiding this comment

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

Hm, you have a point there. I still don't like that a single heavy task can forever block all lower priority tasks, but I don't see a good way out either. Let's leave it like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened a separate pull request with a test to clarify this issue.

src/Semaphore.ts Outdated

this._dispatch();
const task: QueueEntry = { resolve, reject, weight, priority };
const i = this._queue.findIndex((other) => priority > other.priority);
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can optimise for what I think is the most common use case (no weights, no priorities) here by scanning the array for an item with higher priority from the end instead of scanning for an item with lower priority from the start

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good thinking, will adjust.

@DirtyHairy
Copy link
Owner

This change required a significant restructuring of the scheduler, which was pretty straightforward thanks to the high quality of the test suite. I caught at least one bug that definitely would have made it into the PR if I hadn't started from 100% code coverage.

Thanks for the compliment 😏 I usually am not a 100% coverage guy, but I think it really pays off for that type of code.

@dmurvihill
Copy link
Contributor Author

Reversed the order of the list search when queuing new tasks.

@dmurvihill
Copy link
Contributor Author

@DirtyHairy does this look alright?

@DirtyHairy
Copy link
Owner

Sorry for the radio silence @dmurvihill ! Yes, this looks good, but I am currently busy dealing with the fallout of Apple dropping support for PWAs in the EU. Once I am done with this I will take the time for a last review and merge.

@dmurvihill
Copy link
Contributor Author

Holy smokes, that looks like an awful situation... Good luck

Copy link
Owner

@DirtyHairy DirtyHairy left a comment

Choose a reason for hiding this comment

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

Sorry again for the long delay, but I am back from Apple hell and promise that I will not disappear again 😏 I prepared a native wrapper app, and prepared numerous changes to make running my PWA as a plain web page a bit smoother, just to be (pleasantly) surprised by Apple backflipping on the PWA issue.

Anyway, I have a few more minor comments, but after those I'll merge and release. Thanks again for the work you put into this!

src/Semaphore.ts Outdated
if (i === -1 && weight <= this._value) {
// Needs immediate dispatch, skip the queue
this._dispatchItem(task);
} else if (i === -1) {
Copy link
Owner

Choose a reason for hiding this comment

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

This and the next case could be merged, as -1 + 1 = 0

src/Semaphore.ts Outdated
} else {
this._queue.splice(i + 1, 0, task);
}
this._dispatchQueue();
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder whether we actually still need the call to _dispatchQueue here. Scheduling should only happen if the new item is moves all the way to the start of the queue and has suitably low weight, and that case is already taken care of by the first branch.

src/Semaphore.ts Outdated
return new Promise((resolve) => {
if (!this._weightedWaiters[weight - 1]) this._weightedWaiters[weight - 1] = [];
insertSorted(this._weightedWaiters[weight - 1], { resolve, priority });
this._dispatchQueue();
Copy link
Owner

Choose a reason for hiding this comment

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

Again, I am not really sure whether we need this anymore, I think that case is already handled by the first branch.

src/Semaphore.ts Show resolved Hide resolved
src/Semaphore.ts Outdated
function insertSorted<T extends Priority>(a: T[], v: T) {
const i = findIndexFromEnd(a, (other) => v.priority <= other.priority);
if (i === -1) {
a.splice(0, 0, v);
Copy link
Owner

Choose a reason for hiding this comment

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

Again, I think the case is redundant, as -1 + 1 = 0

@dmurvihill
Copy link
Contributor Author

Welcome back! I removed the redundant branches and calls to _dispatchQueue from the queue insertion code. Cheers.

@DirtyHairy DirtyHairy merged commit 43e8858 into DirtyHairy:master Mar 11, 2024
2 checks passed
@DirtyHairy
Copy link
Owner

... and merged ;)

@dmurvihill dmurvihill deleted the priority branch March 11, 2024 21:37
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