Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: allow custom pools #4417
feat: allow custom pools #4417
Changes from all commits
7b618cc
3bca509
36aa1a5
6ffabd3
e1fe2b4
02373bd
719b634
1db5190
e20059a
d60db58
49d3afe
bc92eef
fba232a
0e0f517
1ae44c9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This issue seems to be in
main
branch too, but all pools are executed parallel here, right?If user has
threads
pool andvmThreads
pools set, and each are using default max-threads, the execution will spawn too many workers at the same time.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 how it works. That's why I wanted agnostic implementation of tinypool.
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.
Tinypool can run
worker_threads
andchild_process
parallel.https://github.com/tinylibs/tinypool/blob/999778ed8acccf18303760558f807f0e208c8b3b/test/runtime.test.ts#L151-L170
It's possible to push all
threads
tasks in the pool, then wait for queue to be empty (remaining tasks are running, nothing in queue), and then change theruntime
and push more tasks in the queue to wait for previous ones to finish. Like here is done:vitest/packages/vitest/src/node/pools/threads.ts
Lines 171 to 174 in ab55637
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.
Ideally, I would like an API that we can expose to reuse the same Tinypool instance in different pools:
Maybe instead of having separate
worker.js/child.js/vmWorker.js
files we have a single file where inside of a context we pass down information on how we want to run it? So, we don't need to changefilename
option in Tinypool. This should also make it easier for external pools to reuse itThere 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 like the idea. From Tinypool's side I think that kind of API should already be possible - some kind of wrappers around it might be needed though.
There's also some need for refactoring in threads and forks pools to reduce code duplication. While doing that it might be good to look into this kind of API. But all this can be left out of this PR as it's not relevant to these changes.