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

Replace cycle detection algorithm with Tarjan's strongly connected components algorithm #49

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hedgepigdaniel
Copy link
Contributor

@hedgepigdaniel hedgepigdaniel commented Sep 19, 2019

I noticed that this plugin was taking significant time (almost a second for a 5500 module project) to check for cycles in hot rebuilds. Tarjan's strongly connected components algorithm as I understand is the fastest way to find dependency cycles. Its running time is linear in the number of vertices (modules) + edges (dependencies).

My best guess is that the existing algorithm is quadratic in the number of modules, because for each module in the compilation, all its dependencies are traversed recursively, which in the worst case also visits each module in the compilation.

I haven't done benchmarks, but in the same project after this change, this plugin is no longer perceptible in the ProgressPlugin output.

I also changed the output:

  • One cycle is output only once, not separately for each file it contains
  • For cycles when some modules in the cycle are excluded - it now outputs all modules in the cycle even if some are excluded. This makes more sense IMO, since otherwise it prints e.g. a.js -> b.js even if a.js does not directly import b.js, which is confusing. If all modules in a cycle are excluded, no warning is generated (I added a test to enforce this)

@aackerman
Copy link
Owner

aackerman commented Sep 19, 2019

Thanks for sending this, I appreciate the effort that went into this. Since this reworks a lot of the core functionality I'll have to spend some time reviewing it, and I probably won't get to it until the weekend, I'll try to do a thorough review as soon as I can.

@sepehr
Copy link

sepehr commented Jun 10, 2020

This is a great contribution. Wish we had it merged.

@TeoTN
Copy link

TeoTN commented Sep 22, 2020

Was it benchmarked maybe?

@amakhrov
Copy link

amakhrov commented Feb 8, 2021

Any update on that?
I would so appreciate any improvements to the plugin performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants