Skip to content

Performance issue #187

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

Closed
lfrodrigues opened this issue Feb 22, 2016 · 7 comments
Closed

Performance issue #187

lfrodrigues opened this issue Feb 22, 2016 · 7 comments

Comments

@lfrodrigues
Copy link

In one of my projects (https://github.com/Seedstars/django-react-redux-jwt-base) eslint validation takes 3/4s to run.

Adding eslint-plugin-import triples the time to 12s.

Is there a way to improve run time?

@benmosher
Copy link
Member

So far, other than #149, I don't have many ideas, and even that will likely only improve performance by about 50%. So I'd predict you'd still be looking at 8s. I have some other ideas about caching a map of imports/exports on the FS, but that would be a pretty major exercise as well, and it's not obvious (without just doing it) how much it would help.

This plugin is so slow--relative to others, at least--because it must parse each imported file into an AST and collect metadata.

Due to the way ESLint is built (with good reason, see discussion), the file I/O must happen synchronously. My impression is that this is where most of the time is lost.

That said, I can't be sure, so it's risky (in a cost-benefit sense) to attempt any high-effort mission to improve it.

@benmosher benmosher mentioned this issue Mar 2, 2016
@benmosher
Copy link
Member

FWIW: eslint_d with the --cache option takes me from ~32s to ~10s.

Just note that there is a known potential gotcha with keeping the export cache in memory for a long time (#200) so it's not perfect, yet. Will try to get this fixed soon.

@lencioni
Copy link
Contributor

Using the timing env variable (e.g. TIMING=1 eslint .), this is what I'm seeing in a project I am using this plugin in, with only the no-unresolved rule enabled:

Rule                          | Time (ms) | Relative
:-----------------------------|----------:|--------:
import/no-unresolved          | 18563.065 |    40.6%
react/sort-comp               |  1621.646 |     3.5%
no-irregular-whitespace       |  1386.958 |     3.0%
react/prop-types              |  1197.552 |     2.6%
no-implied-eval               |   829.967 |     1.8%
react/jsx-uses-vars           |   765.702 |     1.7%
keyword-spacing               |   765.665 |     1.7%
no-eval                       |   578.730 |     1.3%
semi-spacing                  |   549.684 |     1.2%
no-whitespace-before-property |   477.285 |     1.0%

I think for the no-unresolved rule, adding a cache would likely help a good amount.

@lencioni
Copy link
Contributor

Glancing at the code, I think that even memoizing some stuff in the resolve() path would likely help on full runs like this.

lencioni added a commit to lencioni/eslint-plugin-import that referenced this issue Mar 17, 2016
In a project where I have import/no-unresolved enabled, I get the
following results with ESLint's TIMING=1 environment variable enabled:

```
Rule                          | Time (ms) | Relative
:-----------------------------|----------:|--------:
import/no-unresolved          | 18563.065 |    40.6%
react/sort-comp               |  1621.646 |     3.5%
no-irregular-whitespace       |  1386.958 |     3.0%
react/prop-types              |  1197.552 |     2.6%
no-implied-eval               |   829.967 |     1.8%
react/jsx-uses-vars           |   765.702 |     1.7%
keyword-spacing               |   765.665 |     1.7%
no-eval                       |   578.730 |     1.3%
semi-spacing                  |   549.684 |     1.2%
no-whitespace-before-property |   477.285 |     1.0%
```

To speed up this outlier, I decided to cache the results of file
existence checks in an object. Adding this cache brings this time down
from ~18s to ~13s:

```
Rule                    | Time (ms) | Relative
:-----------------------|----------:|--------:
import/no-unresolved    | 13199.978 |    32.5%
react/sort-comp         |  1793.781 |     4.4%
react/prop-types        |  1400.975 |     3.4%
no-irregular-whitespace |  1315.485 |     3.2%
react/jsx-uses-vars     |   794.097 |     2.0%
no-implied-eval         |   765.593 |     1.9%
keyword-spacing         |   752.945 |     1.9%
no-eval                 |   592.048 |     1.5%
semi-spacing            |   532.061 |     1.3%
no-control-regex        |   479.856 |     1.2%
```

Addresses import-js#187
lencioni added a commit to lencioni/eslint-plugin-import that referenced this issue Mar 17, 2016
In a project where I have import/no-unresolved enabled, I get the
following results with ESLint's TIMING=1 environment variable enabled:

```
Rule                          | Time (ms) | Relative
:-----------------------------|----------:|--------:
import/no-unresolved          | 18563.065 |    40.6%
react/sort-comp               |  1621.646 |     3.5%
no-irregular-whitespace       |  1386.958 |     3.0%
react/prop-types              |  1197.552 |     2.6%
no-implied-eval               |   829.967 |     1.8%
react/jsx-uses-vars           |   765.702 |     1.7%
keyword-spacing               |   765.665 |     1.7%
no-eval                       |   578.730 |     1.3%
semi-spacing                  |   549.684 |     1.2%
no-whitespace-before-property |   477.285 |     1.0%
```

To speed up this outlier, I decided to cache the results of file
existence checks in a Map. Adding this cache brings this time down
from ~18s to ~13s:

```
Rule                    | Time (ms) | Relative
:-----------------------|----------:|--------:
import/no-unresolved    | 13199.978 |    32.5%
react/sort-comp         |  1793.781 |     4.4%
react/prop-types        |  1400.975 |     3.4%
no-irregular-whitespace |  1315.485 |     3.2%
react/jsx-uses-vars     |   794.097 |     2.0%
no-implied-eval         |   765.593 |     1.9%
keyword-spacing         |   752.945 |     1.9%
no-eval                 |   592.048 |     1.5%
semi-spacing            |   532.061 |     1.3%
no-control-regex        |   479.856 |     1.2%
```

Addresses import-js#187
@benmosher
Copy link
Member

Just published @lencioni's cache contribution to npm as v1.2.0. My total lint time is down from 37s to 25s. Pretty cool!

Notably, my lint time without the import rules is ~12s, so I'm thinking the extra 12s is basically from re-parsing every file for exports. When I've profiled, the parsing is the most expensive bit.

Not sure where to go from here, could try some sort of content-addressed memoizing wrapper parser. Ideally, every module would be parsed only once.

@benmosher
Copy link
Member

Just pushed a bunch of perf improvements to next tag as v1.3.0.

I got down to 17s (just above the time without the plugin at all, something like 14-15s) by removing a bunch of unnecessary node_modules parsing, and adding a memoizing parser.

I'm curious to hear how much improvement you see, @lfrodrigues -- let me know if/when you try it? 😄

@lfrodrigues
Copy link
Author

@benmosher thanks for the update, I just tested v1.3.0 and I'm happy to report that run time went down to +-4.3s with import/errors and import/warnings enabled.

On my use case eslint-plugin-import is taking around 0.5s which is a lot better than before!

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

No branches or pull requests

3 participants