gnodet-bot commented on code in PR #2132:
URL: https://github.com/apache/maven-resolver/pull/2132#discussion_r4114456788
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/ConflictResolver.java:
##########
@@ -254,18 +255,76 @@ public DependencyNode transformGraph(DependencyNode node,
DependencyGraphTransfo
throws RepositoryException {
String cf = ConfigUtils.getString(
context.getSession(), DEFAULT_CONFLICT_RESOLVER_IMPL,
CONFIG_PROP_CONFLICT_RESOLVER_IMPL);
+
ConflictResolver delegate;
- if (AUTO_CONFLICT_RESOLVER.equals(cf) ||
CLASSIC_CONFLICT_RESOLVER.equals(cf)) {
+
+ if (AUTO_CONFLICT_RESOLVER.equals(cf)) {
+ delegate = selectConflictResolver(node, context);
+ } else if (CLASSIC_CONFLICT_RESOLVER.equals(cf)) {
delegate = new ClassicConflictResolver(versionSelector,
scopeSelector, optionalitySelector, scopeDeriver);
} else if (PATH_CONFLICT_RESOLVER.equals(cf)) {
delegate = new PathConflictResolver(versionSelector,
scopeSelector, optionalitySelector, scopeDeriver);
} else {
throw new IllegalArgumentException("Unknown conflict resolver: " +
cf + "; known are "
+ Arrays.asList(AUTO_CONFLICT_RESOLVER,
PATH_CONFLICT_RESOLVER, CLASSIC_CONFLICT_RESOLVER));
}
+
return delegate.transformGraph(node, context);
}
+ /**
+ * Automatically selects the conflict resolver based on the estimated
memory requirements.
+ * PathConflictResolver is used for dependency trees that fit within the
memory threshold,
+ * while ClassicConflictResolver is used for larger trees.
+ */
+ private ConflictResolver selectConflictResolver(DependencyNode node,
DependencyGraphTransformationContext context)
+ throws RepositoryException {
+
+ if (context.get(TransformationContextKeys.SORTED_CONFLICT_IDS) ==
null) {
+ new ConflictIdSorter().transformGraph(node, context);
+ }
+
+ Runtime rt = Runtime.getRuntime();
+ long available = rt.maxMemory() - (rt.totalMemory() - rt.freeMemory());
+
+ // Estimate the maximum number of Path tree nodes that would fit in
25% of available heap.
+ // Each Path object costs ~200 bytes (object header + fields +
children list entry).
+ int maxPathNodes = (int) Math.min(available / (4L * 200),
Integer.MAX_VALUE);
Review Comment:
⚠️ **Still non-deterministic (not addressed from previous review).**
`Runtime.freeMemory()` reflects GC state, allocation pressure from other
threads, and JVM ergonomics. The same dependency graph can get
`PathConflictResolver` on one invocation and `ClassicConflictResolver` on the
next.
Moreover, since PR #2153 merged, `PathConflictResolver` itself uses
depth-bounded expansion (`IdentityHashMap<DependencyNode, Integer>`) to cap
`Path` creation. The OOM that motivated this heuristic is fixed at the source.
This entire memory-estimation block is now solving a problem that no longer
exists.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/ConflictResolver.java:
##########
@@ -119,8 +120,8 @@ public class ConflictResolver implements
DependencyGraphTransformer {
/**
* The name of the conflict resolver implementation to use: "auto"
(default), "path", or "classic" (same as Maven 3).
* <p>
- * When set to "auto", the resolver will currently just use "classic". The
idea here, is that this value will
- * always select the best (most robust, most performant) one, which
currently is "classic".
+ * When set to "auto", the resolver automatically selects the most
appropriate implementation based on the
+ * dependency graph size and available memory.
Review Comment:
💡 **Javadoc contradiction not addressed (from previous review).** This
property-level doc claims auto mode "automatically selects the most appropriate
implementation," but the class-level Javadoc at lines 47 and 52-53 still says:
- "PathConflictResolver — Not yet recommended for production"
- "All projects: Use ClassicConflictResolver for optimal correctness"
These are contradictory. If auto mode can select PathConflictResolver, the
class-level docs need updating too — or this Javadoc should stay conservative
until the class docs are revised.
##########
maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/transformer/ConflictResolverTest.java:
##########
@@ -905,29 +929,24 @@ private static DependencyNode makeDependencyNode(
@ParameterizedTest
@MethodSource("conflictResolverSource")
void denseGraphDoesNotOom(ConflictResolver conflictResolver) throws
RepositoryException {
- // M=N=K=100: without the fix, creates M*N + M*N*K = 1,010,000 Path
objects (~80 MB)
- // → OutOfMemoryError on CI. With the fix: O(N+K) = 200 expansions,
trivial memory.
- int M = 100; // parent modules
- int N = 100; // hub modules (shared, each with children)
- int K = 100; // sub-hub leaf modules (shared across hubs)
+ int M = 100;
Review Comment:
⚠️ **Don't strip documentation comments from existing tests.** These
comments (added by PR #2153) explain the test structure — e.g., `M=N=K=100:
without the fix, creates M*N + M*N*K = 1,010,000 Path objects (~80 MB)` and the
purpose of each node group. Removing them makes the test harder to understand
for future contributors. Please revert the comment removals from
`denseGraphDoesNotOom`, `autoSelectionResolvesConflictsCorrectly`, and
`defaultConfigUsesAutoSelection`.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/ConflictResolver.java:
##########
@@ -254,18 +255,76 @@ public DependencyNode transformGraph(DependencyNode node,
DependencyGraphTransfo
throws RepositoryException {
String cf = ConfigUtils.getString(
context.getSession(), DEFAULT_CONFLICT_RESOLVER_IMPL,
CONFIG_PROP_CONFLICT_RESOLVER_IMPL);
+
ConflictResolver delegate;
- if (AUTO_CONFLICT_RESOLVER.equals(cf) ||
CLASSIC_CONFLICT_RESOLVER.equals(cf)) {
+
+ if (AUTO_CONFLICT_RESOLVER.equals(cf)) {
+ delegate = selectConflictResolver(node, context);
+ } else if (CLASSIC_CONFLICT_RESOLVER.equals(cf)) {
delegate = new ClassicConflictResolver(versionSelector,
scopeSelector, optionalitySelector, scopeDeriver);
} else if (PATH_CONFLICT_RESOLVER.equals(cf)) {
delegate = new PathConflictResolver(versionSelector,
scopeSelector, optionalitySelector, scopeDeriver);
} else {
throw new IllegalArgumentException("Unknown conflict resolver: " +
cf + "; known are "
+ Arrays.asList(AUTO_CONFLICT_RESOLVER,
PATH_CONFLICT_RESOLVER, CLASSIC_CONFLICT_RESOLVER));
}
+
return delegate.transformGraph(node, context);
}
+ /**
+ * Automatically selects the conflict resolver based on the estimated
memory requirements.
+ * PathConflictResolver is used for dependency trees that fit within the
memory threshold,
+ * while ClassicConflictResolver is used for larger trees.
+ */
+ private ConflictResolver selectConflictResolver(DependencyNode node,
DependencyGraphTransformationContext context)
+ throws RepositoryException {
+
+ if (context.get(TransformationContextKeys.SORTED_CONFLICT_IDS) ==
null) {
+ new ConflictIdSorter().transformGraph(node, context);
+ }
+
+ Runtime rt = Runtime.getRuntime();
+ long available = rt.maxMemory() - (rt.totalMemory() - rt.freeMemory());
+
+ // Estimate the maximum number of Path tree nodes that would fit in
25% of available heap.
+ // Each Path object costs ~200 bytes (object header + fields +
children list entry).
+ int maxPathNodes = (int) Math.min(available / (4L * 200),
Integer.MAX_VALUE);
+
+ // Walk the dependency tree to count total nodes (including
diamond-expanded duplicates).
+ // The Path tree mirrors this structure, so the count directly
reflects Path tree size.
+ // Use early-exit: stop counting once we exceed the threshold.
+ if (treeExceedsThreshold(node, maxPathNodes)) {
+ return new ClassicConflictResolver(versionSelector, scopeSelector,
optionalitySelector, scopeDeriver);
+ } else {
+ return new PathConflictResolver(versionSelector, scopeSelector,
optionalitySelector, scopeDeriver);
+ }
Review Comment:
⚠️ **Heuristic no longer matches PathConflictResolver behavior.**
`treeExceedsThreshold` counts shared nodes multiple times (no visited set),
mirroring the *old* PathConflictResolver expansion. After PR #2153,
PathConflictResolver uses depth-bounded expansion and does NOT re-traverse
subtrees of nodes already expanded at a shallower depth. The count from
`treeExceedsThreshold` now **overestimates** PathConflictResolver's actual
memory usage, causing unnecessary fallback to `ClassicConflictResolver` —
exactly the situation auto-selection was supposed to avoid.
##########
maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/transformer/ConflictResolverTest.java:
##########
@@ -860,6 +857,33 @@ void defaultConfigUsesAutoSelection() throws
RepositoryException {
assertSame(baz1Node, fooNode.getChildren().get(1));
}
+ /**
+ * Verifies that tree threshold checking stops as soon as the number of
visited nodes
+ * exceeds the configured threshold.
+ */
+ @org.junit.jupiter.api.Test
+ void treeThresholdStopsEarly() throws Exception {
+ ConflictResolver delegating = new ConflictResolver(
+ new NearestVersionSelector(),
+ new JavaScopeSelector(),
+ new SimpleOptionalitySelector(),
+ new JavaScopeDeriver());
+
+ DependencyNode root = makeDependencyNode("some-group", "root", "1.0");
+ DependencyNode child = makeDependencyNode("some-group", "child",
"1.0");
+ DependencyNode grandChild = makeDependencyNode("some-group",
"grand-child", "1.0");
+
+ root.setChildren(mutableList(child));
Review Comment:
💡 **Reflection-based test is fragile.** Using `setAccessible(true)` to test
a `private` method couples the test to an implementation detail. If the method
is renamed, moved, or its signature changes, this test breaks silently at
runtime rather than at compile time.
Consider either:
- Making `treeExceedsThreshold` package-private (it's a pure utility with no
side effects)
- Testing the behavior through the public API instead (verify that `auto`
config selects the expected resolver for small vs large graphs)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
