-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
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
WorkerTaskManager #19650
WorkerTaskManager #19650
Conversation
(There's a "Draft" feature if the PR is in progress. Useful for signaling to others it is WIP. Under the "Reviewers" section of the right sidebar, where it says "Still in progress? Convert to draft") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is looking good! A few comments and questions.
examples/jsm/loaders/TaskManager.js
Outdated
* @author Kai Salmen / https://kaisalmen.de | ||
*/ | ||
|
||
import { FileLoaderBufferAsync } from "./obj2/utils/FileLoaderBufferAsync.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this import from the obj2
folder necessary? It looks like it just adds a custom onProgress callback to the FileLoader, which may not be applicable to all implementations? If it is necessary it should probably be moved into the TaskManager file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The necessity for it is gone. I created it for other prototyping activity and used it because it was there. With the introduction of loadAsync
in Loader
it collapsed to what it is now and I agree whatever is needed (using FileLoader
with setResponseType( 'arraybuffer'
) should be integrated directly in TaskManager
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
examples/jsm/loaders/TaskManager.js
Outdated
* @param {String[]} [dependencyUrls] | ||
* @return {TaskManager} | ||
*/ | ||
registerType ( type, maximumWorkerCount, initFunction, executeFunction, comRoutingFunction, dependencyUrls ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there benefits to having a separate maxWorkerCount for each type of task? The downside, I think, is that you can't effectively load balance across different types of task. If any worker can do any task, that balancing could be easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe there is need for an overall count and a desired per taskType
count. Was that your idea behind cost parameter?
If any worker can do any task, that balancing could be easier.
Is this a realistic scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consider the cost
parameter unrelated to whether or not there are separate worker queues for different task types.
My original idea had been to have a configurable number of workers for the entire TaskManager, say N. Each worker can perform any type of task, and all tasks are balanced across those N workers. So there would be no per-task worker limit, just the TaskManager total limit.
Is this a realistic scenario?
Having multiple tasks running in parallel is realistic, at least. For example, when using both compressed geometry and compressed textures you'll need DRACOLoader and (one of) KTX2Loader or BasisTextureLoader.
Whether there are very many realistic situations where shared queues are likely to give better performance? I'm not sure. If you think this is more complicated, or worse for some other reason, I'm also fine with leaving this as you have it for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original idea had been to have a configurable number of workers for the entire TaskManager, say N. Each worker can perform any type of task, and all tasks are balanced across those N workers. So there would be no per-task worker limit, just the TaskManager total limit.
Ok, now I see your point. Then we need to create N workers for every taskType either lazy or on init (cost are only extra memory and init time what becomes negligible if Workers really do intensive work). Then we can always use maximum of N for every taskType depending on current demand.
From my point of view the Worker should only include the code domain (init, exec) of one task as otherwise I fear mixing task and their dependencies into the worker will introduce unnecessary complexity and mixing module and non-module code will not be manageable. I hope this makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we need to create N workers for every taskType... Then we can always use maximum of N for every taskType depending on current demand.
This is a bit different than what I meant. Something like...
var manager = new TaskManager().setWorkerLimit(2);
var objLoader = new OBJLoader(manager);
var ktx2Loader = new KTX2Loader(manager);
... would create only two Web Workers total, no matter how many loaders are using that manager, and no matter how many resources are loaded. OBJ and KTX2 tasks could be balanced across those two workers, and a single worker's queue would have tasks of both types.
There are some very real advantages to that approach: you have firm control over the total number of Web Workers, and work is evenly distributed. But the concern of mixing module and non-module code makes sense. I'm not sure the module worker type is going to be worth a whole separate API in the long term, but I don't want to hold up this PR on the question – it's fine with me to continue as you have the PR for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When tasks are add/executed it only runs two in parallel ...
This will be very complicated to enforce. What happens if we get requests in this order:
- 10 OBJ parse tasks
- 2 KTX2 parse tasks
After step (1) I would expect that the manager assigns ~5 OBJ tasks to each OBJ worker. But then when it gets to step (2) there are already two workers running, do we have to wait until one of the workers clears its queue before assigning KTX tasks to a KTX worker? What if more OBJ tasks arrive in the meantime?
I would prefer to avoid all that... maybe we have a setWorkerLimit( n )
method on the TaskManager, and for now it will create N workers per task type and use them all. Then in the future, if we decide to, we can try to combine the worker pool so that tasks share workers for more equitable load balancing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add task puts the processing requests to a fifo queue (currently Promises are stored in array). Whenever a worker signals exec complete a task is taken from the queue and a fitting worker is kicked. What is done in the current TaskManager
/example is not so far off expect for the overall count is per task type and not globally, but tasks are already intermixed in the queue. But getting there not be hard. I will prototype this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! I wonder whether the time to signal the main thread that a task is done and then wait for a new task to come back has a measurable impact on completion times? Thinking of a model with 100+ Draco geometries for example... that doesn't seem like much of a problem though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In lastest commit the maximum amount (N) of parallel worker execution is now handled on TaskManager
level. N workers are created of each type (except for main fallback), but only the maximum defined in TaskManager
are executed in parallel in the example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous code did not work as described. I pushed another update that really works as described.
Init time of workers increases linearly with quantity even if Blobs/Strings are cached. For module workers there seems no obvious way to cache code except for pre-loading the module in the embedding html.
Execution on main is still strange. The FakeTaskWorker
is now treated identically during execution, but when it solely used in the example than it completely blocks any rendering. When it is intermixed with real workers, then the rendering is not blocked. Manual triggering of requestAnimationFrame did not help. For now I leave it as it is
examples/jsm/loaders/TaskManager.js
Outdated
* @param {string} type The type as string | ||
* @return boolean | ||
*/ | ||
supportsType ( type ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I think I find the term "task" or "taskType" clearer than just "type" here, and for the methods below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is inconsistent. I obviously forgot to rename it everywhere. Will change this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in latest commit.
examples/jsm/loaders/TaskManager.js
Outdated
* @param {string} workerJsmUrl The URL to be used for the Worker. Module must provide logic to handle "init" and "execute" messages. | ||
* @return {TaskManager} | ||
*/ | ||
registerTypeJsm ( type, maximumWorkerCount, workerJsmUrl ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never seen the term "jsm" anywhere outside the name of this folder, so I'd prefer to use a clearer term here. It's also important that even though the "examples/js" folder is being removed later this year, that has very little to do with whether this "jsm" method should be used — DRACOLoader, BasisTextureLoader, and KTX2Loader will all use the non-JSM register method. What do you think of:
// Default.
manager.registerTask( type, maxCount, initFn, execFn, ... );
// URL. (undecided which name..)
manager.registerTaskModule( type, maxCount, moduleUrl );
manager.registerTaskUrl( ... );
manager.registerTaskSrc( ... );
I think the fact that the script happens to be an ES Module is less important than the fact that it's an external script. That means any user with a build system like webpack or browserify will need to specifically configure their application to output files in the right directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, let's remove jsm from method signatures. module
is the best replacement term as it is also in line with the naming of the Worker
constructor parameter, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in latest commit.
I think one loader in this PR is probably enough to get the general idea. |
20d31aa
to
f6d5847
Compare
…skManager. Maximum number of workers for every type are created, but only maximum number is executed. Removed the need for FileLoaderBufferAsync
…skManager. Maximum number of workers for every type are created, but only maximum number is executed. Removed the need for FileLoaderBufferAsync
TaskManager: - Adjusted naming (taskType and module) - Reorganize execution flow and renamed addTask to enqueueForExecution - Align FakeWorker main execution behaviuor with TaskWorkers: The maximum no of workers is created for all types webgl_loader_taskmanager: - Allow to define overall execution, loop count and meshes to keep visible - Adjusted naming (taskType and module)
f6d5847
to
6593a08
Compare
Do you have any preference? My comfort zone is OBJ, but I am also aware others loaders are more demanded these days. 😉 Should we collaborate? Is R119 a possible target? |
Cool, you can use the TypeScript compiler to generate ts.d files from the jsdoc declarations. Create a config file like this: {
"include": [
"examples/jsm/loaders/TaskManager.js"
],
"compilerOptions": {
"allowJs": true,
"declaration": true,
"noEmit": false,
"emitDeclarationOnly": true,
"removeComments": true
}
} Write good jsdoc and get the definitions for free. 😄 |
f33c2ec
to
f49dda6
Compare
f49dda6
to
827c658
Compare
@donmccurdy and @mrdoob I have fully updated the description. R120 is a realistic target, I think. Sorry it took so long. If I could do this work in my regular job, we would have been here months ago. I should be able to solve open issues in the upcoming weeks. So, if you don't identify potential blockers, then we could merge this before end of August! |
b1a9754
to
7643a0d
Compare
7643a0d
to
6db95fe
Compare
@donmccurdy and @mrdoob I didn't make it for R120, but the Shall we rename the |
Thanks @kaisalmen! I hope to review this in detail soon... I also associate the name with windows task manager, but it seems like a pretty good name. I like the "Manager" suffix for its similarity to the existing LoadingManager. Maybe WorkerManager? Either that or TaskManager seem fine to me. What do you mean by "legacy workers" in the PR description? How confident are you in the updates to the various loaders that depend on this? I guess I'm wondering if we should try to get in the TaskManager itself first, as a clean PR with nothing depending on it, or merge this all at once. |
Then let's keep the name
Classic, normal or standard workers, so one can distinguish between
I have removed the need Resolved 2020-08-30: I need to look again at the execution loop of the |
I'm worried about giving the impression that Module Workers are the better/recommended option, and using the term "legacy" to describe the alternative gives that impression. The key difference between the two is not the use of ES Modules, but the fact that Module Workers rely on external scripts with a pre-determined URL. That's going to be very hard to use with modern bundlers without requiring users to individually put their dependencies in the right place. Note that we are in the process of deprecating |
It was not my intention to make a recommendation here. I have already adjusted the wording to
But, isn't that even more problematic when you use What I esthetically like about module workers is that they remove the inconsistency between main and worker. With standard workers you have to use a different syntax in the worker ( |
1c871c0
to
424c4fc
Compare
Finally, the PR is ready for review. 🚀 I am still updating the code comments in the new example, but this does not block the review from my point of view. Wording regarding workers (standard and module) has been adapted in the new example, |
How about naming this |
... or |
That works too 👌 |
Updated Typescript definitions Removed MaterialCloneInstruction
…k with the unaltered loader.
8cd5896
to
3405ccb
Compare
@donmccurdy and @mrdoob What do we do? Are still interested with this? Is there anything I can do to make this any more desirable? |
Sorry for the long delay here — I'm struggling with what role WorkerTaskManager should play, and where it should be (examples/jsm, separate repo, etc.) to fulfill that. My original motivation for the TaskManager proposal in #18234 was to consolidate worker management logic required by KTX2Loader and DRACOLoader, and WorkerPool.js has basically solved that need, with a very small amount of code. Because this PR provides features like transferring Mesh, Material, and BufferGeometry instances over a wire, it's also about 20x larger. The serialization is valuable in its own right — like a better/faster Do we think it is likely that this framework is also a good fit for something like @gkjohnson's three-mesh-bvh, or doing physics in a worker with cannon-es? I'm not convinced that it is, unfortunately. In comparison, consider a combination of...
... the combination is more flexible for use cases like physics, and provides a threading implementation compatible with both Node.js and Web Workers. I'm wondering if that is the way we should go here instead. |
If code for transferring core types between web workers is going to be added I still strongly vote for the approach posed in #21035 or at least something that's usable separately from any worker infrastructure three.js is choosing to provide. The benefits of such an approach are not limited to use in transferring web workers, as well. Keeping geometry data as the original typed attributes (or even just cloning a typed array directly) is much faster and the typed array can be stored in browser caches like IndexedDB and sent over websockets if needed. |
Yes, I guess your intention what should be achieved with
Yes, I completely agree with you both. It is contained in this PR, but
I actually thought about if using comLink makes sense. Currently, three-wtm, like OBJLoader2 exist as an independent projects now. three-wtm could evolve into something completely independent of three.js (with transfer tools moved to OBJ2 or no longer required). I have to think about where I want to go with it and what makes sense. I have the feeling that this PR won't get merged because it does not bring the right value to three,js, but if you think my assumption is wrong or any of the code/work done here is useful in another scope (elsewhere in three.js or beyond), then let me know/let's talk. 🙂 |
Dear @mrdoob @donmccurdy and @gkjohnson I will now close this PR. I have waited this long, because I wanted the external project three-wtm. to reach a good state, first. Now is the time. The code has been transformed to TypeScript and I performed clean-up, fixed bugs and added new features (e.g. different worker count for each registered task, generic The code and documentation needs some polishing, but V2.0.0 will be there early May (won't have much time to work on in the upcoming week). Maybe this work can be of to you use even if the connection to three.js is getting weaker. Feedback from you is very welcome. If you like drop by the repo. I will announce the release on Twitter. Btw, OBJLoader2(Parallel) is still alive and is now using it as well: |
Release is almost ready. You can try the new examples here: http://wtd.kaisalmen.de/ |
Last Update 2021-01-28
The
TaskManager
allows to register a task expressed by an initialization and an execution function with an optional comRounting function to be run a web worker. It creates one to a maximum number of workers that can be used for execution. Multiple execution requests can be handled in parallel of the main task, If all workers are currently occupied the requested are enqueued and the returned promise is fulfilled once a worker becomes available again.What the implement status?
exec
andinit
functions can be declared in module and then be packaged in standard worker if needed. I used this to define the worker code once and support both code pathsit just utilizes the parser
examples/jsm/taskmanager
WorkerTaskManager
OBJLoader2
OBJLoader2
andOBJLoader2Parallel
have been removed from three.js with R125. I will ensure it will work withWorkerTaskManager
and also use simplified worker transfer functions (for all interested see here)TODO
TaskManager
is passed to multiple instances of the same loader/class requesting initenqueueForExecution
and_kickExecutions
OBJLoader
(standard and module version).BufferGeometry
,Material
(meta-information) andMesh
bi-driectionally between Main and workers. All "transferables" shall be treated as such. This replaces old functions available withOBJLoader2
Questions
Later