[Performance] `no-cycle`: dont scc for each linted file #3068

soryy708 · 2024-09-23T15:13:11Z

I noticed that no-cycle has a cache miss on SCC very often,
turns out that each linted file builds its own SCC, which is unnecessary.
Lets speed things up, by using the already cached SCC, as long as the context is similar (excluding linted file path)

codecov · 2024-09-23T15:25:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.37%. Comparing base (5c9757c) to head (d0e4752).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3068      +/-   ##
==========================================
+ Coverage   92.21%   94.37%   +2.16%     
==========================================
  Files          82       82              
  Lines        3557     3558       +1     
  Branches     1244     1243       -1     
==========================================
+ Hits         3280     3358      +78     
+ Misses        277      200      -77

Flag	Coverage Δ
	`94.37% <100.00%> (+2.16%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

soryy708 · 2024-09-23T15:38:15Z

I ran this vs master on a large project and measured:

Without this change (v2.30.0): 139 seconds to lint just no-cycle
With this change: 85 seconds to lint just no-cycle
Impact: 38% decrease in lint time

soryy708 · 2024-09-23T16:11:50Z

Pushed a fix for false cache hit for when ExportMap of previous SCC run didn't contain the current file, resulting in us getting a partial SCC from cache.

I ran this vs master on a large project and measured:

Without this change (v2.30.0): 139 seconds to lint just no-cycle
With this change: 94 seconds to lint just no-cycle
Impact: 32% decrease in lint time

Still good IMO

ljharb

This looks good, assuming that there's nothing else in the context that might necessitate a fresh scc, including with all the parsers we support. Can you confirm?

soryy708 · 2024-09-25T15:03:24Z

@ljharb in Discord you asked me:

Are we're sure that those are the only 3 keys in context that matter?

You also said

We support eslint 2 - 8, so it'd be good to check the older ones too.

Ok, I logged the context object in StronglyConnectedComponentsBuilder.for, in various ESLint versions. Here are my observations:

ESLint 7.25.0:

{
  "cacheKey": "/path-to-somewhere/node_modules/@typescript-eslint/parser/dist/index.jsd9da56ee23e751184e57ead488db4381bbe0f9fdc987aa0e7c39a448ea5986827e67ce99a36f4c88b10f0e80d55b71bae8fe96ab1747797f208639408c00c378/path-to-somewhere/src/main.ts",
  "settings": {
    "import/extensions": [
      ".ts",
      ".cts",
      ".mts",
      ".tsx",
      ".js",
      ".jsx",
      ".mjs",
      ".cjs"
    ],
    "import/external-module-folders": [
      "node_modules",
      "node_modules/@types"
    ],
    "import/parsers": {
      "@typescript-eslint/parser": [
        ".ts",
        ".cts",
        ".mts",
        ".tsx"
      ]
    },
    "import/resolver": {
      "node": {
        "extensions": [
          ".ts",
          ".cts",
          ".mts",
          ".tsx",
          ".js",
          ".jsx",
          ".mjs",
          ".cjs"
        ]
      }
    }
  },
  "parserOptions": {
    "ecmaFeatures": {
      "globalReturn": false
    },
    "project": "tsconfig.json",
    "sourceType": "module"
  },
  "parserPath": "/path-to-somewhere/node_modules/@typescript-eslint/parser/dist/index.js",
  "path": "/path-to-somewhere/src/main.ts"
}

ESLint 6.8.0:

{
  "cacheKey": "/path-to-somewhere/node_modules/@typescript-eslint/parser/dist/index.jsd9da56ee23e751184e57ead488db4381bbe0f9fdc987aa0e7c39a448ea5986827e67ce99a36f4c88b10f0e80d55b71bae8fe96ab1747797f208639408c00c378/path-to-somewhere/src/main.ts",
  "settings": {
    "import/extensions": [
      ".ts",
      ".cts",
      ".mts",
      ".tsx",
      ".js",
      ".jsx",
      ".mjs",
      ".cjs"
    ],
    "import/external-module-folders": [
      "node_modules",
      "node_modules/@types"
    ],
    "import/parsers": {
      "@typescript-eslint/parser": [
        ".ts",
        ".cts",
        ".mts",
        ".tsx"
      ]
    },
    "import/resolver": {
      "node": {
        "extensions": [
          ".ts",
          ".cts",
          ".mts",
          ".tsx",
          ".js",
          ".jsx",
          ".mjs",
          ".cjs"
        ]
      }
    }
  },
  "parserOptions": {
    "ecmaFeatures": {
      "globalReturn": false
    },
    "project": "tsconfig.json",
    "sourceType": "module"
  },
  "parserPath": "/path-to-somewhere/node_modules/@typescript-eslint/parser/dist/index.js",
  "path": "/path-to-somewhere/src/main.ts"
}

ESLint 5.16.0:

{
  "cacheKey": "/path-to-somewhere/node_modules/@typescript-eslint/parser/dist/index.jsd579e5a92f2fb830c4066e8075b30bdff66e6bb3eafbbfe9dc70dc8ec7c603f47e67ce99a36f4c88b10f0e80d55b71bae8fe96ab1747797f208639408c00c378/path-to-somewhere/src/main.ts",
  "settings": {
    "import/extensions": [
      ".ts",
      ".cts",
      ".mts",
      ".tsx",
      ".js",
      ".jsx",
      ".mjs",
      ".cjs"
    ],
    "import/external-module-folders": [
      "node_modules",
      "node_modules/@types"
    ],
    "import/parsers": {
      "@typescript-eslint/parser": [
        ".ts",
        ".cts",
        ".mts",
        ".tsx"
      ]
    },
    "import/resolver": {
      "node": {
        "extensions": [
          ".ts",
          ".cts",
          ".mts",
          ".tsx",
          ".js",
          ".jsx",
          ".mjs",
          ".cjs"
        ]
      }
    }
  },
  "parserOptions": {
    "ecmaFeatures": {
      "globalReturn": false
    },
    "project": "tsconfig.json",
    "sourceType": "module",
    "ecmaVersion": 6
  },
  "parserPath": "/path-to-somewhere/node_modules/@typescript-eslint/parser/dist/index.js",
  "path": "/path-to-somewhere/src/main.ts"
}

ESLint 4.19.1:

{
  "cacheKey": "/path-to-somewhere/node_modules/@typescript-eslint/parser/dist/index.jsd579e5a92f2fb830c4066e8075b30bdff66e6bb3eafbbfe9dc70dc8ec7c603f47e67ce99a36f4c88b10f0e80d55b71bae8fe96ab1747797f208639408c00c378/path-to-somewhere/src/main.ts",
  "settings": {
    "import/extensions": [
      ".ts",
      ".cts",
      ".mts",
      ".tsx",
      ".js",
      ".jsx",
      ".mjs",
      ".cjs"
    ],
    "import/external-module-folders": [
      "node_modules",
      "node_modules/@types"
    ],
    "import/parsers": {
      "@typescript-eslint/parser": [
        ".ts",
        ".cts",
        ".mts",
        ".tsx"
      ]
    },
    "import/resolver": {
      "node": {
        "extensions": [
          ".ts",
          ".cts",
          ".mts",
          ".tsx",
          ".js",
          ".jsx",
          ".mjs",
          ".cjs"
        ]
      }
    }
  },
  "parserOptions": {
    "ecmaFeatures": {
      "globalReturn": false
    },
    "project": "tsconfig.json",
    "sourceType": "module",
    "ecmaVersion": 6
  },
  "parserPath": "/path-to-somewhere/node_modules/@typescript-eslint/parser/dist/index.js",
  "path": "/path-to-somewhere/src/main.ts"
}

ESLint 3.19.0:

{
  "cacheKey": "/path-to-somewhere/node_modules/@typescript-eslint/parser/dist/index.jsd579e5a92f2fb830c4066e8075b30bdff66e6bb3eafbbfe9dc70dc8ec7c603f47e67ce99a36f4c88b10f0e80d55b71bae8fe96ab1747797f208639408c00c378/path-to-somewhere/src/main.ts",
  "settings": {
    "import/extensions": [
      ".ts",
      ".cts",
      ".mts",
      ".tsx",
      ".js",
      ".jsx",
      ".mjs",
      ".cjs"
    ],
    "import/external-module-folders": [
      "node_modules",
      "node_modules/@types"
    ],
    "import/parsers": {
      "@typescript-eslint/parser": [
        ".ts",
        ".cts",
        ".mts",
        ".tsx"
      ]
    },
    "import/resolver": {
      "node": {
        "extensions": [
          ".ts",
          ".cts",
          ".mts",
          ".tsx",
          ".js",
          ".jsx",
          ".mjs",
          ".cjs"
        ]
      }
    }
  },
  "parserOptions": {
    "ecmaFeatures": {
      "globalReturn": false
    },
    "project": "tsconfig.json",
    "sourceType": "module",
    "ecmaVersion": 6
  },
  "parserPath": "/path-to-somewhere/node_modules/@typescript-eslint/parser/dist/index.js",
  "path": "/path-to-somewhere/src/main.ts"
}

For ESLint 2 I got:

  0:0  error  Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser.
The file does not match your project config: estree.ts.
The file must be included in at least one of the projects provided

In summary, I believe that yes that's all that interests us in the context object, in various supported ESLint versions.

@G-Rath

| datasource | package | from | to | | ---------- | -------------------- | ------ | ------ | | npm | eslint-plugin-import | 2.30.0 | 2.31.0 | ## [v2.31.0](https://github.com/import-js/eslint-plugin-import/blob/HEAD/CHANGELOG.md#2310---2024-10-03) ##### Added - support eslint v9 (\[[#2996](import-js/eslint-plugin-import#2996)], thanks \[[@G-Rath](https://github.com/G-Rath)] \[[@michaelfaith](https://github.com/michaelfaith)]) - \[`order`]: allow validating named imports (\[[#3043](import-js/eslint-plugin-import#3043)], thanks \[[@manuth](https://github.com/manuth)]) - \[`extensions`]: add the `checkTypeImports` option (\[[#2817](import-js/eslint-plugin-import#2817)], thanks \[[@phryneas](https://github.com/phryneas)]) ##### Fixed - `ExportMap` / flat config: include `languageOptions` in context (\[[#3052](import-js/eslint-plugin-import#3052)], thanks \[[@michaelfaith](https://github.com/michaelfaith)]) - \[`no-named-as-default`]: Allow using an identifier if the export is both a named and a default export (\[[#3032](import-js/eslint-plugin-import#3032)], thanks \[[@akwodkiewicz](https://github.com/akwodkiewicz)]) - \[`export`]: False positive for exported overloaded functions in TS (\[[#3065](import-js/eslint-plugin-import#3065)], thanks \[[@liuxingbaoyu](https://github.com/liuxingbaoyu)]) - `exportMap`: export map cache is tainted by unreliable parse results (\[[#3062](import-js/eslint-plugin-import#3062)], thanks \[[@michaelfaith](https://github.com/michaelfaith)]) - `exportMap`: improve cacheKey when using flat config (\[[#3072](import-js/eslint-plugin-import#3072)], thanks \[[@michaelfaith](https://github.com/michaelfaith)]) - adjust "is source type module" checks for flat config (\[[#2996](import-js/eslint-plugin-import#2996)], thanks \[[@G-Rath](https://github.com/G-Rath)]) ##### Changed - \[Docs] \[`no-relative-packages`]: fix typo (\[[#3066](import-js/eslint-plugin-import#3066)], thanks \[[@joshuaobrien](https://github.com/joshuaobrien)]) - \[Performance] \[`no-cycle`]: dont scc for each linted file (\[[#3068](import-js/eslint-plugin-import#3068)], thanks \[[@soryy708](https://github.com/soryy708)]) - \[Docs] \[`no-cycle`]: add `disableScc` to docs (\[[#3070](import-js/eslint-plugin-import#3070)], thanks \[[@soryy708](https://github.com/soryy708)]) - \[Tests] use re-exported `RuleTester` (\[[#3071](import-js/eslint-plugin-import#3071)], thanks \[[@G-Rath](https://github.com/G-Rath)]) - \[Docs] \[`no-restricted-paths`]: fix grammar (\[[#3073](import-js/eslint-plugin-import#3073)], thanks \[[@unbeauvoyage](https://github.com/unbeauvoyage)]) - \[Tests] \[`no-default-export`], \[`no-named-export`]: add test case (thanks \[[@G-Rath](https://github.com/G-Rath)])

@G-Rath

| datasource | package | from | to | | ---------- | -------------------- | ------ | ------ | | npm | eslint-plugin-import | 2.30.0 | 2.31.0 | ## [v2.31.0](https://github.com/import-js/eslint-plugin-import/blob/HEAD/CHANGELOG.md#2310---2024-10-03) ##### Added - support eslint v9 (\[[#2996](import-js/eslint-plugin-import#2996)], thanks \[[@G-Rath](https://github.com/G-Rath)] \[[@michaelfaith](https://github.com/michaelfaith)]) - \[`order`]: allow validating named imports (\[[#3043](import-js/eslint-plugin-import#3043)], thanks \[[@manuth](https://github.com/manuth)]) - \[`extensions`]: add the `checkTypeImports` option (\[[#2817](import-js/eslint-plugin-import#2817)], thanks \[[@phryneas](https://github.com/phryneas)]) ##### Fixed - `ExportMap` / flat config: include `languageOptions` in context (\[[#3052](import-js/eslint-plugin-import#3052)], thanks \[[@michaelfaith](https://github.com/michaelfaith)]) - \[`no-named-as-default`]: Allow using an identifier if the export is both a named and a default export (\[[#3032](import-js/eslint-plugin-import#3032)], thanks \[[@akwodkiewicz](https://github.com/akwodkiewicz)]) - \[`export`]: False positive for exported overloaded functions in TS (\[[#3065](import-js/eslint-plugin-import#3065)], thanks \[[@liuxingbaoyu](https://github.com/liuxingbaoyu)]) - `exportMap`: export map cache is tainted by unreliable parse results (\[[#3062](import-js/eslint-plugin-import#3062)], thanks \[[@michaelfaith](https://github.com/michaelfaith)]) - `exportMap`: improve cacheKey when using flat config (\[[#3072](import-js/eslint-plugin-import#3072)], thanks \[[@michaelfaith](https://github.com/michaelfaith)]) - adjust "is source type module" checks for flat config (\[[#2996](import-js/eslint-plugin-import#2996)], thanks \[[@G-Rath](https://github.com/G-Rath)]) ##### Changed - \[Docs] \[`no-relative-packages`]: fix typo (\[[#3066](import-js/eslint-plugin-import#3066)], thanks \[[@joshuaobrien](https://github.com/joshuaobrien)]) - \[Performance] \[`no-cycle`]: dont scc for each linted file (\[[#3068](import-js/eslint-plugin-import#3068)], thanks \[[@soryy708](https://github.com/soryy708)]) - \[Docs] \[`no-cycle`]: add `disableScc` to docs (\[[#3070](import-js/eslint-plugin-import#3070)], thanks \[[@soryy708](https://github.com/soryy708)]) - \[Tests] use re-exported `RuleTester` (\[[#3071](import-js/eslint-plugin-import#3071)], thanks \[[@G-Rath](https://github.com/G-Rath)]) - \[Docs] \[`no-restricted-paths`]: fix grammar (\[[#3073](import-js/eslint-plugin-import#3073)], thanks \[[@unbeauvoyage](https://github.com/unbeauvoyage)]) - \[Tests] \[`no-default-export`], \[`no-named-export`]: add test case (thanks \[[@G-Rath](https://github.com/G-Rath)])

[Performance] no-cycle: dont scc for each linted file

f1db531

This was referenced Sep 23, 2024

Even larger import/no-cycle performance downgrade in 2.30.0 #3047

Open

no-cycle performance in 2.30 is even worse than 2.27.5 #3060

Open

ljharb approved these changes Sep 23, 2024

View reviewed changes

ljharb force-pushed the dont-scc-for-each-file branch from d0e4752 to f1db531 Compare September 25, 2024 16:59

ljharb changed the title ~~don't run scc for each linted file, instead use what's already cached~~ [Performance] no-cycle: dont scc for each linted file Sep 25, 2024

ljharb merged commit f1db531 into import-js:main Sep 25, 2024
307 checks passed

thlucas1 mentioned this pull request Oct 22, 2024

Bump eslint-plugin-import from 2.30.0 to 2.31.0 thlucas1/spotifyplus_card#3

Closed

ManuelDevWeb mentioned this pull request Oct 25, 2024

[Snyk] Upgrade eslint-plugin-import from 2.22.1 to 2.31.0 ManuelDevWeb/platziVideoReactRouterYRedux#339

Open

indy2kro mentioned this pull request Oct 25, 2024

[Snyk] Upgrade eslint-plugin-import from 2.30.0 to 2.31.0 indy2kro/laravel-git-hooks#14

Merged

alhugot-azpn mentioned this pull request Oct 25, 2024

security(snyk): upgrade eslint-plugin-import from 2.30.0 to 2.31.0 emerson-eps/eslint-config#277

Merged

qdraw mentioned this pull request Oct 25, 2024

[Snyk] Upgrade eslint-plugin-import from 2.30.0 to 2.31.0 qdraw/starsky#1785

Merged

richyhoopd mentioned this pull request Oct 26, 2024

[Snyk] Upgrade eslint-plugin-import from 2.20.2 to 2.31.0 richyhoopd/Video-App#1239

Open

ManuelDevWeb mentioned this pull request Oct 26, 2024

[Snyk] Upgrade eslint-plugin-import from 2.22.1 to 2.31.0 ManuelDevWeb/platziVideoReact#339

Open

Vishesh7797 mentioned this pull request Oct 27, 2024

[Snyk] Upgrade eslint-plugin-import from 2.14.0 to 2.31.0 Vishesh7797/material-ui#4

Open

Exkaleburx mentioned this pull request Oct 27, 2024

[Snyk] Upgrade eslint-plugin-import from 2.20.1 to 2.31.0 Exkaleburx/plug-and-klay#19

Open

Mastermindzh mentioned this pull request Oct 27, 2024

[Snyk] Upgrade eslint-plugin-import from 2.26.0 to 2.31.0 Mastermindzh/eslint-config#47

Closed

DadonStyle mentioned this pull request Oct 27, 2024

[Snyk] Upgrade eslint-plugin-import from 2.25.4 to 2.31.0 DadonStyle/Crypto-Tower-FE#37

Open

This was referenced Oct 28, 2024

[Snyk] Upgrade eslint-plugin-import from 2.29.1 to 2.31.0 nodeshift/kube-service-bindings#242

Closed

[Snyk] Upgrade eslint-plugin-import from 2.27.5 to 2.31.0 nodeshift/opossum#902

Open

thlucas1 mentioned this pull request Nov 1, 2024

Bump eslint-plugin-import from 2.29.1 to 2.31.0 thlucas1/homeassistantcomponent_soundtouchplus_card#23

Closed

DadonStyle mentioned this pull request Nov 3, 2024

[Snyk] Upgrade eslint-plugin-import from 2.25.4 to 2.31.0 DadonStyle/Crypto-Tower-FE#42

Open

dpesall mentioned this pull request Nov 6, 2024

[Snyk] Upgrade eslint-plugin-import from 2.29.1 to 2.31.0 nasa/cmr-stac#377

Open

Luis-Kristen mentioned this pull request Nov 7, 2024

[Snyk] Upgrade eslint-plugin-import from 2.20.1 to 2.31.0 Luis-Kristen/feature-react#207

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Performance] `no-cycle`: dont scc for each linted file #3068

[Performance] `no-cycle`: dont scc for each linted file #3068

soryy708 commented Sep 23, 2024

codecov bot commented Sep 23, 2024 •

edited

Loading

soryy708 commented Sep 23, 2024

soryy708 commented Sep 23, 2024

ljharb left a comment

soryy708 commented Sep 25, 2024

[Performance] no-cycle: dont scc for each linted file #3068

[Performance] no-cycle: dont scc for each linted file #3068

Conversation

soryy708 commented Sep 23, 2024

codecov bot commented Sep 23, 2024 • edited Loading

Codecov Report

soryy708 commented Sep 23, 2024

soryy708 commented Sep 23, 2024

ljharb left a comment

Choose a reason for hiding this comment

soryy708 commented Sep 25, 2024

[Performance] `no-cycle`: dont scc for each linted file #3068

[Performance] `no-cycle`: dont scc for each linted file #3068

codecov bot commented Sep 23, 2024 •

edited

Loading