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

Perfomance degradation in import plugin after eslint 6 #13722

Closed
jehy opened this issue Sep 30, 2020 · 15 comments
Closed

Perfomance degradation in import plugin after eslint 6 #13722

jehy opened this issue Sep 30, 2020 · 15 comments
Labels
3rd party plugin This is an issue related to a 3rd party plugin, config, or parser archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion

Comments

@jehy
Copy link

jehy commented Sep 30, 2020

Tell us about your environment

  • ESLint Version:
    tested on latest versions of 5,6,7
  • Node Version:
    12.18.3
  • npm Version:
    6.14.6
    What parser (default, @babel/eslint-parser, @typescript-eslint/parser, etc.) are you using?
    default

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

Updated eslint from version 5 to 6 and 7.

What did you expect to happen?

Expected no performance degradation, before update timing looked like this:

image

What actually happened? Please include the actual, raw output from ESLint.
Actually after update to 6 or 7 version I got insane degradation in eslint import plugin.
image
Nothing else besides eslint version was changed. Also tried installing latest version of import plugin but go the same picture.
Seems like there is some important change in Eslint 6.0. I looked through release notes but couldn't find the problem yet.

Are you willing to submit a pull request to fix this bug?
No, still not sure what in Elint caused this problem. Help wanted.

@jehy jehy added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Sep 30, 2020
@anikethsaha
Copy link
Member

anikethsaha commented Sep 30, 2020

Thanks for the issue.

We don't maintain import plugin. I guess you can create an issue in import plugin repo.

The core rules do seem fine as there is not much increase in the time. the increase in time is because of the new features (and options) added to it (I am referring to indent).

@anikethsaha anikethsaha added 3rd party plugin This is an issue related to a 3rd party plugin, config, or parser and removed bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Sep 30, 2020
@jehy
Copy link
Author

jehy commented Sep 30, 2020

Yup, I understand that it's a third party plugin. But ESlint somehow degraded it's performance on update. And I suppose that ESLint developers will be able to find the reason better then plugin maintainer. After all, I suppose that not only this plugin suffered from degradation.

By the way, I narrowed the problem. Everything was still fine with eslint@6.0.0-alpha.0 but degraded in eslint@6.0.0-alpha.1.

@jehy
Copy link
Author

jehy commented Sep 30, 2020

Narrowed the problem to commit 6ae21a4
I suppose something really went wrong with fixing config loading.

@jehy
Copy link
Author

jehy commented Sep 30, 2020

@mysticatea could you take a look? Seems like that's your PR #11546

@anikethsaha
Copy link
Member

Whats the output with update plugins and also with eslint 7 ?

@jehy
Copy link
Author

jehy commented Sep 30, 2020

Whats the output with update plugins and also with eslint 7 ?

Absolutely same :(
It's exactly 6ae21a4 that changes all the picture.

@nzakas
Copy link
Member

nzakas commented Oct 1, 2020

There’s likely not an issue with the import plugin at all. Note that the rule order is different before and after upgrading. It’s not the rules or the plugin that is the problem, it’s all the bootstrapping ESLint is doing before even running the first rule.

Config is pretty complicated, which is why we are replacing it with a simpler system. Unfortunately, I don’t think we can boost the performance of configs right now without jeopardizing current work. I’m pushing forward on #13481 as fast as I can.

@nzakas nzakas added core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Oct 1, 2020
@jehy
Copy link
Author

jehy commented Oct 1, 2020

@nzakas Yeah, you are right! I just tried disabling slow rules and found out that total time stayed same, and other rules became "slow". Really seems like a bug with timings which counts in ESLint bootstrapping. May be we need one more separate timing for bootstrapping?

Good luck with #13481, seems like we'll have to wait for it before upgrading from ESLint 5.

@mdjermanovic
Copy link
Member

I think that timing already measures only time spent in listeners:

eslint/lib/linter/linter.js

Lines 934 to 941 in c1974b3

Object.keys(ruleListeners).forEach(selector => {
emitter.on(
selector,
timing.enabled
? timing.time(ruleId, ruleListeners[selector])
: ruleListeners[selector]
);
});

eslint-plugin-import is specific because a rule that happens to be the first builds and caches a module dependency graph for itself and other rules.

@jehy when you disable slow rules, the other rules that became slow are also eslint-plugin-import rules?

@jehy
Copy link
Author

jehy commented Oct 1, 2020

Some more samples @mdjermanovic

Three import rules:

import/no-unresolved              | 446864.448 |    61.4%
import/no-named-as-default-member | 233939.116 |    32.1%
indent                            |   9687.283 |     1.3%
react/jsx-no-bind                 |   2519.813 |     0.3%
react/no-deprecated               |   2486.111 |     0.3%
react/no-direct-mutation-state    |   2041.882 |     0.3%
react/display-name                |   1853.400 |     0.3%
react/no-string-refs              |   1379.716 |     0.2%
import/no-duplicates              |   1294.837 |     0.2%
react/require-render-return       |   1279.207 |     0.2%

Two import rules (import/no-duplicates timings became exactly as import/no-unresolved after disabling import/no-unresolved):

import/no-duplicates              | 471886.098 |    60.6%
import/no-named-as-default-member | 255601.133 |    32.8%
indent                            |  10698.368 |     1.4%
react/jsx-no-bind                 |   2824.494 |     0.4%
react/no-deprecated               |   2722.666 |     0.3%
react/no-direct-mutation-state    |   2306.663 |     0.3%
react/display-name                |   2042.529 |     0.3%
react/no-string-refs              |   1549.638 |     0.2%
react/require-render-return       |   1398.805 |     0.2%
no-redeclare                      |   1152.237 |     0.1%

One import rule (it took all time, previously divided between other rules):

import/no-named-as-default-member | 481570.943 |    91.5%
indent                            |   9974.863 |     1.9%
react/jsx-no-bind                 |   2596.360 |     0.5%
react/no-deprecated               |   2484.272 |     0.5%
react/no-direct-mutation-state    |   2044.686 |     0.4%
react/display-name                |   1829.184 |     0.3%
react/no-string-refs              |   1412.198 |     0.3%
react/require-render-return       |   1332.183 |     0.3%
no-redeclare                      |   1147.963 |     0.2%
lodash/preferred-alias            |    856.365 |     0.2%

No import rules:

indent                         |  8457.224 |    21.9%
react/jsx-no-bind              |  2227.679 |     5.8%
react/no-deprecated            |  2132.933 |     5.5%
react/no-direct-mutation-state |  1754.359 |     4.5%
react/display-name             |  1578.654 |     4.1%
react/no-string-refs           |  1198.497 |     3.1%
react/require-render-return    |  1101.455 |     2.9%
no-redeclare                   |  1022.821 |     2.7%
lodash/preferred-alias         |   742.384 |     1.9%
lodash/no-unbound-this         |   581.135 |     1.5%

Seems like it's exactly module dependency graph is what became slow, just as you mentioned.

@jehy
Copy link
Author

jehy commented Oct 2, 2020

Tried adding issue to eslint-plugin-import, but it's developer said there is nothing he can do for improvement :(

@ljharb
Copy link
Sponsor Contributor

ljharb commented Oct 2, 2020

More to the point, eslint-plugin-import hasn't changed how it builds up its ExportMap in many many years, so if it suddenly got slower in eslint 6, I'm not sure what we can do to speed it up.

@jehy
Copy link
Author

jehy commented Oct 2, 2020

I can also add ESLint debug log. It's 13 MB, not sure if it can help.

@nzakas
Copy link
Member

nzakas commented Oct 6, 2020

FYI there is an issue open for separation of bootstrap costs from rule performance:
#13695

@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Nov 6, 2020
@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators May 6, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label May 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3rd party plugin This is an issue related to a 3rd party plugin, config, or parser archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion
Projects
None yet
Development

No branches or pull requests

5 participants