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

Cache and reuse resolution results #3931

Merged
merged 5 commits into from Jan 5, 2022
Merged

Cache and reuse resolution results #3931

merged 5 commits into from Jan 5, 2022

Conversation

aikebah
Copy link
Collaborator

@aikebah aikebah commented Dec 30, 2021

Description of Change

Cache resolution results for all dependencies and reuse that instead of doing (un)filtered resolutions dependency-by-dependency. Fixes #3923

Have test cases been added to cover the new functionality?

no, functionality was refactored

@boring-cyborg boring-cyborg bot added the maven changes to the maven plugin label Dec 30, 2021
@aikebah
Copy link
Collaborator Author

aikebah commented Dec 31, 2021

@jeremylong a good step forward. But definitely needs more work as it currently creates duplicate dependencies on aggregate-scanning a large multimodule project

@aikebah
Copy link
Collaborator Author

aikebah commented Jan 2, 2022

@jeremylong Differences in results can be explained by a failure to resolve the reactor-projects in the new code plus existing behaviour that virtual dependencies are not merge together as a single one.

If you're working towards a 6.5.2 release feel free to already promote this from draft to normal and merge it. I will try to find out why the resolution did work in the 6.5.1 case and doesn't work in the 6.5.2-SNAPSHOT case from this branch, but the difference I spotted is in my view sufficiently explainable to warrant inclusion of this change in a next fixpack in order to solve the massive slowdown.

The project I used had 10 inter-module-dependencies, which were 10 resolved dependencies in 6.5.1. In 6.5.2-SNAPSHOT they become unresolved, triggering virtual dependency creation. And as some of those inter-module dependencies occur for multiple dependencies it results in a total of 24 inter-module dependencies being listed for what actually is only 10 inter-module dependencies.

That multiplication of virtual dependencies also happens in 6.5.1 if a reactor-dependency in a project is not yet resolvable.

Not resolving the (existing) release versions of the reactor-dependencies I consider a regression of this change. Duplicating the virtual dependencies I will file a separate GH issue once I've managed to tackle this regression.
If you decide to merge this already in order to put out a 6.5.2 the regression could be handled under a new GH issue.

@aikebah
Copy link
Collaborator Author

aikebah commented Jan 2, 2022

@jeremylong already found the pattern when it fails: when the artifact searched for uses a classifier. So think it's better to get this resolved before merging.

@aikebah
Copy link
Collaborator Author

aikebah commented Jan 2, 2022

@jeremylong already found the pattern when it fails: when the artifact searched for uses a classifier. So think it's better to get this resolved before merging.

Minor correction, not the classifier, but a non-jar packaging (ejb)

@aikebah aikebah marked this pull request as ready for review January 2, 2022 16:22
@aikebah aikebah added this to the 6.5.2 milestone Jan 2, 2022
@aikebah aikebah changed the title Attempt fixing Issue 3923 by caching and reusing resolution results Cache and reuse resolution results Jan 2, 2022
@jeremylong
Copy link
Owner

I feel like we need to release 6.5.2. We can publish another release as soon as the regressions are solved.

@jeremylong jeremylong modified the milestones: 6.5.2, 6.5.3 Jan 3, 2022
@aikebah
Copy link
Collaborator Author

aikebah commented Jan 3, 2022

I feel like we need to release 6.5.2. We can publish another release as soon as the regressions are solved.

Sorry I hadn't commented that the regression was resolved by my last commit. Could've gone into the 6.5.2. But I propose to hold back 6.5.3 until the duplication of virtual dependencies (#3944) also gets resolved as well as I intend to look at that tonight.

@aikebah
Copy link
Collaborator Author

aikebah commented Jan 3, 2022

@jeremylong Fix for #3944 is in its final testing stage and will get its own branch and PR

@jeremylong jeremylong merged commit 40630a1 into main Jan 5, 2022
@jeremylong jeremylong deleted the issue-3923 branch January 5, 2022 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maven changes to the maven plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Huge performance degradation after upgrade dependency-check-maven from 6.3.1 to 6.4.x / 6.5.x
2 participants