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 transitive implementation #25

Merged

Conversation

camsaul
Copy link
Contributor

@camsaul camsaul commented May 29, 2019

Hey @weavejester,

I've noticed that lein-ring starts really slow for larger projects (for example it usually takes 15-20 minutes to launch Metabase via lein ring server, compared to ~1 with lein run).

I did some profiling and determined 98% of the launch time was spent in ns-tracker.dependency/transitive, mainly because it recursively calls transitive on everything before finally removing duplicates in seq-union. I changed this to an implementation that avoids recursive calls for dependencies that have already been dealt with, which has pretty dramatic performance improvements.

To test this I captured a real-world graph of ~200 namespaces from Metabase during launch and compared the old and new implementations. The namespaces graph and code I used to compare implementations is here if you'd like to see for yourself. When testing locally, the new implementation is around 1000x faster:

Implementation Time
Old 873.714 ms
New 0.754 ms

I can also confirm that it has fixed the slow launch time issue I was running into.

The order of results in the new implementation are consistently breadth-first whereas results in the old implementation are mostly breadth-first but, not consistently so. I put an example comparison between the orders returned by the implementations here. As far as I can tell the slight order differences has no effect on the reloading code that relies on it.

@weavejester
Copy link
Owner

Thanks for the improvement! Can you change the commit message to:

Speed up transitive function

@camsaul camsaul force-pushed the faster-transitive-implementation branch from 162c3f6 to 14df04d Compare May 31, 2019 18:36
@camsaul
Copy link
Contributor Author

camsaul commented May 31, 2019

@weavejester done

@weavejester weavejester merged commit ee9d300 into weavejester:master Jun 1, 2019
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.

2 participants