optimize no-cycle rule using strongly connected components by soryy708 · Pull Request #2998 · import-js/eslint-plugin-import (original) (raw)
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))
jeniasaigak and alecmev reacted with thumbs up emoji amq, christophehurpeau, martin-mindflow, and Vardiak reacted with hooray emoji stevoland, pumano, Vardiak, asfaltboy, PedroLaRosa, and platypii reacted with heart emoji YuerLee, Vardiak, connor-baer, and Mesqalito reacted with rocket emoji
…ErrorMessagePath` for faster error messages
This comment was marked as resolved.
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)
👍 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.
Good question. I think it's that this is the first new PR it's been run on since i installed it?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I'll look at failing tests next
would love to see that optimization!
Can't wait for this to be merged!
This comment was marked as resolved.
There's a pathology with SCC right now:
- It traverses ignored modules (isExternalModule), which slows it down
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see two uncovered lines - can we add test cases to cover them?
Otherwise, this LGTM assuming all existing tests pass!
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.
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.
Notice that we discovered performance downgrade at un-ts/eslint-plugin-import-x#113 when we ported this "optimization", I don't quite sure whether there were some improvements after we ported.
cc @SukkaW
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.
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 @SukkaWThanks, 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.
This was referenced
Sep 26, 2024
This was referenced
Oct 3, 2024
This was referenced
Oct 4, 2024
This was referenced
Oct 28, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})