feat(webworker): Simplify the API for running a compute task off the main thread in a worker #891
Conversation
RequestPoolManager
configuration.
external modules
package.json and karma.conf.js.
RequestPoolManager
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
webWorkerManager.js and improve console output in example-runner-cli.js
sedghi
changed the title
…at/workerpool-compute
Babel config and package.json
| @@ -99,7 +99,7 @@ async function run() { | |||
| toolGroup.addTool(WindowLevelTool.toolName); | |||
| toolGroup.addTool(PanTool.toolName); | |||
| toolGroup.addTool(ZoomTool.toolName); | |||
| toolGroup.addTool(StackScrollMouseWheelTool.toolName, { loop: true }); | |||
| toolGroup.addTool(StackScrollMouseWheelTool.toolName, { loop: false }); | |||
There was a problem hiding this comment.
This seems unrelated - that is probably ok, but wasn't sure you intended to have this.
There was a problem hiding this comment.
Yeah the demo was feeling weird that is why i have it reverted
termination on idle workers
| @@ -97,15 +97,15 @@ module.exports = function (config) { | |||
| }, | |||
| // NOTE: For better debugging you can comment out the | |||
| // istanbul-instrumenter-loader below | |||
| { | |||
There was a problem hiding this comment.
Should this be commented out - seems like this is a change to the coverage information we are seeing.
There was a problem hiding this comment.
Karma hates the comlink package. coverageIstanbulReporter is responsible for coverage based on my understanding not this one
…at/workerpool-compute
| Prefetch = 'prefetch', | ||
| /** Lower priority, often used for background computations in the worker */ |
There was a problem hiding this comment.
I think I would say instead that this request type does not consume bandwidth as it is local compute. Thus, it has a different queue to use. It is not whether the priority is lower or higher, it is different.
There was a problem hiding this comment.
No, it is, in fact, lower in priority when queued. This should be comparing a decoding task versus a computation task.
| const workerInstances = workerProperties.instances.filter( | ||
| (instance) => instance !== null | ||
| ); | ||
|
|
||
| let minLoadIndex = 0; | ||
| let minLoadValue = workerProperties.loadCounters[0] || 0; | ||
|
|
||
| for (let i = 1; i < workerInstances.length; i++) { | ||
| const currentLoadValue = workerProperties.loadCounters[i] || 0; | ||
| if (currentLoadValue < minLoadValue) { | ||
| minLoadIndex = i; | ||
| minLoadValue = currentLoadValue; | ||
| } | ||
| } |
There was a problem hiding this comment.
The worker could be found in O(1) if they were stored in a min-heap. But I don't think cornerstone has an implementation of a heap, right? At least the number of WebWorkers is very small and that should not impact the performance.
PS: requestPoolManager could also use a Priority Queue (max heap) instead of sorting the priorities whenever getNextRequest is called.
We can make that change in a near future if it's worth it. 😉
There was a problem hiding this comment.
Makes a lot of sense, a good candidate for v2.x
| } | ||
|
|
||
| // Update the load counter. | ||
| workerProperties.loadCounters[minLoadIndex] += 1; |
There was a problem hiding this comment.
If there are 3 web workers for heavy computations and 15+ tasks (maxNumRequests.compute) each one would have 1 running + 4 in a Chrome/Comlink queue, right? As I commented in the previous PR my only concern is what happens with those tasks when "terminate" is called.
There was a problem hiding this comment.
Add a comment about number of workers and load balancing
| this.maxNumRequests = { | ||
| interaction: 6, | ||
| thumbnail: 6, | ||
| prefetch: 5, | ||
| compute: 15, |
There was a problem hiding this comment.
Maybe we should change maxNumRequests, requestType, etc. to a different name because before this change those three first options were related to image requests (download) but now compute was added and it is not related to that.
registration and task execution methods
Context
This PR reworks the web worker API and proposes a new set of methods to work with workers to make developers life so much easier to be able to run a function of the main thread
Changes & Results
adds webWorkerManager to the Core
Note: Currently, we will not transfer the decoders from the old web worker manager to this new one until the next version of cornerstone3D. The only thing you need to be careful is how many compute workers you create (on top of the decode workers which you can configure in the dicom image loader) so that your system can handle it.
Testing
Follow the steps here
https://deploy-preview-891--cornerstone-3d-docs.netlify.app/docs/concepts/cornerstone-core/webWorker
Checklist
PR
semantic-release format and guidelines.
Code
[] My code has been well-documented (function documentation, inline comments,
etc.)
[] I have run the
yarn build:update-apito update the API documentation, and havecommitted the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)
Public Documentation Updates
additions or removals.
Tested Environment