-
-
Notifications
You must be signed in to change notification settings - Fork 48
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 massively slows down linting / re-linting #52
Comments
@ricardogobbosouza I think we should set |
This makes sense on large projects. |
I agree, should be an option for people that really need it. For now, I also don't see a lot of improvements in our build time - it's 5 seconds faster with threads than without threads enabled - 64.66s vs 69.05s (and we have 297 files and 7440 3rd party files), but having 10s+ delay of each file change is too much. I've also upgraded from node v14 to node v15 to see if there's a difference, but there wasn't one. |
Just out of curiosity, what is your cpu? I imaging form hardware to hardware this will vary. The report should be only issued once per compile and will take the same amount of time threaded or not. The linting ... reading/parsing/validating happens in workers. I'm quite surprised the recompile time you're seeing. I haven't noticed that locally. Is your cpu under heavy load? |
I sure hope you aren't linting third party code. A 10s delay sounds like a problem and is not expected behavior. I maintain a very large project that has thousands of files (ignoring third party files) and editing and recompiling is nearly imperceptible. So I'm curious what could be different in your config? |
Your time measurement here is measuring time between invocations... which is not really measuring the speed of this block. Webpack is calling this repeatedly on all the files in the graph. It only makes it into that block when we see a file we want to lint. And then, we kick off an asynchronous call to have it linted, but return synchronously to not block the compiler. The report starts at then end of compiling and waits for the lint queue to finish. (this will take some time depending on how many files are still being worked on... similar to how long running eslint command line on your project) |
it may be a bug. it shouldn't ever be slower. It turns itself off if there are 2 or less cores reported. on 4+ core systems it should be a net speed up. |
I'm definitely not linting 3rd party code (especially since I don't have this problem with On just a file I've added some timings above, it takes 3ms to lint but then it takes awaitReport around 11-16s to complete. I don't have a slow machine. It's a Dell XPS 9560 - i7-7700HQ, 32gb RAM, and an NVME SSD (on Windows). And the same problem happens to my colleague as well, who runs a Dell Area 51 laptop with (i9-10980HK I think) on PopOS Since this also happens to a lot of people that run This guy says it's the same issue on blank projects as well - facebook/create-react-app#10145 (comment) I will give it a try and see if I have the same issue on a newly created React project and come back. |
@eek I will try to reproduce as well in the morning. |
Great. I've also tested it now and what @imsandez reported here - facebook/create-react-app#10145 (comment) is true. if you create a new react app:
(I've created a TS one to be closer to what I use) I takes between 3-6s to compile a change in App.tsx (closer to 3s rather than 6s) but once you downgrate the
and Compile will take maximum 0.5s. With my custom eslintConfig:
on the blank project it always takes 6s. Once I downgrade to the previous |
I agree that it should be deactivated by default, in most projects that are usually small, it does not make sense to have it activated. We forgot to put this option in the documentation 😟 WTDY @alexander-akait ? |
@ricardogobbosouza, I also understand what @jsg2021 says as well. If it wouldn't be for this weird bug that's affecting recompile time, it would be a performance improvement for all projects. |
@jsg2021 |
Using the defaults provided by The jest-worker doesn't auto-warm up the pool. So until you hit max-pool size each lint request is paying the eslint init cost. After each compile, we cleanup the pool. So, each recompile rebuilds the pool... and since incremental edits often are one file... that takes a pool init + module init + ESLint setup AND run... So, my first stab at this is to disable the pool after the first build. All rebuilds would be on the main process and not respin the pool. I can go further to preserve the pool (and warmup) such that re-compiles don't start over. Dispatching seems to eat ~150-200ms. So recompiles will always be slower by that much minimum. Would some lint configs warrant always run off thread? or would just running lint on the main thread after the first compile be sufficient? @ricardogobbosouza @eek @alexander-akait |
@jsg2021 just running the lint on the main thread after the first compile I think it's sufficient. Updated now to 2.4.1 and things are no longer slow 🙌 thanks! 😁 |
After pr #55, we can feel free to set |
I think best value is |
The main problem is initial workers start and we should not ending |
we didn't take it away. it's just not default. you may turn it on in your config. that's what i have done for my project. |
This is a problem that was experienced in
react-scripts
/create-react-app
v4.0.1
andv4.0.2
Compiling and Re-compiling changes seems extremely slow compared to pre v4 version. After digging a bit, it seems the culprit is
threads: true
pointed out by @radoi90 here - #50 (comment)Expected Behavior
Fast compiling
Actual Behavior
Actual very slow compiling / recompiling.
I dug a bit deeper to see if there's a small issue with the threads and it might be 2 issues, don't know if it's actually the same.
On the first compile:
I've added before this line:
eslint-webpack-plugin/src/index.js
Line 91 in 46fc07a
and my results look like this:
(this is a small excerpt of 300+ files that need to be linted at first run)
I've also measured:
eslint-webpack-plugin/src/index.js
Line 101 in 46fc07a
which took
awaitReport: 29.909ms
and
eslint-webpack-plugin/src/index.js
Line 114 in 46fc07a
generateReportAsset: 0.177ms
then I've edited just a single file to see what happens there and it looks something like this:
Code
https://github.com/facebook/create-react-app/blob/9b08e3c9b365ac790546a3d5027d24f264b42613/packages/react-scripts/config/webpack.config.js#L753-L771
setting
threads: false
as here facebook/create-react-app#10154 (comment)solves the slow lint / relint
// additional code, HEY YO remove this block if you don't need it
How Do We Reproduce?
I guess try it on a newly created React project with
create-react-app
- facebook/create-react-app#10145 (comment)The text was updated successfully, but these errors were encountered: