-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Optimize module dependency graph generation #2053
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
base: main
Are you sure you want to change the base?
Optimize module dependency graph generation #2053
Conversation
Summary of ChangesHello @minjun011026, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on improving the performance of the module dependency graph generation. The core change involves refactoring the sorting mechanism to pre-calculate incoming dependency counts, thereby eliminating redundant computations during the sorting process and leading to a more efficient and faster graph construction. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant performance optimization for the module dependency graph generation. By pre-calculating the incoming dependency counts, the sorting logic is made much more efficient, which is a great improvement. The implementation is correct and achieves the stated goal. I have one suggestion to further improve the code's readability and maintainability by reducing code repetition.
| val groupIncomingCounts = dependencies.groupBy { it.dependency.substringBeforeLast(":") } | ||
| .mapValues { (group, groupDependencies) -> | ||
| groupDependencies.count { dep -> dep.project.substringBeforeLast(":") != group } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pre-calculation is a great optimization. To further improve readability and reduce repetition, consider extracting the substringBeforeLast(":") logic into a helper. This logic to get a module's group is used multiple times here and elsewhere in the function.
For example, you could define a private extension property on String:
private val String.moduleGroup: String get() = substringBeforeLast(":")
// Then use it like this:
val groupIncomingCounts = dependencies.groupBy { it.dependency.moduleGroup }
.mapValues { (group, groupDependencies) ->
groupDependencies.count { dep -> dep.project.moduleGroup != group }
}This would make the code more declarative and easier to maintain. Since this change would be outside the modified lines, I'm leaving it as a suggestion for a potential follow-up.
What I have done and why
Optimized the sorting logic within the graph generation.
Previously, it performed a full scan of the dependency list for every comparison during sorting$O(N log N * D)$ . It now pre-calculates incoming dependency counts into a Map, reducing the lookup time to $O(1)$ .
(N : Number of InnerGroups, D : Number of Dependencies)