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

Potential performance improvements #137

Open
piotr-oles opened this issue Jan 29, 2022 · 20 comments
Open

Potential performance improvements #137

piotr-oles opened this issue Jan 29, 2022 · 20 comments

Comments

@piotr-oles
Copy link

Hi! 👋 I'm an author of ForkTsCheckerWebpackPlugin. I recently removed support for EsLint in the plugin to focus on the TypeScript part. The eslint-webpack-plugin is a suggested replacement, but some users experience performance degradation when switching to this plugin (TypeStrong/fork-ts-checker-webpack-plugin#601) (for example from ~24s to ~40s).

Modification Proposal

Use compiler.modifiedFiles to get a list of files to lint and compiler.removedFiles to get a list of files to remove errors. This way you don't have to wait for compilation.hooks.finishModules to start the linting process.

Expected Behavior / Situation

The eslint-webpack-plugin has similar performance to fork-ts-checker-webpack-plugin@^6.0.0

Actual Behavior / Situation

The eslint-webpack-plugin is often significantly slower.

Please paste the results of npx webpack-cli info here, and mention other relevant information

I believe it's not related :)

@alexander-akait
Copy link
Member

/cc @ricardogobbosouza What do you think?

@jsg2021
Copy link
Contributor

jsg2021 commented Jan 30, 2022

What is the goal? Lint only modified files? or lint all files in the graph? We already have a config to only lint modified files.

Though, I like the new api hooks, I didn't know of them when worked on this last.

@piotr-oles
Copy link
Author

The idea is to lint all files on the initial run and then lint only changed files on next compilations.

Maybe it would be also worth to provide "files" option to the plug-in to specify files to lint (so we can start linting early also on the initial run). We could then filter-out errors coming from files outside of the dependency graph and lint files that were missing (which hopefully will be 0). We can easily have files outside of the dependency graph - for example if you have typescript file that contains types only, this file will not become part of the dependency graph.

@jsg2021
Copy link
Contributor

jsg2021 commented Jan 30, 2022

We made effort to ignore files out of the graph on purpose.

If memory serves, we toss a job at a worker pool for every file discovered. It doesn't wait for the finished callback to lint, only to report. We only re-lint modified files.

The performance difference you're seeing may be overhead of the worker pool?

@piotr-oles
Copy link
Author

I don't think so, it's ~20-second difference. @eamodio, did you experience this difference on the initial compilation, or in the next compilations?

@eamodio
Copy link

eamodio commented Jan 30, 2022

Yeah, initial compilation was about a 20s difference, but even on change it seemed slower too.

@ricardogobbosouza
Copy link
Collaborator

The idea of ​​using graph files is exactly to be faster, even more that we use workers

Could you share a repository that can prove this slowness?

@jsg2021
Copy link
Contributor

jsg2021 commented Jan 31, 2022

Machine details would help too. I could believe this plugin is a little biased to very-wide CPUs. (ie: being less ideal for low-core counts)

@eamodio
Copy link

eamodio commented Jan 31, 2022

I just pushed a branch here: https://github.com/gitkraken/vscode-gitlens/tree/feature/eslint-webpack -- that switches from using eslint from fork-ts-checker-webpack to eslint-webpack-plugin.

Using that same branch, just 1 commit back (e.g. using fork-ts-checker-webpack to run eslint), and turning off eslint caching, it would take ~23-24s to run a yarn run watch, whereas after the upgrade to eslint-webpack-plugin to takes ~39-40s.

I'm running on Windows 11 (10.0.22000 Build 22000) on a desktop PC with

  • Processor: AMD Ryzen 9 3900X 12-Core Processor, 3801 Mhz, 12 Core(s), 24 Logical Processor(s)
  • 32GB DDR4
  • 1TB Sabrent Rocket 4.0 NVMe SSD - Gen4 PCIe x4

I've also tried to enable threading -- using threads: true, threads: 1, or threads: 2 and in all cases webpack never returns.

Hope that helps!

@jsg2021
Copy link
Contributor

jsg2021 commented Jan 31, 2022

threads: number will limit the number of threads (0 obviously disabling). threads: boolean will enable/disable. The default thread limit is cpus - 1. If it's not returning, that sounds like a bug. Without threads enabled it will be much slower.

@Kaizer69
Copy link

Kaizer69 commented Feb 9, 2022

I confirm @eamodio problem.
Using threads : true webpack never returns.

@JUST-Limbo
Copy link

JUST-Limbo commented Mar 28, 2022

I just pushed a branch here: https://github.com/gitkraken/vscode-gitlens/tree/feature/eslint-webpack -- that switches from using eslint from fork-ts-checker-webpack to eslint-webpack-plugin.

Using that same branch, just 1 commit back (e.g. using fork-ts-checker-webpack to run eslint), and turning off eslint caching, it would take ~23-24s to run a yarn run watch, whereas after the upgrade to eslint-webpack-plugin to takes ~39-40s.

I'm running on Windows 11 (10.0.22000 Build 22000) on a desktop PC with

  • Processor: AMD Ryzen 9 3900X 12-Core Processor, 3801 Mhz, 12 Core(s), 24 Logical Processor(s)
  • 32GB DDR4
  • 1TB Sabrent Rocket 4.0 NVMe SSD - Gen4 PCIe x4

I've also tried to enable threading -- using threads: true, threads: 1, or threads: 2 and in all cases webpack never returns.

Hope that helps!

Did you mean when you are using threads: true , there are no eslint errors/warnings webpack returns in the vscode terminal after build complete?

I recently ran into the above problem: when using threads: true ,there are no eslint errors/warnings in the vscode terminal after build complete.
But,when it set to false,the expected errors/warnings come back.

@eamodio
Copy link

eamodio commented Mar 28, 2022

Correct, when using threads: true it never returns at all for me.

@kirill-martynov
Copy link

Any progress on this issue?

With threads: true build time super small, but it's not showing errors and warnings
With threads: false build time huge, shows errors and warns

@decademoon
Copy link

I'm not sure if this is related, but for me, sometimes when threads is enabled some workers never complete and the webpack build stalls. I'm not sure if this is an issue with eslint-webpack-plugin or jest-worker.

macOS 12.4 M1 Max

@eamodio
Copy link

eamodio commented Jan 28, 2023

I just tried this again, since fork-ts-checker-webpack v6.x doesn't like TypeScript 5.0-beta. And still seeing more the double the times for using eslint-webpack-plugin vs fork-ts-checker-webpack with eslint enabled. 60s vs 28s for a startup watch build.

@eamodio
Copy link

eamodio commented Nov 28, 2023

Revisited this again and it seems that things have gotten even worse. Without using caching, fork-ts-checker-webpack takes ~22s to do an initial pass while watching and rebuilds are very fast usually 150ms-1s, while eslint-webpack-plugin takes ~42s, and rebuilds take ~12s.

Setting lintDirtyModulesOnly: true significantly improved the initial pass -- though I don't think that is what I want, since I'm guessing it won't report issues on the initial pass. But it made rebuild WAY worse, ~34s

Is there any planned progress here? Or how best can the community help?

@eamodio
Copy link

eamodio commented Dec 2, 2023

FWIW, I created a new webpack plugin for ESLint: https://github.com/eamodio/eslint-lite-webpack-plugin/
It's pretty basic and fits my needs (and maybe yours) and is as fast (or faster) than fork-ts-checker-webpack

@alexander-akait
Copy link
Member

@eamodio we can emrge your soluiton here, do you want to send a PR (if you need diccusion - welcome ⭐ )

@eamodio
Copy link

eamodio commented Dec 11, 2023

@alexander-akait While I would be all for getting the improvements back in here, I don't personally have the bandwidth as I believe it would be fairly tricky. I took a lot of assumptions, required versions, limited config, no fix support, logging that matches (or improves on) the output of fork-ts-checker-webpack to make the VS Code problem matchers and terminal links work, etc, etc that would be hard to adapt back.

I would happily support anyone attempting to bring it back in though -- and FWIW the new plugin is quite simple.

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

No branches or pull requests

9 participants