import-js / eslint-plugin-import

ESLint plugin with rules that help validate proper imports.
MIT License
5.57k stars 1.57k forks source link

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

Closed soryy708 closed 2 months ago

soryy708 commented 2 months ago

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[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/import-js/eslint-plugin-import/pull/3068/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=import-js) | Coverage Δ | | |---|---|---| | [](https://app.codecov.io/gh/import-js/eslint-plugin-import/pull/3068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=import-js) | `94.37% <100.00%> (+2.16%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=import-js#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

soryy708 commented 2 months ago

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 commented 2 months ago

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

soryy708 commented 2 months ago

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