optimize no-cycle rule using strongly connected components #2998

soryy708 · 2024-04-08T18:09:46Z

Lets make the no-cycle rule faster by not running many unnecessary BFSes.

For each dependency graph (aka ExportMap) we can run Tarjan's SCC once (which is a derivative of DFS = O(n))

That saves us a lot of work because we run a linear-complexity algorithm once, as opposed to for each linted file (which turned us O(n^2))

#2937

…ErrorMessagePath` for faster error messages

socket-security · 2024-04-08T18:11:17Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/chai@3.5.0	None	`+4`	427 kB	chaijs
npm/enhanced-resolve@0.9.1	Transitive: environment, filesystem	`+3`	113 kB	sokra
npm/find-root@1.1.0	filesystem	`0`	5.27 kB	jsdnxx
npm/interpret@1.4.0	None	`0`	14.9 kB	phated
npm/resolve@2.0.0-next.5	environment, filesystem	`+2`	152 kB	ljharb
npm/webpack@5.94.0	Transitive: environment, eval, filesystem, shell, unsafe	`+75`	18.1 MB	evilebottnawi, jhnns, sokra, ...1 more

🚮 Removed packages: npm/@angular-eslint/template-parser@13.5.0), npm/@typescript-eslint/parser@5.62.0), npm/babel-cli@6.26.0), npm/babel-eslint@10.1.0), npm/babel-plugin-module-resolver@2.7.1), npm/babel-preset-airbnb@2.6.0), npm/babel-preset-flow@6.23.0), npm/chai@4.5.0), npm/cross-env@4.0.0), npm/escope@3.6.0), npm/eslint-doc-generator@1.7.1), npm/eslint-import-resolver-typescript@1.1.1), npm/eslint-plugin-eslint-plugin@2.3.0), npm/eslint-plugin-json@2.1.2), npm/find-babel-config@=1.2.0), npm/fs-copy-file-sync@1.1.1), npm/in-publish@2.0.1), npm/jackspeak@=2.1.1), npm/jsonc-parser@=3.2.0), npm/linklocal@2.8.2), npm/lodash.isarray@4.0.0), npm/markdownlint-cli@0.35.0), npm/npm-which@3.0.1), npm/redux@3.7.2), npm/rimraf@2.7.1), npm/safe-publish-latest@2.0.0), npm/sinon@2.4.1)

View full report↗︎

socket-security · 2024-04-08T18:11:23Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

soryy708 · 2024-04-08T18:14:08Z

Why is socket-security failing this?
The changes in the dependencies it found don't make sense.
The library I added @rtsao/scc has no dependencies at all.
Here's their package.json: https://github.com/rtsao/scc/blob/1120edc92040b0ca748ab62a882a98565c85deed/package.json

ljharb · 2024-04-08T23:23:17Z

Good question. I think it's that this is the first new PR it's been run on since i installed it?

src/rules/no-cycle.js

tests/src/rules/no-cycle.js

codecov · 2024-04-10T17:59:04Z

Codecov Report

Attention: Patch coverage is 96.15385% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.91%. Comparing base (bdff75d) to head (55c2741).

Files	Patch %	Lines
src/rules/no-cycle.js	80.00%	1 Missing ⚠️
src/scc.js	97.87%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2998       +/-   ##
===========================================
+ Coverage   85.33%   95.91%   +10.57%     
===========================================
  Files          78       79        +1     
  Lines        3300     3352       +52     
  Branches     1160     1171       +11     
===========================================
+ Hits         2816     3215      +399     
+ Misses        484      137      -347

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

src/scc.js

tests/src/rules/no-cycle.js

src/rules/no-cycle.js

ljharb

Looks great! I'll look at failing tests next

src/scc.js

tests/src/rules/no-cycle.js

pumano · 2024-05-20T09:09:14Z

would love to see that optimization!

jonahallibone · 2024-05-29T18:17:21Z

Can't wait for this to be merged!

pumano · 2024-06-10T18:53:48Z

@ljharb any news on this bro?

soryy708 · 2024-08-23T13:48:43Z

There's a pathology with SCC right now:

It traverses ignored modules (isExternalModule), which slows it down

ljharb

I see two uncovered lines - can we add test cases to cover them?

Otherwise, this LGTM assuming all existing tests pass!

soryy708 · 2024-08-24T10:20:21Z

About the uncovered lines,

I see them in codecov. The 1st is early return marked "no-self-import territory", so unrelated to my changes.

The other is early return when files don't have the same SCC.
Unfortunately it can never happen in the tests since ExportMap is null for some reason, so both files are in an undefined scc.

src/scc.js

soryy708 · 2024-08-26T16:52:10Z

After all these changes, I decided to measure performance impact.
I ran lint on a big TypeScript codebase with many circular dependencies, and looked at total command execution time (as reported by yarn).

Without SCC (disableScc: true):

367 seconds (6:07 minutes)

With SCC:

133 seconds (2:13 minutes)

(I made sure it isn't using eslint cache)

So we're talking about a 63% decrease in run time.

JounQin · 2024-08-27T05:52:27Z

Notice that we discovered performance downgrade at un-ts#113 when we ported this "optimization", I don't quite sure whether there were some improvements after we ported.

cc @SukkaW

soryy708 · 2024-08-27T06:22:54Z

Notice that we discovered performance downgrade at un-ts#113 when we ported this "optimization", I don't quite sure whether there were some improvements after we ported.

cc @SukkaW

Thanks, I have already seen this. What I did about it from when it was ported to the community fork:

Simplified things by ripping out "skip error message path" option. This means less things can go wrong
Changed SCC to differentiate between value and type imports
Changed BFS to look at SCC
Added option to disable SCC via config

In summary, this PR diverged significantly from the port discussed, in a way which should improve performance, invalidating prior benchmarks. A workaround has been built in to fall-back on in case of performance regression, without rolling-back.

SukkaW · 2024-08-27T07:54:06Z

Notice that we discovered performance downgrade at un-ts#113 when we ported this "optimization", I don't quite sure whether there were some improvements after we ported.
cc @SukkaW

Thanks, I have already seen this. What I did about it from when it was ported to the community fork:

Simplified things by ripping out "skip error message path" option. This means less things can go wrong

Changed SCC to differentiate between value and type imports

Changed BFS to look at SCC

Added option to disable SCC via config

In summary, this PR diverged significantly from the port discussed, in a way which should improve performance, invalidating prior benchmarks. A workaround has been built in to fall-back on in case of performance regression, without rolling-back.

It was only after backporting the PR that I realized this was still an early experiment and a PoC.

I'm considering using @newdash/graphlib (which powers cycle-import-check) to reimplement the no-cycle rule. With an existing graph library, we can effortlessly identify any "circle" within the module graph. However, without appending extra information, we lose track of the AST node.

src/rules/no-cycle.js

ljharb

awesome, LGTM once tests pass

soryy708 · 2024-09-03T18:17:23Z

Should be included in v 2.30.0

@JiangWeixian

| datasource | package | from | to | | ---------- | -------------------- | ------ | ------ | | npm | eslint-plugin-import | 2.29.1 | 2.30.0 | ## [v2.30.0](https://github.com/import-js/eslint-plugin-import/blob/HEAD/CHANGELOG.md#2300---2024-09-02) ##### Added - \[`dynamic-import-chunkname`]: add `allowEmpty` option to allow empty leading comments (\[[#2942](import-js/eslint-plugin-import#2942)], thanks \[[@JiangWeixian](https://github.com/JiangWeixian)]) - \[`dynamic-import-chunkname`]: Allow empty chunk name when webpackMode: 'eager' is set; add suggestions to remove name in eager mode (\[[#3004](import-js/eslint-plugin-import#3004)], thanks \[[@amsardesai](https://github.com/amsardesai)]) - \[`no-unused-modules`]: Add `ignoreUnusedTypeExports` option (\[[#3011](import-js/eslint-plugin-import#3011)], thanks \[[@silverwind](https://github.com/silverwind)]) - add support for Flat Config (\[[#3018](import-js/eslint-plugin-import#3018)], thanks \[[@michaelfaith](https://github.com/michaelfaith)]) ##### Fixed - \[`no-extraneous-dependencies`]: allow wrong path (\[[#3012](import-js/eslint-plugin-import#3012)], thanks \[[@chabb](https://github.com/chabb)]) - \[`no-cycle`]: use scc algorithm to optimize (\[[#2998](import-js/eslint-plugin-import#2998)], thanks \[[@soryy708](https://github.com/soryy708)]) - \[`no-duplicates`]: Removing duplicates breaks in TypeScript (\[[#3033](import-js/eslint-plugin-import#3033)], thanks \[[@yesl-kim](https://github.com/yesl-kim)]) - \[`newline-after-import`]: fix considerComments option when require (\[[#2952](import-js/eslint-plugin-import#2952)], thanks \[[@developer-bandi](https://github.com/developer-bandi)]) - \[`order`]: do not compare first path segment for relative paths (\[[#2682](import-js/eslint-plugin-import#2682)]) (\[[#2885](import-js/eslint-plugin-import#2885)], thanks \[[@mihkeleidast](https://github.com/mihkeleidast)]) ##### Changed - \[Docs] `no-extraneous-dependencies`: Make glob pattern description more explicit (\[[#2944](import-js/eslint-plugin-import#2944)], thanks \[[@mulztob](https://github.com/mulztob)]) - \[`no-unused-modules`]: add console message to help debug \[[#2866](import-js/eslint-plugin-import#2866)] - \[Refactor] `ExportMap`: make procedures static instead of monkeypatching exportmap (\[[#2982](import-js/eslint-plugin-import#2982)], thanks \[[@soryy708](https://github.com/soryy708)]) - \[Refactor] `ExportMap`: separate ExportMap instance from its builder logic (\[[#2985](import-js/eslint-plugin-import#2985)], thanks \[[@soryy708](https://github.com/soryy708)]) - \[Docs] `order`: Add a quick note on how unbound imports and --fix (\[[#2640](import-js/eslint-plugin-import#2640)], thanks \[[@MinervaBot](https://github.com/minervabot)]) - \[Tests] appveyor -> GHA (run tests on Windows in both pwsh and WSL + Ubuntu) (\[[#2987](import-js/eslint-plugin-import#2987)], thanks \[[@joeyguerra](https://github.com/joeyguerra)]) - \[actions] migrate OSX tests to GHA (\[[ljharb#37](ljharb/eslint-plugin-import#37)], thanks \[[@aks-](https://github.com/aks-)]) - \[Refactor] `exportMapBuilder`: avoid hoisting (\[[#2989](import-js/eslint-plugin-import#2989)], thanks \[[@soryy708](https://github.com/soryy708)]) - \[Refactor] `ExportMap`: extract "builder" logic to separate files (\[[#2991](import-js/eslint-plugin-import#2991)], thanks \[[@soryy708](https://github.com/soryy708)]) - \[Docs] \[`order`]: update the description of the `pathGroupsExcludedImportTypes` option (\[[#3036](import-js/eslint-plugin-import#3036)], thanks \[[@liby](https://github.com/liby)]) - \[readme] Clarify how to install the plugin (\[[#2993](import-js/eslint-plugin-import#2993)], thanks \[[@jwbth](https://github.com/jwbth)])

[New] [Refactor] no-cycle: use scc algorithm to optimize; add `skip…

98a0991

…ErrorMessagePath` for faster error messages

This comment was marked as resolved.

Sign in to view

ljharb reviewed Apr 8, 2024

View reviewed changes

src/rules/no-cycle.js Outdated Show resolved Hide resolved

tests/src/rules/no-cycle.js Outdated Show resolved Hide resolved

ljharb reviewed Apr 23, 2024

View reviewed changes

src/scc.js Outdated Show resolved Hide resolved

src/scc.js Show resolved Hide resolved

tests/src/rules/no-cycle.js Outdated Show resolved Hide resolved

tests/src/rules/no-cycle.js Outdated Show resolved Hide resolved

kuoruan reviewed Apr 24, 2024

View reviewed changes

src/rules/no-cycle.js Outdated Show resolved Hide resolved

ljharb reviewed Apr 24, 2024

View reviewed changes

src/scc.js Show resolved Hide resolved

tests/src/rules/no-cycle.js Outdated Show resolved Hide resolved

tests/src/rules/no-cycle.js Outdated Show resolved Hide resolved

ljharb force-pushed the leverage-scc-in-no-cycle branch from f64a123 to a47b5b9 Compare May 2, 2024 05:48

ljharb added the semver-minor label May 2, 2024

ljharb mentioned this pull request May 15, 2024

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

Closed

Janpot mentioned this pull request Jun 26, 2024

[code-infra] Refactor eslint import/no-cycle rule mui/material-ui#42705

Merged

SukkaW mentioned this pull request Jul 11, 2024

perf: improve rule no-cycle using strongly connected components un-ts/eslint-plugin-import-x#111

Merged

ljharb force-pushed the leverage-scc-in-no-cycle branch from a47b5b9 to 580c241 Compare July 29, 2024 19:50

This comment was marked as resolved.

Sign in to view

soryy708 marked this pull request as ready for review August 23, 2024 19:48

ljharb approved these changes Aug 24, 2024

View reviewed changes

ljharb reviewed Aug 24, 2024

View reviewed changes

src/scc.js Show resolved Hide resolved

ljharb force-pushed the leverage-scc-in-no-cycle branch from 55c2741 to 98cbfc0 Compare August 28, 2024 19:33

ljharb reviewed Aug 28, 2024

View reviewed changes

src/rules/no-cycle.js Outdated Show resolved Hide resolved

ljharb approved these changes Aug 28, 2024

View reviewed changes

ljharb force-pushed the leverage-scc-in-no-cycle branch from 9060fc0 to 98a0991 Compare August 28, 2024 21:40

ljharb merged commit 98a0991 into import-js:main Aug 29, 2024
307 checks passed

SukkaW mentioned this pull request Sep 4, 2024

[3.0.1] Huge performance regression in import/no-cycle un-ts/eslint-plugin-import-x#113

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optimize no-cycle rule using strongly connected components #2998

optimize no-cycle rule using strongly connected components #2998

soryy708 commented Apr 8, 2024 •

edited

Loading

This comment was marked as resolved.

socket-security bot commented Apr 8, 2024 •

edited

Loading

socket-security bot commented Apr 8, 2024 •

edited

Loading

soryy708 commented Apr 8, 2024

ljharb commented Apr 8, 2024

codecov bot commented Apr 10, 2024 •

edited

Loading

ljharb left a comment

pumano commented May 20, 2024

jonahallibone commented May 29, 2024

pumano commented Jun 10, 2024

This comment was marked as resolved.

soryy708 commented Aug 23, 2024

ljharb left a comment

soryy708 commented Aug 24, 2024

soryy708 commented Aug 26, 2024

JounQin commented Aug 27, 2024

soryy708 commented Aug 27, 2024

SukkaW commented Aug 27, 2024

ljharb left a comment

soryy708 commented Sep 3, 2024

optimize no-cycle rule using strongly connected components #2998

optimize no-cycle rule using strongly connected components #2998

Conversation

soryy708 commented Apr 8, 2024 • edited Loading

This comment was marked as resolved.

socket-security bot commented Apr 8, 2024 • edited Loading

socket-security bot commented Apr 8, 2024 • edited Loading

soryy708 commented Apr 8, 2024

ljharb commented Apr 8, 2024

codecov bot commented Apr 10, 2024 • edited Loading

Codecov Report

ljharb left a comment

Choose a reason for hiding this comment

pumano commented May 20, 2024

jonahallibone commented May 29, 2024

pumano commented Jun 10, 2024

This comment was marked as resolved.

soryy708 commented Aug 23, 2024

ljharb left a comment

Choose a reason for hiding this comment

soryy708 commented Aug 24, 2024

soryy708 commented Aug 26, 2024

JounQin commented Aug 27, 2024

soryy708 commented Aug 27, 2024

SukkaW commented Aug 27, 2024

ljharb left a comment

Choose a reason for hiding this comment

soryy708 commented Sep 3, 2024

soryy708 commented Apr 8, 2024 •

edited

Loading

socket-security bot commented Apr 8, 2024 •

edited

Loading

socket-security bot commented Apr 8, 2024 •

edited

Loading

codecov bot commented Apr 10, 2024 •

edited

Loading