feat(terser): Update WorkerPool to reuse Workers #1409
Conversation
|
@tada5hi can you review this please? |
There was a problem hiding this comment.
tests included was checked, and while technically there are tests, they were only modified to check for a new error value. I'd like to see some more robust testing in place to prevent regression: something like providing some basic reporting from the plugin that we could analyze - namely the number of workers used, so that worker re-use could be verified. Note that I'm not speaking about log output specifically, but if that's the only option then that would work.
Is there another plugin that has something like this so I can see how the mechanics of that could work? e.g., is it another export from the |
|
there isn't to my knowledge, which is part of the reason I left it open-ended. I wouldn't even care if it was really dirty, just seeking to protect your changes against regression in the future. Give it a poke, and if at the end you don't have anything that's usable for determining that in a test, we'll make a note and move on. |
|
@shellscape I've updated the tests. @tada5hi sorry, I'm not seeing the notes. |
sorry i forgot to submit the review 🙈 |
|
@tada5hi I believe I've addressed all your feedback. Please let me know if there's anything else. |
|
@tada5hi please try to wait for two approvals, thats the standard we tend to follow. if after a week or so no one has stepped up to review, we'll ping people direct and if at that point it's crickets, we'll merge. we're definitely not in a rush to merge in this project. |
@shellscape Oh dam I'm sorry about that. I assumed from past experience that one approval would be enough. I will definitely keep it in mind in the future and follow the convention. Would like to apologize again for the circumstances. |
|
No worries, no need to apologize. We should probably write that down somewhere. |
|
@shellscape Then I am reassured |
Rollup Plugin Name:
terserThis PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers: #1341 (review)
Description
The existing WorkerPool creates a new Worker instance for every chunk. This PR reuses them instead. There is still a max number of workers, and the pool size still increases based on demand up to this max.
In my testing, on a 10 CPU machine, this reduced the build time from about 4.5 minutes to 2.5 minutes. This is for a large project with about 8,000 chunks.
I refactored the WorkerPool based on this Node doc example. I changed it to not create all the workers immediately though since this isn't needed in simple cases.
The changes to
module.tsare based on the logic in https://github.com/TrySound/rollup-plugin-terser/blob/master/rollup-plugin-terser.js This is needed since the Workers now need to be explicitly terminated.Similar to PR #1394, this PR also improves the error messages.