plugins icon indicating copy to clipboard operation
plugins copied to clipboard

fix(terser): always make at least 1 worker thread

Open easrng opened this issue 2 years ago • 8 comments

Rollup Plugin Name: terser

This PR contains:

  • [x] bugfix
  • [ ] feature
  • [ ] refactor
  • [ ] documentation
  • [ ] other

Are tests included?

  • [x] yes (bugfixes and features will not be merged without tests)
  • [ ] no

Breaking Changes?

  • [ ] yes (breaking changes will not be merged unless absolutely necessary)
  • [x] no

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: Resolves #1594

Description

On some platforms (I noticed it on Android), os.cpus() can return [], resulting in no workers being spawned, so the jobs never run. This patch makes sure the maximum number of workers is never 0.

easrng avatar Oct 08 '23 03:10 easrng

A future enhancement could be to use os.availableParallelism (requires v18.14+), since it's recommended over using os.cpus().length.

dasa avatar Oct 09 '23 17:10 dasa

Looks like you need an upstream update:

git remote add upstream [email protected]:rollup/plugins.git
git fetch upstream
git merge upstream/master
git push

That should get you set so that Windows Node 20 action passes.

shellscape avatar Oct 09 '23 19:10 shellscape

Alright I updated your fork for you from upstream/master and looks like there's a failure on Windows directly related to WASM. Please have a look.

shellscape avatar Oct 15 '23 15:10 shellscape

Alright I updated your fork for you from upstream/master and looks like there's a failure on Windows directly related to WASM. Please have a look.

Aren't V8 errors either bugs in node itself or OOMs usually?

easrng avatar Oct 16 '23 19:10 easrng

Not sure in this case. It doesn't happen on the Ubuntu builds. The existing code doesn't throw this error, so it's possible the changes have surfaced a bug.

shellscape avatar Oct 16 '23 20:10 shellscape

Maybe this change is worth a try?

diff --git a/packages/terser/test/test.js b/packages/terser/test/test.js
index 9dabacd..fd36d58 100644
--- a/packages/terser/test/test.js
+++ b/packages/terser/test/test.js
@@ -360,7 +360,9 @@ test.serial('terser preserve vars in nameCache when provided', async (t) => {
 });
 
 test.serial('minify when os.cpus().length === 0', async (t) => {
-  const original = os.cpus;
+  const originalAP = os.availableParallelism;
+  const originalCPUS = os.cpus;
+  os.availableParallelism = null;
   os.cpus = () => [];
   try {
     const bundle = await rollup({
@@ -373,6 +375,7 @@ test.serial('minify when os.cpus().length === 0', async (t) => {
     t.is(output.code, '"use strict";window.a=5,window.a<3&&console.log(4);\n');
     t.falsy(output.map);
   } finally {
-    os.cpus = original;
+    os.availableParallelism = originalAP;
+    os.cpus = originalCPUS;
   }
 });

It passes locally on Node 21.5.0.

dasa avatar Dec 22 '23 23:12 dasa

@shellscape I can't tell if the tests ran after @easrng last commit. If not, would it be possible to run them again?

dasa avatar Jul 30 '24 22:07 dasa