Copilot commented on code in PR #1938:
URL: https://github.com/apache/maven-resolver/pull/1938#discussion_r3487784159
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/PathConflictResolver.java:
##########
@@ -170,28 +167,33 @@ public DependencyNode transformGraph(DependencyNode node,
DependencyGraphTransfo
scopeDeriver.getInstance(node, context),
optionalitySelector.getInstance(node, context),
conflictIds,
+ sortedConflictIds.size(),
node);
// loop over topographically sorted conflictIds
for (String conflictId : sortedConflictIds) {
- // paths in given conflict group to consider
- Set<Path> paths = state.partitions.get(conflictId);
- if (paths.isEmpty()) {
+ // paths in given conflict group to consider; filter out those
moved out of scope
+ List<Path> allPaths = state.partitions.get(conflictId);
+ List<Path> activePaths = new ArrayList<>(allPaths.size());
+ List<ConflictItem> items = new ArrayList<>(allPaths.size());
+ for (Path p : allPaths) {
+ if (!p.outOfScope) {
+ activePaths.add(p);
+ items.add(new ConflictItem(p));
+ }
+ }
+ if (activePaths.isEmpty()) {
Review Comment:
`partitions` retains out-of-scope `Path` entries indefinitely. Even though
`activePaths` filters them out, the backing `allPaths` list still strongly
references losers (and entire detached subtrees in `Verbosity.NONE`/redundant
`STANDARD`), which can prevent GC and raise peak memory — the opposite of the
intended OOM mitigation. Consider compacting `allPaths` in-place while
filtering (or otherwise dropping references to out-of-scope entries) so
resolved/losing branches don’t stay retained via `partitions`.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/ConflictResolver.java:
##########
@@ -258,22 +264,58 @@ public ConflictResolver(
}
@Override
+ @SuppressWarnings("unchecked")
public DependencyNode transformGraph(DependencyNode node,
DependencyGraphTransformationContext context)
throws RepositoryException {
String cf = ConfigUtils.getString(
context.getSession(), DEFAULT_CONFLICT_RESOLVER_IMPL,
CONFIG_PROP_CONFLICT_RESOLVER_IMPL);
ConflictResolver delegate;
- if (PATH_CONFLICT_RESOLVER.equals(cf)) {
+ if (AUTO_CONFLICT_RESOLVER.equals(cf)) {
+ delegate = selectConflictResolver(node, context);
+ } else if (PATH_CONFLICT_RESOLVER.equals(cf)) {
delegate = new PathConflictResolver(versionSelector,
scopeSelector, optionalitySelector, scopeDeriver);
} else if (CLASSIC_CONFLICT_RESOLVER.equals(cf)) {
delegate = new ClassicConflictResolver(versionSelector,
scopeSelector, optionalitySelector, scopeDeriver);
} else {
throw new IllegalArgumentException("Unknown conflict resolver: " +
cf + "; known are "
- + Arrays.asList(PATH_CONFLICT_RESOLVER,
CLASSIC_CONFLICT_RESOLVER));
+ + Arrays.asList(AUTO_CONFLICT_RESOLVER,
PATH_CONFLICT_RESOLVER, CLASSIC_CONFLICT_RESOLVER));
}
return delegate.transformGraph(node, context);
}
+ /**
+ * Selects the most appropriate conflict resolver based on graph size and
available memory.
+ * <p>
+ * PathConflictResolver builds a parallel tree of Path objects that costs
~200 bytes per node.
+ * For very large dependency graphs (millions of nodes), this can exhaust
the heap.
Review Comment:
The new default "auto" resolver selection path isn’t covered by unit tests
(existing tests exercise `PathConflictResolver` and `ClassicConflictResolver`
directly). Adding tests that validate both branches of the heuristic would help
prevent regressions (e.g., by refactoring heap-availability and/or estimate
calculation behind overridable helpers or injectable suppliers so tests can
deterministically force either selection).
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/PathConflictResolver.java:
##########
@@ -610,43 +643,53 @@ private boolean isDirectDependencyOnPathToRoot(Artifact
artifact) {
* verbosity mode we remove "redundant" nodes (of a range) leaving
only "winner equal" loser, that have same GACEV as winner.
*/
private int relatedSiblingsCount(Artifact artifact, Path parent) {
- String ga = artifact.getGroupId() + ":" + artifact.getArtifactId();
- return Math.toIntExact(parent.children.stream()
- .map(n -> n.dn.getArtifact())
- .filter(a -> ga.equals(a.getGroupId() + ":" +
a.getArtifactId()))
- .count());
+ if (parent.children == null) {
+ return 0;
+ }
+ String groupId = artifact.getGroupId();
+ String artifactId = artifact.getArtifactId();
+ int count = 0;
+ for (Path n : parent.children) {
+ Artifact a = n.dn.getArtifact();
+ if (Objects.equals(groupId, a.getGroupId()) &&
Objects.equals(artifactId, a.getArtifactId())) {
+ count++;
+ }
+ }
+ return count;
}
/**
- * Removes this and all child {@link Path} nodes from winner selection
scope; essentially marks whole subtree
+ * Marks this and all child {@link Path} nodes as out of scope;
essentially marks whole subtree
* from "this and below" as loser, to not be considered in subsequent
winner selections.
- * Uses {@link LinkedHashSet} for O(1) removal instead of the previous
O(N) ArrayList.remove.
+ * Uses a boolean flag instead of removing from partition collections,
avoiding the per-entry
+ * overhead of LinkedHashSet (~48 bytes/entry). Out-of-scope paths are
filtered at query time.
*/
private void moveOutOfScope() {
- this.state.partitions.get(this.conflictId).remove(this);
- for (Path child : this.children) {
- child.moveOutOfScope();
+ this.outOfScope = true;
+ if (this.children != null) {
+ for (Path child : this.children) {
+ child.moveOutOfScope();
Review Comment:
`moveOutOfScope()` is still recursive. Deep dependency chains can still
trigger `StackOverflowError` when an early node becomes a loser (the subtree
walk recurses once per level). Converting this to an explicit stack keeps the
“no recursion” goal consistent with the new iterative `gatherCRNodes()` change.
--
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]
