Huge `import/no-cycle` performance downgrade in 2.25.3

[zaicevas]

I upgraded eslint-plugin-import 2.22.1 -> 2.25.3 and the performance downgrade is huge. eslint is 7.24.0.

Roughly a hundred files:

Rule                                       |  Time (ms) | Relative
:------------------------------------------|-----------:|--------:
import/no-cycle                            | 304818.252 |    97.0%
prettier/prettier                          |   2347.669 |     0.7%
import/named                               |    754.553 |     0.2%

2.24.2 is fine:

Rule                                       | Time (ms) | Relative
:------------------------------------------|----------:|--------:
import/no-cycle                            |  4206.707 |    23.0%
prettier/prettier                          |  1914.555 |    10.5%
react/prefer-stateless-function            |   849.317 |     4.6%

[ljharb]

Thanks, that's quite large.

The only changes that touched ExportMap in that range are 9a744f73 and #2212.

Do you have a TypeScript codebase? Also, can you confirm that v2.25.4 (which contains #2341) has the same speed drop?

[zaicevas]

Do you have a TypeScript codebase?

Yes

can you confirm that v2.25.4 (which contains #2341) has the same speed drop?

Yes:

Rule                                          |  Time (ms) | Relative
:---------------------------------------------|-----------:|--------:
import/no-cycle                               | 300578.485 |    95.2%
prettier/prettier                             |   2384.299 |     0.8%
react/prefer-stateless-function               |    866.305 |     0.3%

[ljharb]

That strongly suggests it’s #2240. cc @mrmckeb

[mrmckeb]

Are you able to share a small reproduction of the issue @zaicevas? Or if not, can you share your tsconfig.json?

[zaicevas]

Are you able to share a small reproduction of the issue @zaicevas?

Sorry, won't be able to do that due to lack of time.

tsconfig.json:

{
  "compilerOptions": {
    "target": "ES2016",
    "module": "commonjs",
    "jsx": "react-jsx",
    "noEmit": true,
    "strict": true,
    "noImplicitAny": false,
    "noImplicitThis": false,
    "baseUrl": "./app/",
    "esModuleInterop": true,
    "allowUnreachableCode": false,
    "isolatedModules": true
  },
  "include": [
    "./app/**/*.ts",
    "./app/**/*.tsx"
  ],
  "exclude": [
    "./node_modules/**/*"
  ]
}

[sadovnychyi]

Got 30s speedup (1:59s -> 1:29s for total linting time) by setting ignoreExternal to false.

-'import/no-cycle': [2, {ignoreExternal: true, maxDepth: 3}],
+'import/no-cycle': [2, {ignoreExternal: false, maxDepth: 3}],

[mrmckeb]

Thanks all, I did some quick testing around this today and can confirm that there is a massive performance downgrade between these releases.

Findings

The following are timings for import/cycle. This test project has 7 JavaScript and TypeScript files.

Version	New tsconfig resolution	Old tsconfig resolution
2.22.1	~80ms	~80ms
2.25.4	~235ms	~235ms

It seems that the issue is related to another change, @ljharb.

Methodology

1. Install the latest 2.25.4 alongside a fresh Next.js app.
2. Enable import/no-cycle and run TIMING=1 yarn eslint .
3. Copy this file in place of the version that shipped with 2.25.4. https://unpkg.com/[email protected]/lib/ExportMap.js
4. Modify the code (around line 705) to reimplement the new behaviour.

   
         // const jsonText = _fs2.default.readFileSync(tsConfigInfo.tsConfigPath).toString();
         // if (!parseConfigFileTextToJson) {
         //   var _require = require('typescript');
         //   parseConfigFileTextToJson = _require.parseConfigFileTextToJson;
         // }
         // const tsConfig = parseConfigFileTextToJson(tsConfigInfo.tsConfigPath, jsonText).config;
         // return tsConfig.compilerOptions.esModuleInterop;
   
         const ts = require('typescript');
         const path = require('path');
         const configFile = ts.readConfigFile(tsConfigInfo.tsConfigPath, ts.sys.readFile);
         const config = ts.parseJsonConfigFileContent(
           configFile.config,
           ts.sys,
           path.dirname(tsConfigInfo.tsConfigPath),
         );
         // So we don't have to make other changes in this file.
         config.compilerOptions = config.options;
         return config;
   

5. Do the same in reverse (reimplement old behaviour in new version).

[zaicevas]

2.22.1 -> 2.24.2 is also a significant performance downgrade:

2.22.1:

import/no-cycle                            | 15407.126 |     6.1%

2.24.2:

import/no-cycle                   | 70428.732 |    36.3%

[zaicevas]

It turns out that 2.24.2 does not report import/no-cycle violations at all 😕

// a.ts
import './b'

export function ab() {
  //
}

// b.ts
import { ab } from './a'

[DarkPurple141]

Keen to watch this issue, have noted this rule is eating into our CI, accounting for ~80% of our overall linting pipeline in a large-ish monorepo. I'm interested to know whether there is any thought on caching cycles previously searched across files to decrease the runtime performance hit of the rule.

[SDAdham]

same here, no-cycle seems to be a performance destroyer

[JounQin]

Can you try https://github.com/un-es/eslint-plugin-i to see whether it's much faster?

[SDAdham]

It didn't work:

yarn add -D eslint-plugin-import@npm:eslint-plugin-i
➤ YN0000: ┌ Resolution step
➤ YN0016: │ eslint-plugin-import@npm:eslint-plugin-i: Registry failed to return tag "eslint-plugin-i"
➤ YN0000: └ Completed in 3s 302ms
➤ YN0000: Failed with errors in 3s 305ms

[JounQin]

@SDAdham Sorry I don't use yarn@v2+, maybe you can find something similar to package aliasing.

---

https://yarnpkg.com/features/protocols/#table

So it seems eslint-plugin-import@npm:eslint-plugin-i@latest should do the trick.

[burtek]

https://yarnpkg.com/features/protocols/#table

So it seems eslint-plugin-import@npm:eslint-plugin-i@latest should do the trick.

eslint-plugin-import is a nested dependency in my case, but it worked for me (using yarn) to use eslint-plugin-i via (in package.json):

{
  // rest of package.json
  "resolutions": {
    "eslint-plugin-import": "npm:eslint-plugin-i@latest"
  }
}

[dartess]

Can you try https://github.com/un-es/eslint-plugin-i to see whether it's much faster?

Maybe I'm doing something wrong, but I got exactly the same result.

[lrdxg1]

Same here unfortunately. I was quite excited for this fix and was able to use the yarn resolution field to force eslint-plugin-i to be used but no difference.

Before:

Rule                                | Time (ms) | Relative
:-----------------------------------|----------:|--------:
import/no-cycle                     | 70282.194 |    38.1%
import/no-relative-packages         | 10151.240 |     5.5%
import/no-extraneous-dependencies   |  8517.592 |     4.6%
react/destructuring-assignment      |  7432.624 |     4.0%
import/order                        |  4776.704 |     2.6%
react/no-did-update-set-state       |  3923.661 |     2.1%
@typescript-eslint/no-unused-vars   |  3896.220 |     2.1%
react/no-unstable-nested-components |  2844.975 |     1.5%
react/no-deprecated                 |  2837.452 |     1.5%
import/extensions                   |  2737.678 |     1.5%

After:

Rule                                | Time (ms) | Relative
:-----------------------------------|----------:|--------:
import/no-cycle                     | 73077.763 |    37.5%
import/no-relative-packages         | 10611.396 |     5.4%
import/no-extraneous-dependencies   |  8764.613 |     4.5%
react/destructuring-assignment      |  7719.522 |     4.0%
import/order                        |  5061.809 |     2.6%
react/no-did-update-set-state       |  4245.396 |     2.2%
@typescript-eslint/no-unused-vars   |  4151.959 |     2.1%
react/no-deprecated                 |  3027.035 |     1.6%
react/no-unstable-nested-components |  2995.082 |     1.5%
react/function-component-definition |  2980.219 |     1.5%

[JounQin]

Then it's not a resolving issue but performance effects in ExportMap.

[clarfonthey]

We have a large TS monorepo that takes around a half hour to lint using import/no-cycle. I'm not expecting it to be blazingly fast, but, there's definitely some performance problems here.

[evelant]

Anybody managed to solve this one? I just tried to add eslint to my repo for the first time ever and ran into this. Linting a single file takes 3+ minutes which is obviously not usable.

edit: actually it takes 10 minutes for a single file haha

Rule                                              |  Time (ms) | Relative
:-------------------------------------------------|-----------:|--------:
import/no-cycle                                   | 645443.504 |   100.0%
import/no-relative-packages                       |     65.605 |     0.0%
import/no-duplicates                              |     12.048 |     0.0%
simple-import-sort/imports                        |     11.205 |     0.0%
@typescript-eslint/consistent-type-imports        |      9.080 |     0.0%
no-relative-import-paths/no-relative-import-paths |      7.600 |     0.0%
@typescript-eslint/no-loss-of-precision           |      4.117 |     0.0%
@typescript-eslint/no-empty-function              |      2.528 |     0.0%
import/newline-after-import                       |      2.356 |     0.0%
no-constant-condition                             |      2.208 |     0.0%

Setting maxDepth: 2 makes it take 20s instead, but 20s is still much too slow for a single file.

[MayGo]

Is this going to be fixed, or is there a workaround?

[ljharb]

@MayGo it will be fixed once there's a PR to fix it, and there's no workaround or else that would be the fix.

[tnguyen42]

Do we know if this issue exists on both npm and yarn, and if it exists on all versions of yarn? Or is there some other cases where it doesn't exist? I'm surprised there is so little attention to it since it's been open for 15 months now

[ljharb]

@tnguyen42 npm or yarn should have no impact on it at all, since it's about runtime performance.

It's not that it's been paid "so little attention", it's that performance issues are very hard to reproduce reliably, and without that, they're very hard to fix.

[lukaselmer]

We're also working in a monorepo, and we just updated to the new eslint version. The performance still seems to be a big issue - import/no-cycle is unusable like this. I'll try to analyze eslint-plugin-import in the next couple of days and try to fix the performance issue.

Rule                                    | Time (ms) | Relative
:---------------------------------------|----------:|--------:
import/no-cycle                         | 58819.279 |    96.2%
@typescript-eslint/no-floating-promises |   751.793 |     1.2%
import/namespace                        |   317.888 |     0.5%
@typescript-eslint/naming-convention    |   260.983 |     0.4%
import/no-extraneous-dependencies       |   189.503 |     0.3%
import/no-unresolved                    |   116.565 |     0.2%
@typescript-eslint/no-redeclare         |   101.951 |     0.2%
react/display-name                      |    53.880 |     0.1%
import/order                            |    45.388 |     0.1%
react/no-direct-mutation-state          |    43.678 |     0.1%

[lukaselmer]

I found out a couple of things:

• Most time is spent within ExportMap.parse
• Reducing the depth improves the situation:
  • Depth 1: 3.92s
  • Depth 2: 10.40s
  • Depth 3: 14.46s
  • Depth 4: 15.56s
  • Depth 1000: 16.75s
• There is an option ignoreExternal
  • With this option on, the time goes down drastically to around 3s
  • However, when using absolute imports / path mapping / aliases, it produces false positives
    • for this reason, import/internal-regex needs to be configured to include these files, for example:

//  .eslintrc.js
module.exports = {
  settings: {
    'import/internal-regex': '^(@myNamespace/|client/|server/|shared/|__test__/|scripts/|types/)',
  },
  rules: {
    'import/no-cycle': ['error', { ignoreExternal: true }]
  },
  // [other config...]
}

• Most of the time spent in ExportMap.parse was used to parse files from libraries. The most impactful two libraries for me are
  • @mui/icons-material
  • @mui/material
• The main issue is IMO that the library reads and parses all the files
  • I first thought that fs.readFileSync may be the main issue (instead of this,import {readFile} from 'fs/promises' and then await fs.readFile(...) could be used?), but according to the flame graph [1] and [2], parsing the file takes longer
  • Does it really have to parse all these files? From top to bottom? 🤔
  • Here's a list of files which are read and parsed by my project [3]
    • When I ignore the @mui files, the lint time goes down drastically (from ~16s to ~4s), see [4]
    • I also checked if actually reading the file makes a difference, and it still takes around 4s when returning after reading the file [5]
• The sorted flamegraph looks like this: [6]
  • On the right side [7], we can see the garbage collection, and the readFileSync function, and the visit function
  • Then we see that a bit more to the left, captureDependencyWithSpecifiers spends most of the time reading files [8]
  • On the left side [9] is where the parsing happens
  • Maybe I don't see something obvious that someone else notices?
• Conclusion
  • Reproducing the performance issues should be pretty easy by installing @mui-ions-material, @mui/material, and creating a file which imports one or two things from these packages. Since I used my own project to do the analysis, I cannot guarantee it, but I'm pretty confident that it can be reproduced like this
  • I don't know if it would help, but instead of doing everything in serial, we could use the async function to process / parse the files concurrently? There may be some IO that could be parallelized this way?
  • I don't understand why importing / analyzing external files makes sense - you should probably not have cycles between your code and external packages anyways, and if you do, you would notice it in package.json - right?
  • The analysis doesn't explain why updating eslint changes the performance of import/no-cycle - I'm just trying to understand why it's slow
  • For me, changing the configuration to ignore the external packages, and adding my own absolute prefixes, is a workable solution. Do you think it should be (better) documented that this is a potential solution if you're suffering from performance issues?

[1] Flamegraph overview:

[2]

[3]

parsedFiles.txt

[4]

ExportMap.js

[5]

[6]

[7]

[8]

[9]

[ljharb]

It has to parse every file fully in order to know what it imports/exports (or requires/module.exports, for CJS).

A half dozen rules depend on the export map - no-cycle is just one of them - but the map is cached across them, thankfully. This does mean that improvements in the export map benefit many rules at once.

I think it would be perfectly reasonable to detect the presence of worker_threads and do work in parallel when possible - the challenge would be to see if the communication overhead outweighed the CPU savings.

[lukaselmer]

Wow thanks for the quick reaction!

It has to parse every file fully in order to know what it imports/exports (or requires/module.exports, for CJS).

Ok, I understand that. I thought that a "lightweight" import (which only scans for imports/exports/requires/module.exports) could be faster.

What about the external files? E.g. there's no chance that @mui/material imports something from my app.

A half dozen rules depend on the export map - no-cycle is just one of them - but the map is cached across them, thankfully. This does mean that improvements in the export map benefit many rules at once.

Yes, I noticed, and I checked for that immediately, because it would have been a low-hanging fruit.

I think it would be perfectly reasonable to detect the presence of worker_threads and do work in parallel when possible - the challenge would be to see if the communication overhead outweighed the CPU savings.

Yep. I'm not convinced it would bring any benefits, but I'll try implementing it in the next couple of days. Do you have a good tip on how to do performance tests? I'm currently modifying the compiled version directly in my sample app directory, and I'm thinking there must be a better way... :)

[ljharb]

Unfortunately I don't :-/ usually i npm link the plugin, and npm link eslint-plugin-import into the project i want to test it with, and then run npm run build in the plugin after I've made changes, and then rerun the linting task in the project.

[Lonli-Lokli]

It turns out that 2.24.2 does not report import/no-cycle violations at all 😕

// a.ts
import './b'

export function ab() {
  //
}

// b.ts
import { ab } from './a'

Actually I think it's the biggest issue here