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

Improve performances of Bloop/install #4600

Merged

Conversation

Baccata
Copy link
Contributor

@Baccata Baccata commented Feb 20, 2025

Prevents having to iteratively querying the location of each artifact's resolved files. This makes a big difference in large mill projects.

Prevents having to iteratively querying the location of each artifact's
resolved files. This makes a big difference in large mill projects.
@Baccata Baccata marked this pull request as ready for review February 20, 2025 16:42
@lolgab
Copy link
Member

lolgab commented Feb 20, 2025

Does this close #4469 ?
If not it seems for sure related.

@lihaoyi
Copy link
Member

lihaoyi commented Feb 21, 2025

CC @alexarchambault and @lefou , not sure if you have any opinions on this since you know more about IDE stuff than I do. Otherwise it looks good to me and hopefully the existing tests catch any bugs

@Baccata
Copy link
Contributor Author

Baccata commented Feb 21, 2025

Does this close #4469 ?

I'm honestly not sure, but I've tried this change on a large mill project that was certainly experiencing similar symptoms, and it made it better.

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me.

@lihaoyi lihaoyi merged commit 601c5ae into com-lihaoyi:main Feb 21, 2025
40 checks passed
@Baccata Baccata deleted the baccata/more-performant-bloop-config branch February 21, 2025 12:44
Copy link
Collaborator

@alexarchambault alexarchambault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This switches that bit of code from the v1 to the v2 API of coursier, which is nice. The former code was apparently running the resolution twice, so that's better now 👍

@Baccata
Copy link
Contributor Author

Baccata commented Feb 21, 2025

Arguably it could be made event better if the task graph of java/scala modules were keeping the coursier dependencies around alongside the paths to the downloaded artifacts ... but 🤷‍♂️

@alexarchambault
Copy link
Collaborator

Arguably it could be made event better if the task graph of java/scala modules were keeping the coursier dependencies around alongside the paths to the downloaded artifacts ... but 🤷‍♂️

#4608 adds helpers to get artifact details without calling too directly the coursier API, that could help here.

We could try adding tasks keeping all those details, but I fear these might be quite heavyweight 🤔

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.

5 participants