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

import/namespace is very slow #2340

Open
aaronadamsCA opened this issue Dec 29, 2021 · 25 comments
Open

import/namespace is very slow #2340

aaronadamsCA opened this issue Dec 29, 2021 · 25 comments

Comments

@aaronadamsCA
Copy link
Contributor

aaronadamsCA commented Dec 29, 2021

In our medium-sized mixed JS/TS codebase, specifically disabling import/namespace significantly reduced our lint times. I haven't found this mentioned by anyone else, so I wanted to share the finding, because it was very surprising.

Our prior configuration ran like this:

Rule                              | Time (ms) | Relative
:---------------------------------|----------:|--------:
import/namespace                  |  5762.140 |    64.7%
import/no-relative-packages       |   673.212 |     7.6%
import/named                      |   471.116 |     5.3%
import/no-extraneous-dependencies |   317.690 |     3.6%
import/extensions                 |   254.717 |     2.9%
Done in 14.53s.

I always assumed this was just the "first rule tax" while the export map is created, but adding "import/namespace": "off" did this for us:

Rule                              | Time (ms) | Relative
:---------------------------------|----------:|--------:
import/named                      |  1459.028 |    33.6%
import/no-relative-packages       |   577.911 |    13.3%
import/no-duplicates              |   408.152 |     9.4%
import/extensions                 |   401.591 |     9.2%
import/no-extraneous-dependencies |   276.880 |     6.4%
Done in 9.44s.

Given we already use "import/no-namespace": "error", this rule wasn't doing anything for us anyway. With one line we've managed to significantly improve IDE responsiveness.

@ljharb
Copy link
Member

ljharb commented Dec 29, 2021

Indeed, it's not necessarily that that rule is slow - it's that whichever rule is the first one to build up an ExportsMap of your entire codebase will be slow. Not every rule uses it.

However, named does use the ExportsMap, so something strange must be going on with the namespace rule. I'd be happy to review a PR that improved its performance.

@the-ult
Copy link

the-ult commented Jan 18, 2022

Same in our project.
Seems to be really slow:

Rule                               | Time (ms) | Relative
:----------------------------------|----------:|--------:
import/namespace                   | 21149.982 |    62.4%
import/default                     |  2871.053 |     8.5%
import/no-named-as-default         |  2305.758 |     6.8%
import/no-named-as-default-member  |  1858.213 |     5.5%
rxjs/finnish                       |  1617.432 |     4.8%
import/order                       |   777.413 |     2.3%
import/no-unresolved               |   519.802 |     1.5%
sonarjs/no-ignored-return          |   391.950 |     1.2%
import/export                      |   387.884 |     1.1%
@nrwl/nx/enforce-module-boundaries |   324.435 |     1.0%
Rule | Time (ms) | Relative
:----|----------:|--------:

Rules:

"overrides": [
    {
      "files": ["*.ts", "*.tsx", "*.js", "*.jsx"],
      "plugins": [
             "unused-imports", 
             "rxjs-angular"
       ],
      "settings": {
        "import/parsers": {
          "@typescript-eslint/parser": [".ts", ".tsx"]
        },
        "import/resolver": {
          "typescript": {
            "alwaysTryTypes": true,
            "project": "./tsconfig.*?.json"
          }
        }
      },
      "extends": [
        "plugin:@nrwl/nx/angular",
        "plugin:unicorn/recommended",
        "plugin:sonarjs/recommended",
        "plugin:rxjs/recommended",
        "plugin:import/recommended",
        "plugin:import/typescript",
        "plugin:storybook/recommended",
        "plugin:@nrwl/nx/typescript"
      ],
      "parserOptions": {
        "project": ["tsconfig.*?.json"]
      },
      "rules": {
 
        "import/newline-after-import": "error",
        "import/no-unused-modules": "error",
        "import/order": [
          "error",
          {
            "alphabetize": {
              "order": "asc",
              "caseInsensitive": true
            },
            "newlines-between": "always",
            "pathGroupsExcludedImportTypes": ["builtin"]
          }
        ],
}

@aaronadamsCA
Copy link
Contributor Author

@the-ult Make sure to compare total time with vs. without the rule, so you can rule out the building of the export map as the cause. It would be good to have another solid data point here!

@epmatsw
Copy link

epmatsw commented Feb 22, 2022

Another data point here:

without namespace:

Rule                           | Time (ms) | Relative
:------------------------------|----------:|--------:
prettier/prettier              | 38484.339 |    43.1%
import/named                   | 13464.919 |    15.1%
react/no-deprecated            |  4004.563 |     4.5%
react/no-direct-mutation-state |  3019.211 |     3.4%
import/no-restricted-paths     |  2797.501 |     3.1%
import/extensions              |  2255.116 |     2.5%
react/no-string-refs           |  1929.564 |     2.2%
react/require-render-return    |  1392.313 |     1.6%
react/display-name             |  1247.210 |     1.4%
import/no-duplicates           |   888.921 |     1.0%

with namespace:

import/namespace                    | 68662.137 |    47.8%
prettier/prettier                   | 37565.273 |    26.2%
react/no-deprecated                 |  3884.333 |     2.7%
react/no-direct-mutation-state      |  2940.704 |     2.0%
import/no-restricted-paths          |  2334.704 |     1.6%
react/no-string-refs                |  1845.670 |     1.3%
import/extensions                   |  1786.525 |     1.2%
react/no-unstable-nested-components |  1482.991 |     1.0%
react/require-render-return         |  1371.941 |     1.0%
import/named                        |  1118.946 |     0.8%

eslint-plugin-import@2.25.4, eslint@7.32.0, node@14.18.0

@epmatsw
Copy link

epmatsw commented Feb 22, 2022

Throwing some timing blocks around the visitors, it spends ~40% of the time in Program and ~60% in MemberExpression.

ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Feb 22, 2022
@ljharb
Copy link
Member

ljharb commented Feb 22, 2022

I've pushed up b0e6f7f but I don't expect it'll have a major impact on performance.

@epmatsw
Copy link

epmatsw commented Feb 22, 2022

A somewhat informed guess: it looks like it's maybe cache missing on different relative paths to the same file: ex. ../../thing vs ../thing. I wonder if you could normalize those somehow with something like:

let importPath = declaration.source.value;
if (/\.+\//.test(importPath)) {
    importPath = path.resolve(path.dirname(context.getPhysicalFilename()), importPath);
}

@ljharb
Copy link
Member

ljharb commented Feb 22, 2022

@epmatsw that does sound promising - if you wanted to try that locally (running the tests, and benchmarking it against your codebase) that would be very helpful :-) (even better, follow that up with a PR ;-) )

@epmatsw
Copy link

epmatsw commented Feb 22, 2022

Seems like no dice, unfortunately. No apparent improvement from putting that snippet into Program :(

@digeomel
Copy link

digeomel commented Feb 24, 2022

I'm getting similar results on an angular-eslint project:

Rule                                     | Time (ms) | Relative
:----------------------------------------|----------:|--------:
import/namespace                         |  4511.714 |    97.9%
import/no-unresolved                     |    70.773 |     1.5%
import/no-duplicates                     |    12.654 |     0.3%
@angular-eslint/component-selector       |     3.337 |     0.1%
import/export                            |     2.203 |     0.0%
import/no-named-as-default-member        |     1.985 |     0.0%
@angular-eslint/no-conflicting-lifecycle |     1.429 |     0.0%
@angular-eslint/no-output-rename         |     0.983 |     0.0%
import/default                           |     0.699 |     0.0%
@angular-eslint/contextual-lifecycle     |     0.690 |     0.0%

As you can see, the top 3 rules are from this plugin.

@aaronadamsCA
Copy link
Contributor Author

@digeomel Please make sure to compare total run time with the rule enabled vs. disabled.

I think it's now pretty well established that the rule has an internal slowdown, but if you want to add your data point, you'll need to provide that comparison.

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Apr 9, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint-plugin-import](https://github.com/import-js/eslint-plugin-import) | devDependencies | minor | [`2.25.4` -> `2.26.0`](https://renovatebot.com/diffs/npm/eslint-plugin-import/2.25.4/2.26.0) |

---

### Release Notes

<details>
<summary>import-js/eslint-plugin-import</summary>

### [`v2.26.0`](https://github.com/import-js/eslint-plugin-import/blob/HEAD/CHANGELOG.md#&#8203;2260---2022-04-05)

[Compare Source](import-js/eslint-plugin-import@v2.25.4...v2.26.0)

##### Added

-   \[`no-named-default`, `no-default-export`, `prefer-default-export`, `no-named-export`, `export`, `named`, `namespace`, `no-unused-modules`]: support arbitrary module namespace names (\[[#&#8203;2358](import-js/eslint-plugin-import#2358)], thanks \[[@&#8203;sosukesuzuki](https://github.com/sosukesuzuki)])
-   \[`no-dynamic-require`]: support dynamic import with espree (\[[#&#8203;2371](import-js/eslint-plugin-import#2371)], thanks \[[@&#8203;sosukesuzuki](https://github.com/sosukesuzuki)])
-   \[`no-relative-packages`]: add fixer (\[[#&#8203;2381](import-js/eslint-plugin-import#2381)], thanks \[[@&#8203;forivall](https://github.com/forivall)])

##### Fixed

-   \[`default`]: `typescript-eslint-parser`: avoid a crash on exporting as namespace (thanks \[[@&#8203;ljharb](https://github.com/ljharb)])
-   \[`export`]/TypeScript: false positive for typescript namespace merging (\[[#&#8203;1964](import-js/eslint-plugin-import#1964)], thanks \[[@&#8203;magarcia](https://github.com/magarcia)])
-   \[`no-duplicates`]: ignore duplicate modules in different TypeScript module declarations (\[[#&#8203;2378](import-js/eslint-plugin-import#2378)], thanks \[[@&#8203;remcohaszing](https://github.com/remcohaszing)])
-   \[`no-unused-modules`]: avoid a crash when processing re-exports (\[[#&#8203;2388](import-js/eslint-plugin-import#2388)], thanks \[[@&#8203;ljharb](https://github.com/ljharb)])

##### Changed

-   \[Tests] `no-nodejs-modules`: add tests for node protocol URL (\[[#&#8203;2367](import-js/eslint-plugin-import#2367)], thanks \[[@&#8203;sosukesuzuki](https://github.com/sosukesuzuki)])
-   \[Tests] `default`, `no-anonymous-default-export`, `no-mutable-exports`, `no-named-as-default-member`, `no-named-as-default`: add tests for arbitrary module namespace names (\[[#&#8203;2358](import-js/eslint-plugin-import#2358)], thanks \[[@&#8203;sosukesuzuki](https://github.com/sosukesuzuki)])
-   \[Docs] \[`no-unresolved`]: Fix RegExp escaping in readme (\[[#&#8203;2332](import-js/eslint-plugin-import#2332)], thanks \[[@&#8203;stephtr](https://github.com/stephtr)])
-   \[Refactor] `namespace`: try to improve performance (\[[#&#8203;2340](import-js/eslint-plugin-import#2340)], thanks \[[@&#8203;ljharb](https://github.com/ljharb)])
-   \[Docs] make rule doc titles consistent (\[[#&#8203;2393](import-js/eslint-plugin-import#2393)], thanks \[[@&#8203;TheJaredWilcurt](https://github.com/TheJaredWilcurt)])
-   \[Docs] `order`: TS code examples should use TS code blocks (\[[#&#8203;2411](import-js/eslint-plugin-import#2411)], thanks \[[@&#8203;MM25Zamanian](https://github.com/MM25Zamanian)])
-   \[Docs] `no-unresolved`: fix link (\[[#&#8203;2417](import-js/eslint-plugin-import#2417)], thanks \[[@&#8203;kylemh](https://github.com/kylemh)])

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1284
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@the-ult
Copy link

the-ult commented Apr 21, 2022

With import/order enabled in eslint.json
20sec
Screenshot 2022-04-21 at 14 56 13

without:
13sec
Screenshot 2022-04-21 at 15 02 40

For a simple (small library)

@ljharb
Copy link
Member

ljharb commented Apr 21, 2022

@the-ult theres a bunch of import rules in that diff, not just order.

@the-ult
Copy link

the-ult commented Apr 21, 2022

yes @ljharb , you are right.
However. Disabling "import/namespace": "off", specifically gives a major performance boost. With both other plugins enabled AND/OR disabled.

Tried disabling all other plugins / rules etc as well. But the difference would be like 1 sec per project.

Somehow, there seems to be something else going on as well (maybe NX related and gonna look into that), cause the lint is even much slower than the TIMING seconds displayed.

[EDIT]
setting "import/extensions": [".ts"], seems to help as well :

"settings": {
   "import/extensions": [".ts"],  // THIS GIVES A MAJOR PERFORMANCE BOOST
},
rules: {
  "import/namespace": "error"
} 

@ljharb
Copy link
Member

ljharb commented Apr 21, 2022

The import/extensions setting always needs to be set, so it's a bug in your config if it's not.

With that fixed config, what's the speed difference (with a bunch of import rules enabled) between "import/namespace on" and "import/namespace off"? (the eslint text output, please; screenshots of text aren't accessible)

@OutdatedVersion
Copy link
Contributor

OutdatedVersion commented Apr 21, 2022

(Derailing a bit. We can take it elsewhere if that's best.)

The import/extensions setting always needs to be set, so it's a bug in your config if it's not.

In this case, the README could be clarified. Its current wording suggests it is optional, I believe:

You may set the following settings in your .eslintrc:
This defaults to ['.js']

Would it make sense to say "Despite the default, it is recommended to explicitly set import/extensions for your use-case."?

@ljharb
Copy link
Member

ljharb commented Apr 21, 2022

Yes, that'd be great. It especially must be set if you're using "not standard javascript", namely, TS.

@the-ult
Copy link

the-ult commented Apr 22, 2022

The import/extensions setting always needs to be set, so it's a bug in your config if it's not.

This is/was indeed not very clear in the documentation.

Could/should it be part of the - plugin:import/typescript ??
Or be part of the documentation here?

With that fixed config, what's the speed difference (with a bunch of import rules enabled) between "import/namespace on" and "import/namespace off"? (the eslint text output, please; screenshots of text aren't accessible)

Tried to show it with the screenshots with the different build times.
But it seems like in some cases from 35sec to 12sec or even 1min to 10sec. So a major difference.

Will test/try it some more today

@JounQin
Copy link
Collaborator

JounQin commented Jul 25, 2022

In my business case, import/no-duplicates is very slow (I disabled import/namespace for .ts files).

企业微信截图_2de8b34d-93d2-481f-962e-a87439e61e01

@ljharb
Copy link
Member

ljharb commented Aug 30, 2022

See #2340 (comment); whichever is the first rule to build the ExportMap will be the slowest.

nickrttn added a commit to markprompt/markprompt-js that referenced this issue May 28, 2023
@tylerlaprade
Copy link

I have another data point from my own codebase. Of note is that my 2nd slowest import/ rule (no-unused-modules) saw a minor increase in time, but nowhere close to the time saved by excluding import/namespace.

Rule On:

Total: 22,772 ms
image

Rule Off:

Total: 16,133 ms
image

@jorisw
Copy link

jorisw commented Jan 15, 2024

Seeing as apparently the first rule that builds the export map is always reported as the slowest, thus is misrepresenting time spent to the user, could this be seen as a bug in the reporting of the TIMING feature? This way we can't really know where to look for the bottleneck in our linter setup.

@ljharb
Copy link
Member

ljharb commented Jan 15, 2024

Unfortunately since eslint doesn't provide any kind of "rule prep" stage, there's no way to separate out the export map timing from the rule timing.

@jorisw
Copy link

jorisw commented Jan 15, 2024 via email

@ljharb
Copy link
Member

ljharb commented Jan 15, 2024

hmm, maybe! I have no idea how eslint decides to order the rules.

What's probably easiest is to run timings; then disable the slowest rule and run them again, and in theory all but two should be roughly the same - and the one that's massively bigger in each run would be the one containing the export map buildup.

qwqcode added a commit to ArtalkJS/Artalk that referenced this issue Oct 18, 2024
- Add new ESLint rules for ArtalkPlugin:
  - Available rules: `noCycleDeps`, `noLifeCycleEventInNestedBlocks`, `noEventInWatchConf`, `noInjectInNestedBlocks`, `noInjectOutsidePlugin`, `onePluginPerFile`. For more details, see [eslint-plugin-artalk](https://github.com/ArtalkJS/Artalk/tree/master/ui/eslint-plugin-artalk).
- Add tests for the new ESLint rules.
- Implement Tarjan's algorithm for detecting SCCs in the `noCycleDeps` rule.
- Improve code organization and maintainability.
- Disable the 'import-x/namespace' rule for better performance. (import-js/eslint-plugin-import#2340)
qwqcode added a commit to ArtalkJS/Artalk that referenced this issue Oct 18, 2024
- Add new ESLint rules for ArtalkPlugin:
  - Available rules: `noCycleDeps`, `noLifeCycleEventInNestedBlocks`, `noEventInWatchConf`, `noInjectInNestedBlocks`, `noInjectOutsidePlugin`, `onePluginPerFile`. For more details, see [eslint-plugin-artalk](https://github.com/ArtalkJS/Artalk/tree/master/ui/eslint-plugin-artalk).
- Add tests for the new ESLint rules.
- Implement Tarjan's algorithm for detecting SCCs in the `noCycleDeps` rule.
- Improve code organization and maintainability.
- Disable the 'import-x/namespace' rule for better performance. (import-js/eslint-plugin-import#2340)
qwqcode added a commit to ArtalkJS/Artalk that referenced this issue Oct 18, 2024
- Add new ESLint rules for ArtalkPlugin:
  - Available rules: `noCycleDeps`, `noLifeCycleEventInNestedBlocks`, `noEventInWatchConf`, `noInjectInNestedBlocks`, `noInjectOutsidePlugin`, `onePluginPerFile`. For more details, see [eslint-plugin-artalk](https://github.com/ArtalkJS/Artalk/tree/master/ui/eslint-plugin-artalk).
- Add tests for the new ESLint rules.
- Implement Tarjan's algorithm for detecting SCCs in the `noCycleDeps` rule.
- Improve code organization and maintainability.
- Disable the 'import-x/namespace' rule for better performance. (import-js/eslint-plugin-import#2340)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

9 participants