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

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

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

soryy708
Copy link
Contributor

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)

Copy link

codecov bot commented Sep 23, 2024

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

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

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

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@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.

@ljharb 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 ljharb merged commit f1db531 into import-js:main Sep 25, 2024
307 checks passed
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 4, 2024
| 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)])
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 4, 2024
| 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)])
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants