paleite / eslint-plugin-diff

Run ESLint on your changes only
https://npm.im/eslint-plugin-diff
MIT License
173 stars 13 forks source link

Performance problems with the diff plugin #2

Closed DaVarga closed 3 years ago

DaVarga commented 3 years ago

Hello I came across this plugin today. Looks very promising but I have some problems once i integrate this plugin into my eslintrc. The linting process takes about 10-20 times longer with the plugin. I have configured everything according to the readme. Even if I disable almost all rules it takes forever to complete. Without the plugin a linting process for the whole project takes about 20 seconds. Even if I only want to link one file with the plugin it takes over a minute.

I am using Windows 10 with nodejs v12.16.1. Let me know if I can provide more information.

amc1804 commented 3 years ago

I haven't tried running it, just skimmed the code. When eslint reports many messages for one file, is the plugin re-running git diff for each message? It should only need to run git diff and extract the line ranges once per file. (Or it could run git diff just once for all the files with messages, and parse the pathnames along with the line numbers.)

DaVarga commented 3 years ago

You are right, getDiffForFile is called once for each lint message. getDiffForFile itself spawns a child process. https://github.com/paleite/eslint-plugin-diff/blob/2b66df4d8dc8e28cde801ba075fc9150369964fd/src/processors.ts#L17

Second thing to optimize would be to lint only files that have any changes. Currently all files are linted and the corresponding messages are removed.

Here are some messurements of linting one legacy file with 1383 problems (1370 errors, 13 warnings):

With plugin

PS C:\dev\project> Measure-Command {eslint src/test.js}
error Command failed with exit code 1.

Days              : 0
Hours             : 0
Minutes           : 1
Seconds           : 51
Milliseconds      : 697
Ticks             : 1116972388
TotalDays         : 0,00129279211574074
TotalHours        : 0,0310270107777778
TotalMinutes      : 1,86162064666667
TotalSeconds      : 111,6972388
TotalMilliseconds : 111697,2388

Without plugin

PS C:\dev\project> Measure-Command {eslint src/test.js}
error Command failed with exit code 1.

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 2
Milliseconds      : 83
Ticks             : 20839568
TotalDays         : 2,41198703703704E-05
TotalHours        : 0,000578876888888889
TotalMinutes      : 0,0347326133333333
TotalSeconds      : 2,0839568
TotalMilliseconds : 2083,9568

@paleite If you give me a hint on how to test a custombuild of the plugin locally I can create a pullrequest with appropriate optimizations.

paleite commented 3 years ago

Wow!

This has been on my todo-list since I first released this plugin, but I hadn't realized that it would make such a big difference in other OSes. I also didn't think of preprocessing the list and filtering out any files without diffs, so that was another great addition ๐Ÿ†

I really appreciate you taking the time to report this and am even more grateful for your PR. I've looked through it and it looks very good. Great to see the updates to the tests as well!

I will merge and release a new version during the next day, so I hope you (and everyone else using this plugin) can benefit of the performance optimizations very soon.

๐Ÿ‡ฉ๐Ÿ‡ช Bin dir sehr dankbar dass du dir so viel Mรผhe gegeben hast!

paleite commented 3 years ago

Released now: https://github.com/paleite/eslint-plugin-diff/releases/tag/v1.0.4

ping @DaVarga

DaVarga commented 3 years ago

Tanks mate ๐Ÿ‘