Add workerPoolConcurrency as a configuration option #5636

anuragkalia · 2023-08-03T08:33:30Z

What's the problem this PR addresses?

This PR introduces a configuration option to set a limit to the concurrency in the worker pool. Right now, it reads from require('os').cpus() which can be overstated in certain CI system which are running many pods on the same machine. This, in turn, can mean the container running can crash. More generally, it is a good idea to let the user decide the CPU concurrency they want to employ.

Closes #5635

How did you fix it?

I added this option as a feature ala networkConcurrency. And sent it as a constructor arg to new WorkerPool(). The default value remains nodeUtils/availableParallelism() so that this is opt-in and the current behaviour remains in tact in future. The configuration also has the advantage of not exceeding the global number of workers if another part of the codebase needs worker threads for themselves.

Checklist

I have read the Contributing Guide.

I have set the packages that need to be released for my changes to be effective.

I will check that all automated PR checks pass before the PR gets reviewed.

anuragkalia · 2023-08-03T09:20:02Z

Will figure out the process to merge to v3.x before opening a new PR.

anuragkalia · 2023-08-03T10:08:58Z

I realized that this is the correct way. v3.x are cherry-picked as needed. Reopening this and making the builds pass.

anuragkalia · 2023-08-04T07:56:49Z

@merceyz I may created some confusion by closing and re-opening this PR. After conversing on the discord channel, I realized that merging with master even for v3.x releases this is the way to go. I have fixed the tests and tested the changes.

arcanis · 2023-08-06T13:14:06Z

packages/yarnpkg-core/sources/tgzUtils.ts

@@ -39,11 +40,11 @@ export interface ExtractBufferOptions {

 let workerPool: WorkerPool<ConvertToZipPayload, PortablePath> | null;

-export async function convertToZip(tgz: Buffer, opts: ExtractBufferOptions) {
+export async function convertToZip(tgz: Buffer, limit: Limit, opts: ExtractBufferOptions) {


Can you move limit inside the opts object? And perhaps renaming it poolSize

Moved under opts and renamed it to poolSize.

arcanis · 2023-08-06T13:14:35Z

packages/yarnpkg-core/sources/tgzUtils.ts

  const tmpFolder = await xfs.mktempPromise();
  const tmpFile = ppath.join(tmpFolder, `archive.zip`);

-  workerPool ||= new WorkerPool(getZipWorkerSource());
+  workerPool ||= new WorkerPool(getZipWorkerSource(), limit);


Similar to the previous point, I'd prefer if we used an option bag rather than a free parameter.

Added an optional option bag to WorkerPool.

merceyz

workerPoolConcurrency

That seems a bit too implementation detail specific, perhaps it should be called availableParallelismOverride to match what you're overriding.
For example in yarn workspaces foreach we also use that function and that isn't done by a worker pool.
Though as I mentioned in #5635 (comment), I'd prefer if this was fixed upstream in Node.js, then everything using that function would Just Work™.

merceyz · 2023-08-06T15:00:20Z

.yarn/versions/85334687.yml

In it's current state this PR is a major change.

Indeed - although once moved as optional parameters in the option bags it'll be fine as a minor (only the modified packages + cli can be marked as minor, the rest can be declined).

Changed the version file to bump only the changed plugins as well as @yarnpkg/core and @yarnpkg/cli to minor.

arcanis · 2023-08-06T19:53:55Z

That seems a bit too implementation detail specific, perhaps it should be called availableParallelismOverride to match what you're overriding.

Not sure I agree with that - the function name itself is an implementation detail, so I don't think it's required to use the name availableParallelism in the setting name, which feels to me less clear than workerPool - which, to me, conveys fairly well that it represents the maximal amount of cpus Yarn should use (a user would have no idea if it uses the workerPool class or not behind the scene).

Perhaps another name like maxCpuCount would work, but I'm not sure it's a significant improvement either.

Though as I mentioned in #5635 (comment), I'd prefer if this was fixed upstream in Node.js, then everything using that function would Just Work™.

I don't know how easy that'd be - from a quick look there's been resistance to that, and I'm not expert enough to know if it should be reconsidered or not.

In any case, having this setting feels reasonable all in all - various other tools provide configuration settings for this kind of thing (like Jest's maxWorkers setting), and it can be a good way to allow disabling the worker pool altogether in some special cases (which has sometimes been helpful to me when investigating Node-in-the-browser things).

…/worker-pool-concurrency

anuragkalia · 2023-08-08T03:13:09Z

Ah, tests are failing! let me fix them.

anuragkalia · 2023-08-08T07:56:38Z

Ah, tests are failing! let me fix them.

Fixed! It was because of sending p-limit into the web worker. Now, filtering it out before sending.

arcanis · 2023-08-17T09:09:25Z

packages/yarnpkg-core/tests/tgzUtils.test.ts

@@ -10,7 +11,7 @@ describe(`tgzUtils`, () => {
          npath.join(__dirname, `fixtures/carbon-icons-svelte-10.21.0.tgz`),
        ),
      );
-      await expect(tgzUtils.convertToZip(data, {compressionLevel: 0})).resolves.toBeTruthy();
+      await expect(tgzUtils.convertToZip(data, {compressionLevel: 0, poolSize: pLimit(2)})).resolves.toBeTruthy();


Is there a reason to use an explicit pool size here rather than the default?

@anuragkalia

**What's the problem this PR addresses?** This PR extends @anuragkalia's work in #5636 (it starts from the same commits and builds upon them). - The original implementation was keeping a singleton worker pool - I felt this was problematic since we may enter situations (even if only in tests) where multiple calls to `convertToZip` would want to use different pools. - Converting an archive without using a worker was only possible when the concurrency was completely disabled. This was my idea, but retrospectively it felt better to me to have two settings: one for the concurrency, and another to define whether the concurrency is enabled or not. - Passing all those settings from the various fetchers would have been unwieldly. **How did you fix it?** - I created a `TaskPool` interface; `WorkerPool` implements it (using workers, same implementation as before), but so does `AsyncPool` (a new implementation that simply forwards the call to the `pLimit` limiter). - I feel like this also addresses @merceyz's comment regarding the setting name - it's now called `taskPoolConcurrency`, which doesn't directly refers to workers. - A `getConfigurationWorker` function returns the proper task pool for the given configuration object. To make that possible, WeakMap instances can now be used as storage argument for the `get*WithDefault` family of functions. **Checklist**    - [x] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing).   - [x] I have set the packages that need to be released for my changes to be effective.   - [x] I will check that all automated PR checks pass before the PR gets reviewed. --------- Co-authored-by: Anurag Kalia <[email protected]>

arcanis · 2023-10-22T09:58:47Z

Closing since it got superseded by #5764 ! Thanks @anuragkalia 🙏

akalia-atlassian added 2 commits August 3, 2023 13:52

Add workerPoolCOncurrency as a configuration option

06fb184

Add version to be released

bfc6a06

merceyz self-requested a review August 3, 2023 08:54

anuragkalia changed the title ~~Add workerPoolCOncurrency as a configuration option~~ Add workerPoolConcurrency as a configuration option Aug 3, 2023

anuragkalia closed this Aug 3, 2023

anuragkalia reopened this Aug 3, 2023

Fix: failing tests

d42494a

arcanis reviewed Aug 6, 2023

View reviewed changes

merceyz requested changes Aug 6, 2023

View reviewed changes

akalia-atlassian added 6 commits August 7, 2023 23:45

Move limit as an optional argument in options bag everywhere

8ea7928

Merge branch 'master' of https://github.com/yarnpkg/berry into akalia…

8f9f4d5

…/worker-pool-concurrency

Refactor: limit ➡️ poolSize

5c2d015

Nit: Remove stray spaces

2aa93ba

Nit: Remove stray spaces - 2

d4a326a

Bump @yarnpkg/cli as minor too

31fc984

anuragkalia requested review from arcanis and merceyz August 7, 2023 18:51

Remove p-limit from being sent to workers

1a87a9f

arcanis reviewed Aug 17, 2023

View reviewed changes

anuragkalia force-pushed the akalia/worker-pool-concurrency branch from f3f29bc to 4f50a91 Compare August 24, 2023 20:40

Add option to set workerPoolConcurrency to zero

f89fcc9

anuragkalia force-pushed the akalia/worker-pool-concurrency branch from 4f50a91 to f89fcc9 Compare August 24, 2023 21:05

arcanis mentioned this pull request Sep 28, 2023

Reworks the worker pool to be more configurable #5764

Merged

3 tasks

arcanis closed this Oct 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add workerPoolConcurrency as a configuration option #5636

Add workerPoolConcurrency as a configuration option #5636

anuragkalia commented Aug 3, 2023 •

edited

Loading

anuragkalia commented Aug 3, 2023

anuragkalia commented Aug 3, 2023

anuragkalia commented Aug 4, 2023

arcanis Aug 6, 2023

anuragkalia Aug 7, 2023

arcanis Aug 6, 2023

anuragkalia Aug 7, 2023

merceyz left a comment

merceyz Aug 6, 2023

arcanis Aug 6, 2023 •

edited

Loading

anuragkalia Aug 7, 2023 •

edited

Loading

arcanis commented Aug 6, 2023

anuragkalia commented Aug 8, 2023

anuragkalia commented Aug 8, 2023

arcanis Aug 17, 2023

arcanis commented Oct 22, 2023

Add workerPoolConcurrency as a configuration option #5636

Add workerPoolConcurrency as a configuration option #5636

Conversation

anuragkalia commented Aug 3, 2023 • edited Loading

anuragkalia commented Aug 3, 2023

anuragkalia commented Aug 3, 2023

anuragkalia commented Aug 4, 2023

arcanis Aug 6, 2023

Choose a reason for hiding this comment

anuragkalia Aug 7, 2023

Choose a reason for hiding this comment

arcanis Aug 6, 2023

Choose a reason for hiding this comment

anuragkalia Aug 7, 2023

Choose a reason for hiding this comment

merceyz left a comment

Choose a reason for hiding this comment

merceyz Aug 6, 2023

Choose a reason for hiding this comment

arcanis Aug 6, 2023 • edited Loading

Choose a reason for hiding this comment

anuragkalia Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

arcanis commented Aug 6, 2023

anuragkalia commented Aug 8, 2023

anuragkalia commented Aug 8, 2023

arcanis Aug 17, 2023

Choose a reason for hiding this comment

arcanis commented Oct 22, 2023

anuragkalia commented Aug 3, 2023 •

edited

Loading

arcanis Aug 6, 2023 •

edited

Loading

anuragkalia Aug 7, 2023 •

edited

Loading