Add priority queueing for Mutex and Semaphore #75
Conversation
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Good thinking, will adjust.
Thanks for the compliment 😏 I usually am not a 100% coverage guy, but I think it really pays off for that type of code. |
|
Reversed the order of the list search when queuing new tasks. |
|
@DirtyHairy does this look alright? |
|
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. |
|
Holy smokes, that looks like an awful situation... Good luck |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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
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); |
There was a problem hiding this comment.
Again, I think the case is redundant, as -1 + 1 = 0
|
Welcome back! I removed the redundant branches and calls to |
|
... and merged ;) |
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: