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

Huge import/no-cycle performance downgrade in 2.25.3 #2348

Closed
zaicevas opened this issue Jan 3, 2022 · 44 comments
Closed

Huge import/no-cycle performance downgrade in 2.25.3 #2348

zaicevas opened this issue Jan 3, 2022 · 44 comments

Comments

@zaicevas
Copy link

zaicevas commented Jan 3, 2022

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
Copy link
Member

ljharb commented Jan 3, 2022

Thanks, that's quite large.

The only changes that touched ExportMap in that range are 9a744f7 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
Copy link
Author

zaicevas commented Jan 4, 2022

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
Copy link
Member

ljharb commented Jan 4, 2022

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

@mrmckeb
Copy link
Contributor

mrmckeb commented Jan 5, 2022

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

@zaicevas
Copy link
Author

zaicevas commented Jan 5, 2022

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
Copy link

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
Copy link
Contributor

mrmckeb commented Jan 6, 2022

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/eslint-plugin-import@2.22.1/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
Copy link
Author

zaicevas commented Jan 6, 2022

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
Copy link
Author

zaicevas commented Jan 7, 2022

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
Copy link

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
Copy link

SDAdham commented Jul 26, 2022

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

@JounQin
Copy link
Collaborator

JounQin commented Jul 26, 2022

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

@SDAdham
Copy link

SDAdham commented Jul 26, 2022

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
Copy link
Collaborator

JounQin commented Jul 26, 2022

@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
Copy link

burtek commented Jul 26, 2022

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
Copy link

dartess commented Jul 27, 2022

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
Copy link

lrdxg1 commented Jul 27, 2022

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
Copy link
Collaborator

JounQin commented Jul 27, 2022

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

@clarfonthey
Copy link

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
Copy link

evelant commented Dec 3, 2022

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
Copy link

MayGo commented Feb 17, 2023

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

@ljharb
Copy link
Member

ljharb commented Feb 17, 2023

@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
Copy link

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
Copy link
Member

ljharb commented Apr 4, 2023

@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
Copy link

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
Copy link

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:

image

[2]

image

image

image

[3]

parsedFiles.txt

[4]

ExportMap.js

image

image

[5]

image

image

[6]

image

[7]

image

[8]

image

[9]

image

@ljharb
Copy link
Member

ljharb commented Apr 17, 2023

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
Copy link

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
Copy link
Member

ljharb commented Apr 17, 2023

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
Copy link

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

@TkDodo
Copy link

TkDodo commented Jun 12, 2023

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}],

I can confirm this. We had ignoreExternal on, so linting took around ~3 minutes. After I turned it off, it went down to ~1 minute

@ljharb
Copy link
Member

ljharb commented Jun 12, 2023

I'd expect an "ignore" option to make things faster, not slower, so there must be some low-hanging fruit here - a PR would be welcome.

@XDean
Copy link

XDean commented Jul 4, 2023

Is there any solution or plan? The newest 2.27.5 still very slow.

@sztadii
Copy link

sztadii commented Sep 13, 2023

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}],

Also I can confirm. From 13 minutes to 2 min.

@soryy708
Copy link
Contributor

Here's an idea for how to make import/no-cycle faster:
#2937

@martin-mindflow
Copy link

Bumping this, it's a also a big problem in our (semi-large) monorepo, with no-cycle accounting for ~80% of the relative linting time. None of the workarounds of this thread worked for us :( so far we've simply resorted to outsourcing cycle detection to another tool (madge), but the DX is clearly worse than having it build-in to eslint

@diegodorado
Copy link

Same issue here: with ignoreExternal it is actually worst... I was expecting that flag to improve linting time

@ljharb
Copy link
Member

ljharb commented May 15, 2024

When #2998 is merged and released, this should improve drastically.

@jzaefferer
Copy link

@ljharb thanks for getting that merged! Any estimate when it could get released? I'm mostly asking since the last regular release was back in December, so it's hard to guess when the next one might come around.

@soryy708
Copy link
Contributor

soryy708 commented Sep 2, 2024

@jzaefferer

ljharb thanks for getting that merged! Any estimate when it could get released? I'm mostly asking since the last regular release was back in December, so it's hard to guess when the next one might come around.

The maintainer is working on flat config support, so after it will be done, expect a minor version I think?

@soryy708
Copy link
Contributor

soryy708 commented Sep 2, 2024

@mrmckeb regarding previous conversation about your contribution: #2240

Resolving TypeScript config more correctly is a good change, but I'm curious if it could be a big contributor to filesystem accesses reported in profiling?

  • I don't know how many FS reads TS needs to compile an extended TS config. Can this be profiled?
  • Is it reasonable to put extended ts config behind a feature flag (lint rule option)? so only those who have multiple tsconfigs in hierarchy will be impacted by the performance hit?
  • Is it possible to cache results instead of resolving it from scratch each time?

@mrmckeb
Copy link
Contributor

mrmckeb commented Sep 3, 2024

Hi @soryy708!

As noted here, I did test this and found that it wasn't the cause of the significant performance impact noted in this thread. I could have missed something, but was confident at the time.
#2348 (comment)

I didn't go into profiling, so I'm not sure of the exact performance impact, but I remember it was very small, especially compared to the problem noted in this thread, which I could also reproduce on the release in the title.

I think in-memory caching of the fully resolved/parsed config is a good idea.

Putting extended configs behind a flag is probably the wrong solution here, especially as we don't know if it has caused any performance issues:

  • If the extends field isn't there, there shouldn't be a lookup (this is using native TypeScript functionality).
  • If the extends field is there and not being resolved, eslint-plugin-import won't work as expected.

@soryy708
Copy link
Contributor

soryy708 commented Sep 3, 2024

The SCC optimization has been released, try v >=2.30.0

@ljharb
Copy link
Member

ljharb commented Sep 3, 2024

I'm going to close this; if there's still a perf issue in v2.30+, please file a new issue.

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

No branches or pull requests