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

[MRESOLVER-587] Memory usage improvements #536

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Jul 13, 2024

Do not use Object as descriptor key, use dedicated type. And intern the List<Dependency> on artifact descriptors. Also introduce "intern"-ing of ArtifactDescriptor dependencies and managedDependencies that is configurable (speed vs memory).


https://issues.apache.org/jira/browse/MRESOLVER-587

@cstamas
Copy link
Member Author

cstamas commented Jul 13, 2024

This PR causes noticeable slow down: building Quarkus (mvn clean install -Dquickly) went from 9:20 to 11:10.

Ideas:

  • make interning of dependencies configurable, default NO
  • make interning of dependencyManagement configurable, default YES
  • maybe intern only over some size limit?

The most offender is huge count of depMgt inherited from parent POM, so this above would make sensible defaults.

Do not use Artifact instance as keys, and intern the
List<Dependency> on artifact descriptors as well.

---

https://issues.apache.org/jira/browse/MRESOLVER-587
@cstamas cstamas force-pushed the maven-resolver-1.9.x-MRESOLVER-587 branch from 434a5f0 to 3348e42 Compare August 2, 2024 10:01
…efore)

Adds two new config properties that controls interning of
ArtifactDescriptor dependencies and managedDependencies.

Interning both radically lower memory consumption but at
same time increases runtime,
@cstamas
Copy link
Member Author

cstamas commented Aug 2, 2024

@gsmet this PR now has it all merged changes, plus, introduces config (defaults to "as before" -- false both) to intern or not intern the artifact descriptor deps list and managedDeps lists.

Am tinkering, that maybe feasible default would be false/true (do not intern deps, do intern managedDeps)?

@cstamas cstamas added this to the 2.0.1 milestone Aug 2, 2024
@gsmet
Copy link
Contributor

gsmet commented Aug 2, 2024

Am tinkering, that maybe feasible default would be false/true (do not intern deps, do intern managedDeps)?

It looks like a good compromise from my side.

Intern only managed deps.

Also simplify a bit.
@@ -257,7 +257,7 @@ public static final class DescriptorKey {

private DescriptorKey(Artifact artifact) {
this.artifact = artifact;
this.hashCode = buildHashCode();
this.hashCode = Objects.hashCode(artifact);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah sorry, I was used to more complex hashCode construction and kept this pattern (we avoid using Objects.hash as it allocates an array and can be problematic in some cases so we end up with traditional boilerplate hashCodes).

@cstamas cstamas merged commit cdc68ea into apache:maven-resolver-1.9.x Aug 2, 2024
11 checks passed
@cstamas cstamas deleted the maven-resolver-1.9.x-MRESOLVER-587 branch August 2, 2024 12:03
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