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

fix: limit number of workers when creating haste maps in projects #9259

Merged
merged 4 commits into from Dec 3, 2019

Conversation

thymikee
Copy link
Collaborator

@thymikee thymikee commented Dec 3, 2019

Summary

Partially addresses #9236.

When creating haste contexts and maps, Jest currently spawns maxWorkers per "project". While it doesn't make a difference in a single-project workspace, it's a big deal for those using multiple projects.

A quick-and-dirty-but-works solution is to limit the number of workers based on the projects number. It's sub-optimal, because we may have smaller and larger projects which we assign the same number of workers. However a proper solution would require more work. E.g. we could delegate createHasteMap of each project to a worker pool created in jest-core, instead of deferring parallelization to createHasteMap itself. Feel free to implement this solution. However, I'm not pursuing this, as @scotthovestadt has likely far more meaningful changes to haste map performance.

In Jest own test suite, this diff result in haste map creation time drop from 3.1s to 0.8s on my machine, which is noticeable.

@thymikee thymikee changed the title feat: limit number of workers when creating haste maps in projects fix: limit number of workers when creating haste maps in projects Dec 3, 2019
@@ -5,9 +5,7 @@
"**/node_modules": true,
"**/build": true
},
"editor.formatOnSave": true,
Copy link
Member

Choose a reason for hiding this comment

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

sure? 😀

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Format on save may conflict with eslint fix, if editor settings for prettier differ

Copy link
Member

@SimenB SimenB Dec 4, 2019

Choose a reason for hiding this comment

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

seems silly to me to have editor specific config files checked in, but I'm not losing sleep over it 🤷‍♂

@@ -122,7 +122,10 @@ const buildContextsAndHasteMaps = async (
createDirectory(config.cacheDirectory);
const hasteMapInstance = Runtime.createHasteMap(config, {
console: new CustomConsole(outputStream, outputStream),
maxWorkers: globalConfig.maxWorkers,
maxWorkers: Math.max(
Copy link
Member

Choose a reason for hiding this comment

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

there's not meaningful test we can write, right?

@SimenB SimenB merged commit 9ac2dcd into jestjs:master Dec 3, 2019
@SimenB
Copy link
Member

SimenB commented Dec 3, 2019

@scotthovestadt FYI whenever you get back. Not sure how this fits into your refactor

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants