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

Threads #39

Merged
merged 3 commits into from
Nov 20, 2020
Merged

Threads #39

merged 3 commits into from
Nov 20, 2020

Conversation

jsg2021
Copy link
Contributor

@jsg2021 jsg2021 commented Nov 8, 2020

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Push eslint onto a thread pool. For large projects, this helps speed it up.

Additional Info

This is using a feature introduced in node 12... so it will fallback and disable the thread pool if it's not available.

@codecov
Copy link

codecov bot commented Nov 8, 2020

Codecov Report

Merging #39 (2681d56) into master (43119b9) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #39   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         8    +1     
  Lines          175       200   +25     
  Branches        48        55    +7     
=========================================
+ Hits           175       200   +25     
Impacted Files Coverage Δ
src/options.js 100.00% <ø> (ø)
src/getESLint.js 100.00% <100.00%> (ø)
src/index.js 100.00% <100.00%> (ø)
src/linter.js 100.00% <100.00%> (ø)
src/worker.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43119b9...2681d56. Read the comment docs.

@jsg2021 jsg2021 changed the title WIP: Threads Threads Nov 16, 2020
@jsg2021
Copy link
Contributor Author

jsg2021 commented Nov 16, 2020

This cuts my build time nearly in half. ~169s down to ~85s. :)

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Why do not use jest-worker? All these problems have already been solved, also simple and good API

Also I think it should be implemented on eslint side, but it is trade-off

@jsg2021
Copy link
Contributor Author

jsg2021 commented Nov 18, 2020

Why do not use jest-worker? All these problems have already been solved, also simple and good API

I was trying to not add dependencies. This uses just the API provided by node 12+. I also was not aware of jest-worker. I will look at it.

Also I think it should be implemented on eslint side, but it is trade-off

We are passing one file at a time to eslint.lintFiles() as webpack processes dependencies... unless there are plans to make eslint parallelize that method, it would be good to support some parallelization here.

@alexander-akait
Copy link
Member

I was trying to not add dependencies. This uses just the API provided by node 12+. I also was not aware of jest-worker. I will look at it.

Using jest-worker is recommended, here, we use this in some plugins

We are passing one file at a time to eslint.lintFiles() as webpack processes dependencies... unless there are plans to make eslint parallelize that method, it would be good to support some parallelization here.

Yep, agree

@jsg2021
Copy link
Contributor Author

jsg2021 commented Nov 20, 2020

swapped to using jest-worker.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

/cc @ricardogobbosouza, looks good for me

@ricardogobbosouza ricardogobbosouza merged commit 1e38fc7 into webpack-contrib:master Nov 20, 2020
@@ -1,5 +1,6 @@
{
"compilerOptions": {
"noEmit": true,
Copy link
Member

Choose a reason for hiding this comment

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

/cc @ricardogobbosouza I think here mistake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, that was intentional. tsc was attempting to overwrite source files without that and producing an error in vscode. There is an inverse of that on the npm script generating the type definitions.

Copy link
Member

Choose a reason for hiding this comment

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

oh, yes, my mistake

@jsg2021 jsg2021 deleted the threads branch November 20, 2020 14:20
const max =
typeof threads !== 'number'
? threads
? os.cpus().length - 1
Copy link

Choose a reason for hiding this comment

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

What is the reasoning to use less threads than cpus?
In the old make days one would usually have done n+1 as someone might always be io blocked.

Copy link
Member

Choose a reason for hiding this comment

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

One CPU for webpack, otherwise you can block webpack thread

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants