eslint-webpack-plugin icon indicating copy to clipboard operation
eslint-webpack-plugin copied to clipboard

Potential performance improvements

Open piotr-oles opened this issue 3 years ago • 20 comments

Hi! :wave: 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 (https://github.com/TypeStrong/fork-ts-checker-webpack-plugin/issues/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 :)

piotr-oles avatar Jan 29 '22 22:01 piotr-oles

/cc @ricardogobbosouza What do you think?

alexander-akait avatar Jan 30 '22 11:01 alexander-akait

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.

jsg2021 avatar Jan 30 '22 18:01 jsg2021

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.

piotr-oles avatar Jan 30 '22 19:01 piotr-oles

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?

jsg2021 avatar Jan 30 '22 22:01 jsg2021

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?

piotr-oles avatar Jan 30 '22 22:01 piotr-oles

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

eamodio avatar Jan 30 '22 23:01 eamodio

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?

ricardogobbosouza avatar Jan 31 '22 00:01 ricardogobbosouza

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)

jsg2021 avatar Jan 31 '22 16:01 jsg2021

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!

eamodio avatar Jan 31 '22 21:01 eamodio

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.

jsg2021 avatar Jan 31 '22 23:01 jsg2021

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

Kaizer69 avatar Feb 09 '22 10:02 Kaizer69

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.

JUST-Limbo avatar Mar 28 '22 17:03 JUST-Limbo

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

eamodio avatar Mar 28 '22 17:03 eamodio

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

kirill-martynov avatar May 06 '22 14:05 kirill-martynov

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

decademoon avatar Jul 27 '22 22:07 decademoon

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 avatar Jan 28 '23 05:01 eamodio

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 avatar Nov 28 '23 04:11 eamodio

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

eamodio avatar Dec 02 '23 00:12 eamodio

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

alexander-akait avatar Dec 11 '23 15:12 alexander-akait

@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.

eamodio avatar Dec 11 '23 23:12 eamodio