[MNG-8696] Hide the cache from DefaultDependencyResolverResult constructor #2347

desruisseaux · 2025-05-18T17:08:06Z

This is an alternative to pull request #2219 resolving the same issue, but with more emphasis that PathModularizationCache should not be created in the DefaultDependencyResolverResult constructor, and that doing so is a temporary hack that may be removed in a future version. More specifically:

Make package-private the DefaultDependencyResolverResult constructor having a PathModularizationCache argument, because the latter is package-private.
Add a public constructor without the cache argument for codes in other packages that need to instantiate DefaultDependencyResolverResult directly.
- Put a warning in the javadoc saying that this constructor may be removed in a future version.
Add PathModularizationCache private field in DefaultDependencyResolver. This is partially for performance reasons (see below), but also for expressing the intend that the cache has a longer lifetime than DependencyResolverResult.

Rational

Initializing PathModularizationCache inside the DependencyResolverResult constructor is close to useless, because a cache is useful only when the cached values are reused. The way that the DependencyResolverResult is coded, the same result instance is unlikely to ask the same cached value twice. The cache become useful only when many DependencyResolverResult are instantiated while reusing the same cache.

The "useless" approach is nevertheless used by DependencyResolverResult, but this is only because I do not yet know how to manage a session-wide cache. The current way to create the cache was intended to be temporary.

I'm not sure if it happens often that the same DependencyResolverResultBuilder is reused for creating more than one DependencyResolverResult. If not, moving the cache inside DependencyResolverResultBuilder, as done in this commit, may not bring real performance improvement. However, it makes clearer that PathModularizationCache is expected to have a longer lifetime than DependencyResolverResult. The discussion in #2219 gives the impression that such clarification is worth to do.

…having a PathModularizationCache argument, and add a public constructor without the cache argument for code in other packages that need to instantiate. The public constructor may be temporary, until we decide in the future where to store session-wide cache.

Pankraz76

spelling, potential static inline.

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java

Pankraz76 · 2025-05-18T17:46:18Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java

+    /**
+     * Creates an initially empty resolver.
+     */
+    public DefaultDependencyResolver() {}


who need this? will create very fragile object.

This constructor was already there before, only implicit (when a Java file does not declare any constructor at all). This addition only makes it explicit.

yes right, thanks. But is in use? Encounter compile issues, therefore created and sure about usage?
Might not needed before. Want to make sure.

I think that it is instantiated via reflection, as the class is annotated with @Named and @Singleton from the org.apache.maven.api.di package.

test

delete, rebuild, pass.

It was done there before and it is without we having to make up whats already there.

I'm not sure which change is requested here. Is it to remove the constructor? It was added only for making explicit what was implicit before. So this is mostly for documentation purpose.

Demonstration

Save the following file somewhere:

/** * A dummy comment. */ public class Test { /** * Another dummy comment. */ public Test() { } }

Compile as below with a recent Java version (tested with Java 24). The -Xdoclint option is for having warnings about the documentation. The -g:none option is for omitting debugging information, because the line numbers will change in this demo.

javac -g:none -Xdoclint:all Test.java

Compilation succeed with no warning. Save the output file:

mv Test.class Test.bak

Remove the constructor (including its comment) and recompile with the same command. Note that we now get the following warning:

Test.java:4: warning: use of default constructor, which does not provide a comment public class Test { ^ 1 warning

Compare the output files:

diff Test.bak Test.class

They are identical. Making the implicit constructor explicit changes nothing to the compiled code, but is nevertheless recommended according above warning.

It was added only for making explicit what was implicit before

yes we on the same page. Why we make this? Only if we have 1 constructor that is NON default, only then this change is required, to provide implicit default again. This is not true yet, therefore its "boilerplate" as its there, with or without.

This is a change we can see on compiler lvl, or if rare case we use hibernate having the need for non args constructor like in. https://stackoverflow.com/questions/2935826/why-does-hibernate-require-no-argument-then we see this issue on test lvl.

Please provide stacktrace or reproducer proving this code in mandatory by failing build. Assuming removing the constructor will turn out the same.

Yes provided sorry. Then lets merge and local build should fail when removed, right?
Then we simply know riving removal PR. it must fail build.

Then lets merge and local build should fail when removed, right?

We could make the build fail when the explicit constructor is absent by adding the -Werror compiler options. But Maven is not ready for that yet. For now, this change is just resolving one documentation warning among the many the Maven would have if the -Xdoclint:all option was provided.

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java

Pankraz76 · 2025-05-18T17:48:39Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java

+     */
+    public DefaultDependencyResolverResult(
+            DependencyResolverRequest request, List<Exception> exceptions, Node root, int count) {
+        this(request, new PathModularizationCache(), exceptions, root, count);


yes this gives dedication and option.

Suggested change

this(request, new PathModularizationCache(), exceptions, root, count);

this(request, moduleCache(), exceptions, root, count);

The moduleCache() method is defined in another class. We cannot access that method from here.

Pankraz76 · 2025-05-18T17:50:28Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java

@@ -126,7 +141,11 @@ public DependencyResolverResult collect(@Nonnull DependencyResolverRequest reque
                final CollectResult result =
                        session.getRepositorySystem().collectDependencies(systemSession, collectRequest);
                return new DefaultDependencyResolverResult(
-                        null, null, result.getExceptions(), session.getNode(result.getRoot(), request.getVerbose()), 0);
+                        null,
+                        moduleCache(),


as its dry, and static, inline. Ide shoud give warning on method or constructor that value is always same.

I'm not sure to understand. Inline the call to moduleCache()? This is uneasy, as that method does lazy instantiation.

sorry wrong assumption. This is called every time as this the only return therefore default.

Pankraz76 · 2025-05-18T17:51:33Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java

            DependencyResolverRequest request,
            PathModularizationCache cache,
            List<Exception> exceptions,
            Node root,
            int count) {
        this.request = request;
-        this.cache = cache;
+        this.cache = Objects.requireNonNull(cache);


or here

Suggested change

this.cache = Objects.requireNonNull(cache);

this.cache = Objects.requireNonNull(moduleCache());

one of the suggestions is right. as moduleCache() is SSOT.

The moduleCache() method is defined in another class. We cannot access that method from here.

However, this is indeed the kind of code that we would have after we clarified how to manage session-wide caches. The moduleCache() method would be in the class managing those caches. We are just not there yet.

Fwiw, session wide informations can be stored in Session.getData().

Pankraz76

doc link.

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java

Pankraz76 · 2025-05-18T19:33:44Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java

                if (request.getRequestType() == DependencyResolverRequest.RequestType.FLATTEN) {
-                    DefaultDependencyResolverResult flattenResult = new DefaultDependencyResolverResult(


this scope was right. this field concern lives only here. Even could dedicate whole block.

The change was for removing code duplication. The exact same code was inside the two branches of the if … else statement, with only different variable names.

yes see.

Common part can be extracted from 'if'

its same obtain the warning:

@Override public DependencyResolverResult resolve(DependencyResolverRequest request) throws DependencyResolverException, ArtifactResolverException { InternalSession session = InternalSession.from(nonNull(request, "request").getSession()); try { DependencyResolverResult collectorResult = collect(request); DependencyResolverResult result = collectorResult; List<RemoteRepository> repositories = request.getRepositories() != null ? request.getRepositories() : request.getProject().isPresent() ? session.getService(ProjectManager.class) .getRemoteProjectRepositories( request.getProject().get()) : session.getRemoteRepositories(); if (request.getRequestType()!= DependencyResolverRequest.RequestType.COLLECT) { List<Node> nodes = flatten(session, collectorResult.getRoot(), request.getPathScope()); DefaultDependencyResolverResult resolverResult = new DefaultDependencyResolverResult( null, moduleCache(), collectorResult.getExceptions(), collectorResult.getRoot(), nodes.size()); if (request.getRequestType() == DependencyResolverRequest.RequestType.FLATTEN) { for (Node node : nodes) { resolverResult.addNode(node); } } else { for (Node node : nodes) { Path path = (node.getArtifact() != null) ? session.getService(ArtifactResolver.class).resolve(session, nodes.stream() .map(Node::getDependency) .filter(Objects::nonNull) .map(Artifact::toCoordinates) .collect(Collectors.toList()), repositories) .getResult(node.getArtifact().toCoordinates()) .getPath() : null; try { resolverResult.addDependency(node, node.getDependency(), request.getPathTypeFilter(), path); } catch (IOException e) { throw cannotReadModuleInfo(path, e); } } } result = resolverResult; } return result; } finally { RequestTraceHelper.exit(RequestTraceHelper.enter(session, request)); } }

is the finally an issue inlining exit ?

is the finally an issue inlining exit ?

Judging by the method names, it would probably not work. RequestTraceHelper.enter probably needs to be invoked before to start the execution of the code inside the try block. Inlining would cause enter to be invoked too late.

Pankraz76

quest.

Pankraz76 · 2025-05-18T20:32:46Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java

                if (request.getRequestType() == DependencyResolverRequest.RequestType.FLATTEN) {
-                    DefaultDependencyResolverResult flattenResult = new DefaultDependencyResolverResult(


yes see.

Common part can be extracted from 'if'

its same obtain the warning:

@Override public DependencyResolverResult resolve(DependencyResolverRequest request) throws DependencyResolverException, ArtifactResolverException { InternalSession session = InternalSession.from(nonNull(request, "request").getSession()); try { DependencyResolverResult collectorResult = collect(request); DependencyResolverResult result = collectorResult; List<RemoteRepository> repositories = request.getRepositories() != null ? request.getRepositories() : request.getProject().isPresent() ? session.getService(ProjectManager.class) .getRemoteProjectRepositories( request.getProject().get()) : session.getRemoteRepositories(); if (request.getRequestType()!= DependencyResolverRequest.RequestType.COLLECT) { List<Node> nodes = flatten(session, collectorResult.getRoot(), request.getPathScope()); DefaultDependencyResolverResult resolverResult = new DefaultDependencyResolverResult( null, moduleCache(), collectorResult.getExceptions(), collectorResult.getRoot(), nodes.size()); if (request.getRequestType() == DependencyResolverRequest.RequestType.FLATTEN) { for (Node node : nodes) { resolverResult.addNode(node); } } else { for (Node node : nodes) { Path path = (node.getArtifact() != null) ? session.getService(ArtifactResolver.class).resolve(session, nodes.stream() .map(Node::getDependency) .filter(Objects::nonNull) .map(Artifact::toCoordinates) .collect(Collectors.toList()), repositories) .getResult(node.getArtifact().toCoordinates()) .getPath() : null; try { resolverResult.addDependency(node, node.getDependency(), request.getPathTypeFilter(), path); } catch (IOException e) { throw cannotReadModuleInfo(path, e); } } } result = resolverResult; } return result; } finally { RequestTraceHelper.exit(RequestTraceHelper.enter(session, request)); } }

is the finally an issue inlining exit ?

Pankraz76 · 2025-05-18T20:35:43Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java

+     *
+     * @see #moduleCache()
+     */
+    private PathModularizationCache moduleCache;


except of static this is the same outcome.

Suggested change

private PathModularizationCache moduleCache;

private final static PathModularizationCache MODULE_CACHE = new PathModularizationCache();

Pankraz76 · 2025-05-18T20:36:13Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java

@@ -126,7 +141,11 @@ public DependencyResolverResult collect(@Nonnull DependencyResolverRequest reque
                final CollectResult result =
                        session.getRepositorySystem().collectDependencies(systemSession, collectRequest);
                return new DefaultDependencyResolverResult(
-                        null, null, result.getExceptions(), session.getNode(result.getRoot(), request.getVerbose()), 0);
+                        null,
+                        moduleCache(),


sorry wrong assumption. This is called every time as this the only return therefore default.

Pankraz76 · 2025-05-18T20:38:11Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java

+    /**
+     * {@return the cache of information about the modules contained in a path element}.
+     *
+     * <p><b>TODO:</b> This method should not be in this class, because the cache should be global to the session.
+     * This method exists here only temporarily, until clarified where to store session-wide caches.</p>
+     */
+    private PathModularizationCache moduleCache() {
+        if (moduleCache == null) {
+            moduleCache = new PathModularizationCache();
+        }
+        return moduleCache;
+    }
+


Suggested change

/**

* {@return the cache of information about the modules contained in a path element}.

*

* <p><b>TODO:</b> This method should not be in this class, because the cache should be global to the session.

* This method exists here only temporarily, until clarified where to store session-wide caches.</p>

*/

private PathModularizationCache moduleCache() {

if (moduleCache == null) {

moduleCache = new PathModularizationCache();

}

return moduleCache;

}

making field final is the same result. static would change to all classes change same cache. If no collision risk then might be possible.

A collision is possible if a JAR file changes between two executions. It may happen in particular if a JAR is the output of another project. It can also be a memory leak if a Maven process runs for a long time (the daemon?) and JAR paths change often (e.g. snapshot with timestamps). This is why the desired lifetime is a session (e.g. a call to mvn compile), with a fresh cache rebuilt every time that mvn compile is executed from the root of a multi-projects.

Pankraz76 · 2025-05-18T20:40:05Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java

@@ -68,6 +68,22 @@
 @Singleton
 public class DefaultDependencyResolver implements DependencyResolver {

+    /**
+     * Cache of information about the modules contained in a path element.
+     * This cache is created when first needed. It may be never created.


Suggested change

* This cache is created when first needed. It may be never created.

imho no, as always have return call, on default.

imho no, as always have return call, on default.

Right. Checking again, a see no code path where the cache would not be created. I will remove the lazy instantiation aspect.

gnodet · 2025-05-19T20:53:01Z

@desruisseaux for consistency and to honor the fact that the {{DepfaultDependencyResolverResult}} implements {{DependencyResolverResult}} which is annotated with {{@immutable}}, I'd rather have {{DepfaultDependencyResolverResult}} being immutable. Is there a way to use a temporary intermediate class to store the computation data, and only keep the final computed state in this class ?

desruisseaux · 2025-05-19T21:11:02Z

Is there a way to use a temporary intermediate class to store the computation data (…snip…)?

Yes, ideally the cache should be in some session-wide place. The only reason why the cache is declared in DefaultDependencyResolver is because I do not know if there is a central place in Maven 4 where we can put stuff that we would like to cache for the duration of the session.

Regarding DefaultDependencyResolverResult this class is conceptually unmodifiable. The add methods are package-private and used only a construction time. I will add Collections.unmodifiable(…) wrappers in the getter methods for safety.

- PathModularizationCache is always required, no need for lazy instantiation. - DefaultDependencyResolverResult returns immutable views of collections.

desruisseaux mentioned this pull request May 18, 2025

[MNG-8696] PathModularizationCache needs to set cache #2219

Open

Pankraz76 suggested changes May 18, 2025

View reviewed changes

Pankraz76 reviewed May 18, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java Outdated Show resolved Hide resolved

Pankraz76 reviewed May 18, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java Show resolved Hide resolved

Pankraz76 reviewed May 18, 2025

View reviewed changes

Spelling and formatting fixes from comments on the pull request.

f878a62

desruisseaux force-pushed the fix/PathModularizationCache-constructor branch from 56f483e to f878a62 Compare May 18, 2025 20:18

Pankraz76 reviewed May 18, 2025

View reviewed changes

Changes based on comments in the pull request:

8e773d5

- PathModularizationCache is always required, no need for lazy instantiation. - DefaultDependencyResolverResult returns immutable views of collections.

gnodet approved these changes Jun 12, 2025

View reviewed changes

gnodet added this to the 4.0.0 milestone Jun 18, 2025

	this(request, new PathModularizationCache(), exceptions, root, count);
	this(request, moduleCache(), exceptions, root, count);

	this.cache = Objects.requireNonNull(cache);
	this.cache = Objects.requireNonNull(moduleCache());

		if (request.getRequestType() == DependencyResolverRequest.RequestType.FLATTEN) {
		DefaultDependencyResolverResult flattenResult = new DefaultDependencyResolverResult(

	private PathModularizationCache moduleCache;
	private final static PathModularizationCache MODULE_CACHE = new PathModularizationCache();

[MNG-8696] Hide the cache from DefaultDependencyResolverResult constructor #2347

Are you sure you want to change the base?

[MNG-8696] Hide the cache from DefaultDependencyResolverResult constructor #2347

Conversation

desruisseaux commented May 18, 2025

Rational

Uh oh!

Pankraz76 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Demonstration

Uh oh!

Pankraz76 May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Pankraz76 May 18, 2025 •

edited

Loading

Pankraz76 May 19, 2025 •

edited

Loading

Pankraz76 May 18, 2025 •

edited

Loading

desruisseaux commented May 19, 2025 •

edited

Loading