-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Build: Limit the numbers of cores used by the build #26059
Conversation
Limit to half of the available cores. This seems to give the best performance across machines.
Prior art #25456 |
On my MacBook Pro, Core i7 (4 cores, 8 threads), no babel cache present.
|
@@ -72,8 +72,6 @@ const babelLoader = { | |||
}, | |||
}; | |||
|
|||
threadLoader.warmup( {}, [ 'babel-loader' ] ); |
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.
Why did you remove this? Or why was it here in the first place?
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.
it was an attempt to warmup the workers before they would be used. However, it has no apparent speed impact and can make the build hang, waiting for the workers to terminate.
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.
see docs on prewarming at https://github.com/webpack-contrib/thread-loader
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.
to repro the hang, add back the prewarm statement and the thread loader require, then configure the thread loader in the server build to use 4 workers. For me, npm run build-server
would run, apparently complete, but then hang waiting for something complete.
Removing the warmup killed the hang.
webpack.config.js
Outdated
{ | ||
loader: 'thread-loader', | ||
options: { | ||
workers: Math.max( Math.floor( os.cpus().length / 2 ), 1 ), |
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.
Let's bump the minimum to 2
. Even in a hypothetical single core machine, 2 threads would be better than 1 thread (plus the overhead of using thread-loader
for a single thread)
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.
Sure. I was thinking about conditionally ditching the thread-loader based on the number of cores, but defaulting to 2 is fine too.
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.
I didn't time it, but these commands still work:
npm start
CALYPSO_ENV=production npm run build
Limit to half of the available cores. This seems to give the best performance across machines.