Skip to content
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

Allow thread-loader configuration #795

Merged
merged 1 commit into from
Apr 29, 2021

Conversation

bendemboski
Copy link
Contributor

Currently thread-loader is not configurable, meaning that in some CI environments where the OS reports a very large number of CPUs, thread-loader will create a very large worker pool.

The only configuration that is currently available is setting JOBS=1, but the pool warmup call doesn't check JOBS, so always spawns the full number of workers.

So this commit:

  • Skips warming the worker pool if JOBS=1 and we know we won't be using thread-loader
  • Adds a thread-loader configuration object to the options interface that allows applications to pass their own thread-loader configuration, or false to disable the use of thread-loader entirely.

Currently `thread-loader` is not configurable, meaning that in some CI environments where the OS reports a very large number of CPUs, `thread-loader` will create a very large worker pool.

The only configuration that is currently available is setting `JOBS=1`, but the pool warmup call doesn't check `JOBS`, so always spawns the full number of workers.

So this commit:

* Skips warming the worker pool if `JOBS=1` and we know we won't be using `thread-loader`
* Adds a `thread-loader` configuration object to the options interface that allows applications to pass their own `thread-loader` configuration, or `false` to disable the use of `thread-loader` entirely.
@bendemboski
Copy link
Contributor Author

The context around this (for me) is that my embroider builds are failing in CI because the containers have 4GB of RAM, and that gets used up quickly spawning 35 or however many workers (which is unavoidable currently because of the worker pool warming). Just the change for JOBS=1 to disable the warmup would fix the issue, but it'd also be great to be able to keep concurrency, just limit it to a reasonable number of workers.

But if there are any concerns about the configuration/options change, I'd be happy to split the change where JOBS=1 disabled the worker pool warmup into a separate PR as that would unblock my CI builds.

@@ -76,6 +66,7 @@ const Webpack: Packager<Options> = class Webpack implements PackagerInstance {
private extraConfig: Configuration | undefined;
private passthroughCache: Map<string, Stats> = new Map();
private publicAssetURL: string | undefined;
private extraThreadLoaderOptions: object | false | undefined;
Copy link

@avihai-yosef avihai-yosef Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe replace object with ThreadLoaderOptions Type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find types for thread-loader anywhere, and wasn't sure we'd want to sign up for maintaining our own type. We certainly could though...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I couldn't find either, nvm..

@ef4
Copy link
Contributor

ef4 commented Apr 29, 2021

Thanks for digging into this, this looks great.

@ef4 ef4 merged commit 30a1f06 into embroider-build:master Apr 29, 2021
@rwjblue rwjblue changed the title Allow thread-loader configuration Allow thread-loader configuration May 4, 2021
@rwjblue rwjblue added the enhancement New feature or request label May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants