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

Faster is acyclic test #66

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

Conversation

vbraun
Copy link

@vbraun vbraun commented Jan 15, 2021

This branch implements the fast acyclic test based on the depth first search. If there are cycles then the old algorithm is used, in particular the output stays the same.

In my project (5000+ webpack modules) this speeds up the circular dependency check from about 4300ms to 30ms (of course only if there are no cycles, if there are then nothing changes)

FIxes #65

@amakhrov
Copy link

amakhrov commented Feb 8, 2021

This should work great if a particular project enforces no cycles (e.g. via failOnError plugin setting, or via other measures).
However, if it's not generally enforced, a project would tend to have one or two "well-known" circular dependencies. In this case the extra isAcyclic check before the actual cycles enumerating would probably add some overhead? I would assume it's somewhat negligible (given its O(N) complexity) - but any thoughts on that? @vbraun

@lexanth
Copy link

lexanth commented Feb 9, 2021

If the overhead is too great (not saying it necessarily is), looks like making the "passQuickly" optional would be reasonably easy?

  if (passQuickly && isAcyclic(graph))
    return;

Would be great to get this in - my team is concerned about adding this plugin because of performance (we'd have failOnError on, so would expect failure to be the unusual case).

@vbraun
Copy link
Author

vbraun commented Feb 10, 2021

Well in my experience the overhead (in case of cycles present) is <1%, so I'd call than negligible.

If you have cycles that you are fine with then you really need a setting that somehow excludes that cycle from error reporting. Though I'd argue that your time would be much better spent breaking that cycle than figuring out how to configure webpack to allow a particular circular dependency.

@Knagis
Copy link

Knagis commented May 26, 2021

On webpack 5 this branch shows the following warning:

 [DEP_WEBPACK_DEPRECATION_ARRAY_TO_SET] DeprecationWarning: Compilation.modules was changed from Array to Set (using Array method 'filter' is deprecated)

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.

Circular dependency checker can be slow
4 participants