diff --git a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java index c7c3edecb1..8d1d6f763f 100644 --- a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java +++ b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java @@ -377,8 +377,6 @@ private void doRecurse( DependencySelector childSelector = parentContext.depSelector != null ? parentContext.depSelector.deriveChildSelector(context) : null; - DependencyManager childManager = - parentContext.depManager != null ? parentContext.depManager.deriveChildManager(context) : null; DependencyTraverser childTraverser = parentContext.depTraverser != null ? parentContext.depTraverser.deriveChildTraverser(context) : null; VersionFilter childFilter = @@ -389,50 +387,80 @@ private void doRecurse( : remoteRepositoryManager.aggregateRepositories( args.session, parentContext.repositories, descriptorResult.getRepositories(), true); - Object key = args.pool.toKey( + // Optimization: try pool cache with the parent manager as a speculative key BEFORE + // calling deriveChildManager. When deriveChildManager returns `this` (the common case — + // no new management data at this depth), the parent manager IS the child manager, so + // this speculative key matches any previously stored entry. On pool hit, we skip + // deriveChildManager entirely, saving even the memoization cache lookup. + Object speculativeKey = args.pool.toKey( parentContext.dependency.getArtifact(), childRepos, childSelector, - childManager, + parentContext.depManager, childTraverser, childFilter); + List children = args.pool.getChildren(speculativeKey); + if (children != null) { + child.setChildren(children); + return; + } - List children = args.pool.getChildren(key); - if (children == null) { - boolean skipResolution = args.skipper.skipResolution(child, parentContext.parents); - if (!skipResolution) { - List parents = new ArrayList<>(parentContext.parents.size() + 1); - parents.addAll(parentContext.parents); - parents.add(child); - for (Dependency dependency : descriptorResult.getDependencies()) { - if (childSelector != null && !childSelector.selectDependency(dependency)) { - continue; - } - RequestTrace childTrace = collectStepTrace( - parentContext.trace, args.request.getRequestContext(), parents, dependency); - PremanagedDependency premanagedDependency = PremanagedDependency.create( - childManager, dependency, disableVersionManagement, args.premanagedState); - DependencyProcessingContext processingContext = new DependencyProcessingContext( - childSelector, - childManager, - childTraverser, - childFilter, - childTrace, - childRepos, - descriptorResult.getManagedDependencies(), - parents, - dependency, - premanagedDependency); - // resolve descriptors ahead for managed dependency - processingContext.withDependency(processingContext.premanagedDependency.getManagedDependency()); - resolveArtifactDescriptorAsync(args, processingContext, results); - args.dependencyProcessingQueue.add(processingContext); + // Speculative miss — compute the actual derived manager + DependencyManager childManager = + parentContext.depManager != null ? parentContext.depManager.deriveChildManager(context) : null; + + Object key; + if (childManager == parentContext.depManager) { + // Manager unchanged — speculative key was correct, already checked and missed + key = speculativeKey; + } else { + // Manager changed — recompute key and check pool again with the correct manager + key = args.pool.toKey( + parentContext.dependency.getArtifact(), + childRepos, + childSelector, + childManager, + childTraverser, + childFilter); + children = args.pool.getChildren(key); + if (children != null) { + child.setChildren(children); + return; + } + } + + // True cache miss — do full resolution + boolean skipResolution = args.skipper.skipResolution(child, parentContext.parents); + if (!skipResolution) { + List parents = new ArrayList<>(parentContext.parents.size() + 1); + parents.addAll(parentContext.parents); + parents.add(child); + for (Dependency dependency : descriptorResult.getDependencies()) { + if (childSelector != null && !childSelector.selectDependency(dependency)) { + continue; } - args.pool.putChildren(key, child.getChildren()); - args.skipper.cache(child, parents); + RequestTrace childTrace = + collectStepTrace(parentContext.trace, args.request.getRequestContext(), parents, dependency); + PremanagedDependency premanagedDependency = PremanagedDependency.create( + childManager, dependency, disableVersionManagement, args.premanagedState); + DependencyProcessingContext processingContext = new DependencyProcessingContext( + childSelector, + childManager, + childTraverser, + childFilter, + childTrace, + childRepos, + descriptorResult.getManagedDependencies(), + parents, + dependency, + premanagedDependency); + // resolve descriptors ahead for managed dependency + processingContext.withDependency(processingContext.premanagedDependency.getManagedDependency()); + resolveArtifactDescriptorAsync(args, processingContext, results); + args.dependencyProcessingQueue.add(processingContext); } - } else { - child.setChildren(children); + args.pool.putChildren(key, child.getChildren()); + args.skipper.cache(child, parents); } } diff --git a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/collect/bf/BfWithSkipperDependencyCollectorTest.java b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/collect/bf/BfWithSkipperDependencyCollectorTest.java index cf9edd8794..4876e40d59 100644 --- a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/collect/bf/BfWithSkipperDependencyCollectorTest.java +++ b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/collect/bf/BfWithSkipperDependencyCollectorTest.java @@ -25,6 +25,7 @@ import org.eclipse.aether.collection.CollectResult; import org.eclipse.aether.collection.DependencyCollectionException; import org.eclipse.aether.graph.Dependency; +import org.eclipse.aether.graph.DependencyNode; import org.eclipse.aether.graph.Exclusion; import org.eclipse.aether.impl.ArtifactDescriptorReader; import org.eclipse.aether.internal.impl.StubRemoteRepositoryManager; @@ -37,6 +38,8 @@ import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; /** * UT for {@link BfDependencyCollector}. @@ -68,6 +71,75 @@ private Dependency newDep(String coords, String scope, Collection exc return d.setExclusions(exclusions); } + /** + * Verifies that the pool cache is transparent w.r.t. the DependencyManager: the graph + * structure must not depend on whether the pool hits or misses. + *

+ * Scenario (from #2013): + * 

+     *   root
+     *   ├── b      → c → d
+     *   └── b-alt  → c → d   (c is a shared transitive dependency)
+     *
+ * With {@link TransitiveDependencyManager}, {@code deriveChildManager()} used to always + * create a new instance (unique {@code path} field), making every pool key unique. + * The pool would miss for {@code c} under {@code b-alt}, the skipper would mark it as + * a duplicate, and the node would end up with zero children — even though the same + * {@code c} under {@code b} had children. + *

+ * The fix in {@code AbstractDependencyManager.deriveChildManager()} reuses the same + * manager instance when no new management data is collected, so the pool key matches + * and children are preserved. + */ + @Test + void testPoolCacheTransparencyWithTransitiveDependencyManager() throws DependencyCollectionException { + collector = setupCollector(newReader("pool-cache-transparency/")); + parser = new DependencyGraphParser("artifact-descriptions/pool-cache-transparency/"); + session.setDependencyManager(new TransitiveDependencyManager(null)); + + Dependency root = newDep("gid:root:ext:1.0", "compile"); + CollectRequest request = new CollectRequest(root, Collections.singletonList(repository)); + CollectResult result = collector.collectDependencies(session, request); + + assertEquals(0, result.getExceptions().size()); + + // root has two children: b and b-alt + DependencyNode rootNode = result.getRoot(); + assertEquals(2, rootNode.getChildren().size(), "root should have 2 children (b, b-alt)"); + + // b → c + DependencyNode b = rootNode.getChildren().get(0); + assertEquals("b", b.getArtifact().getArtifactId()); + assertFalse(b.getChildren().isEmpty(), "b should have children"); + + // b → c → d + DependencyNode cUnderB = b.getChildren().get(0); + assertEquals("c", cUnderB.getArtifact().getArtifactId()); + assertFalse(cUnderB.getChildren().isEmpty(), "c under b should have children (d)"); + + // b-alt → c (this is the key assertion: c under b-alt must also have children) + DependencyNode bAlt = rootNode.getChildren().get(1); + assertEquals("b-alt", bAlt.getArtifact().getArtifactId()); + assertFalse(bAlt.getChildren().isEmpty(), "b-alt should have children"); + + DependencyNode cUnderBAlt = bAlt.getChildren().get(0); + assertEquals("c", cUnderBAlt.getArtifact().getArtifactId()); + + // Before the fix, this assertion would fail: c under b-alt had zero children + // because the pool key differed (different DependencyManager instance) and the + // skipper marked it as a duplicate. + assertFalse( + cUnderBAlt.getChildren().isEmpty(), + "c under b-alt should have children (d) — pool cache must be transparent"); + + // Verify that c's child is d in both subtrees + assertEquals("d", cUnderB.getChildren().get(0).getArtifact().getArtifactId()); + assertTrue( + cUnderBAlt.getChildren().stream() + .anyMatch(n -> "d".equals(n.getArtifact().getArtifactId())), + "c under b-alt should have d as a child"); + } + @Test void testSkipperWithDifferentExclusion() throws DependencyCollectionException { collector = setupCollector(newReader("managed/")); diff --git a/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b-alt_1.0.ini b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b-alt_1.0.ini new file mode 100644 index 0000000000..a5aecf921b --- /dev/null +++ b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b-alt_1.0.ini @@ -0,0 +1,2 @@ +[dependencies] +gid:c:ext:1.0 diff --git a/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b_1.0.ini b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b_1.0.ini new file mode 100644 index 0000000000..a5aecf921b --- /dev/null +++ b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b_1.0.ini @@ -0,0 +1,2 @@ +[dependencies] +gid:c:ext:1.0 diff --git a/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_c_1.0.ini b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_c_1.0.ini new file mode 100644 index 0000000000..732de5300a --- /dev/null +++ b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_c_1.0.ini @@ -0,0 +1,2 @@ +[dependencies] +gid:d:ext:1.0 diff --git a/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_d_1.0.ini b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_d_1.0.ini new file mode 100644 index 0000000000..61a252c235 --- /dev/null +++ b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_d_1.0.ini @@ -0,0 +1 @@ +[dependencies] diff --git a/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_root_1.0.ini b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_root_1.0.ini new file mode 100644 index 0000000000..f59e27e009 --- /dev/null +++ b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_root_1.0.ini @@ -0,0 +1,3 @@ +[dependencies] +gid:b:ext:1.0 +gid:b-alt:ext:1.0 diff --git a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java index 2b2eeab358..d38786fe79 100644 --- a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java +++ b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java @@ -22,6 +22,7 @@ import java.util.Collection; import java.util.HashMap; import java.util.LinkedHashSet; +import java.util.List; import java.util.Objects; import org.eclipse.aether.artifact.Artifact; @@ -35,6 +36,14 @@ import static java.util.Objects.requireNonNull; +// Note on lookup semantics: management rules follow "nearest to root wins" precedence. +// Each instance maintains a cumulative ancestor HashMap that flattens the full parent +// chain into a single map, providing O(1) lookups instead of O(depth) chain walks. +// The cumulative map is built incrementally at construction time: child.ancestors = +// parent.ancestors + parent.ownData. When the parent has no per-level data, the +// reference is shared (zero copy). Since containsManagedXxx() during derive blocks +// duplicate entries for most properties, there is at most one value per key. + /** * A dependency manager support class for Maven-specific dependency graph management. * @@ -105,8 +114,12 @@ * @since 2.0.0 */ public abstract class AbstractDependencyManager implements DependencyManager { - /** The path of parent managers from root to current level. */ - protected final ArrayList path; + /** + * Parent manager in the dependency graph (forms a linked list from leaf toward root). + * Replaces the previous {@code ArrayList path} field — + * siblings share the same parent reference (O(1) derive instead of O(depth) copy). + */ + protected final AbstractDependencyManager parent; /** The current depth in the dependency graph (0 = factory, 1 = root, 2+ = descendants). */ protected final int depth; @@ -135,9 +148,51 @@ public abstract class AbstractDependencyManager implements DependencyManager { /** System dependency scope handler, may be null if no system scope is defined. */ protected final SystemDependencyScope systemDependencyScope; - /** Pre-computed hash code (excludes managedLocalPaths). */ + // ── Cumulative ancestor maps ────────────────────────────────────────────── + // Flattened view of ALL management entries from root through parent. + // Provides O(1) lookups instead of O(depth) parent-chain walks. + // When the parent has no per-level data, the child shares the same reference + // (zero-copy). These are derived data, excluded from equals/hashCode. + + /** Union of all ancestor version entries (root through parent). */ + private final HashMap ancestorVersions; + + /** Union of all ancestor scope entries (root through parent). */ + private final HashMap ancestorScopes; + + /** Union of all ancestor optional entries (root through parent). */ + private final HashMap ancestorOptionals; + + /** Union of all ancestor local-path entries (root through parent). */ + private final HashMap ancestorLocalPaths; + + /** Union of all ancestor exclusion entries (root through parent), merged additively. */ + private final HashMap> ancestorExclusions; + + /** + * Pre-computed hash code (excludes managedLocalPaths). + * Cascading: incorporates the parent's hashCode so a single int comparison + * reflects the entire ancestor chain without walking it. + */ private final int hashCode; + /** + * Single-entry memoization cache for {@link #deriveChildManager(DependencyCollectionContext)}: + * remembers the last managed-dependency list (by reference identity) and the result it produced. + *

+ * In BFS dependency collection, consecutive siblings typically share the same parent artifact + * descriptor and hence the same interned managed-dependency list (guaranteed by DataPool's + * {@code internArtifactDescriptorManagedDependencies}). A single-entry cache therefore hits + * on nearly every call while using only two fields of constant memory — unlike an unbounded + * {@code IdentityHashMap} which would retain every derived {@code DependencyManager} and + * prevent GC of the dependency subtrees they reference. + *

+ * The BFS collector's traversal loop is single-threaded, so no synchronization is needed. + */ + private transient List lastManagedDeps; + + private transient DependencyManager lastDeriveResult; + /** * Creates a new dependency manager with the specified derivation and application parameters. * @@ -148,7 +203,7 @@ public abstract class AbstractDependencyManager implements DependencyManager { */ protected AbstractDependencyManager(int deriveUntil, int applyFrom, ScopeManager scopeManager) { this( - new ArrayList<>(), + null, 0, deriveUntil, applyFrom, @@ -164,7 +219,7 @@ protected AbstractDependencyManager(int deriveUntil, int applyFrom, ScopeManager @SuppressWarnings("checkstyle:ParameterNumber") protected AbstractDependencyManager( - ArrayList path, + AbstractDependencyManager parent, int depth, int deriveUntil, int applyFrom, @@ -174,7 +229,7 @@ protected AbstractDependencyManager( MMap managedLocalPaths, MMap>> managedExclusions, SystemDependencyScope systemDependencyScope) { - this.path = path; + this.parent = parent; this.depth = depth; this.deriveUntil = deriveUntil; this.applyFrom = applyFrom; @@ -186,8 +241,70 @@ protected AbstractDependencyManager( // nullable: if using scope manager, but there is no system scope defined this.systemDependencyScope = systemDependencyScope; - // exclude managedLocalPaths - this.hashCode = Objects.hash(path, depth, managedVersions, managedScopes, managedOptionals, managedExclusions); + // Build cumulative ancestor maps: parent's ancestors + parent's own per-level data. + // When parent has no per-level data, the child shares the parent's reference (zero copy). + if (parent != null) { + this.ancestorVersions = mergeAncestors(parent.ancestorVersions, parent.managedVersions); + this.ancestorScopes = mergeAncestors(parent.ancestorScopes, parent.managedScopes); + this.ancestorOptionals = mergeAncestors(parent.ancestorOptionals, parent.managedOptionals); + this.ancestorLocalPaths = mergeAncestors(parent.ancestorLocalPaths, parent.managedLocalPaths); + this.ancestorExclusions = mergeAncestorExclusions(parent.ancestorExclusions, parent.managedExclusions); + } else { + this.ancestorVersions = null; + this.ancestorScopes = null; + this.ancestorOptionals = null; + this.ancestorLocalPaths = null; + this.ancestorExclusions = null; + } + + // Cascading hash: incorporates the parent's pre-computed hash so a single int + // comparison reflects the entire ancestor chain. Excludes managedLocalPaths. + this.hashCode = Objects.hash( + parent != null ? parent.hashCode : 0, + depth, + managedVersions, + managedScopes, + managedOptionals, + managedExclusions); + } + + /** + * Merges the parent's cumulative ancestor map with the parent's own per-level MMap, + * producing the child's cumulative ancestor map. When the parent has no per-level data, + * the parent's cumulative map is returned as-is (zero copy). + */ + private static HashMap mergeAncestors(HashMap parentAncestors, MMap parentOwn) { + if (parentAncestors == null && parentOwn == null) { + return null; + } + if (parentOwn == null) { + return parentAncestors; // share reference — no new data at this level + } + HashMap result = parentAncestors != null ? new HashMap<>(parentAncestors) : new HashMap<>(); + result.putAll(parentOwn.delegate); + return result; + } + + /** + * Merges the parent's cumulative ancestor exclusions with the parent's own per-level exclusions. + * Unlike other properties, exclusions are accumulated additively (same key → merge collections). + */ + private static HashMap> mergeAncestorExclusions( + HashMap> parentAncestors, MMap>> parentOwn) { + if (parentAncestors == null && parentOwn == null) { + return null; + } + if (parentOwn == null) { + return parentAncestors; // share reference + } + HashMap> result = + parentAncestors != null ? new HashMap<>(parentAncestors) : new HashMap<>(); + parentOwn.delegate.forEach((key, holder) -> result.merge(key, holder.getValue(), (existing, newExcl) -> { + ArrayList merged = new ArrayList<>(existing); + merged.addAll(newExcl); + return merged; + })); + return result; } protected abstract DependencyManager newInstance( @@ -198,115 +315,117 @@ protected abstract DependencyManager newInstance( MMap>> managedExclusions); private boolean containsManagedVersion(Key key, MMap managedVersions) { - for (AbstractDependencyManager ancestor : path) { - if (ancestor.managedVersions != null && ancestor.managedVersions.containsKey(key)) { - return true; - } + // Check current instance's own managed versions first (restores the pre-d4035d3a + // check that was accidentally dropped when the parameter was introduced). + if (this.managedVersions != null && this.managedVersions.containsKey(key)) { + return true; + } + // O(1) lookup in cumulative ancestor map (replaces O(depth) parent chain walk) + if (ancestorVersions != null && ancestorVersions.containsKey(key)) { + return true; } + // Check in-progress new map for duplicates within the same derivation step. return managedVersions != null && managedVersions.containsKey(key); } + /** + * O(1) lookup in the cumulative ancestor map for the managed version. + * At depth 1, also checks own data (root self-application): when DefaultDependencyManager + * applies management from depth 0, the root-level rules are stored in DM1.managedVersions + * and must be visible to DM1.manageDependency(). + */ private String getManagedVersion(Key key) { - for (AbstractDependencyManager ancestor : path) { - if (ancestor.managedVersions != null && ancestor.managedVersions.containsKey(key)) { - return ancestor.managedVersions.get(key); - } - } + String result = ancestorVersions != null ? ancestorVersions.get(key) : null; if (depth == 1 && managedVersions != null && managedVersions.containsKey(key)) { - return managedVersions.get(key); + result = managedVersions.get(key); } - return null; + return result; } private boolean containsManagedScope(Key key, MMap managedScopes) { - for (AbstractDependencyManager ancestor : path) { - if (ancestor.managedScopes != null && ancestor.managedScopes.containsKey(key)) { - return true; - } + if (this.managedScopes != null && this.managedScopes.containsKey(key)) { + return true; + } + if (ancestorScopes != null && ancestorScopes.containsKey(key)) { + return true; } return managedScopes != null && managedScopes.containsKey(key); } private String getManagedScope(Key key) { - for (AbstractDependencyManager ancestor : path) { - if (ancestor.managedScopes != null && ancestor.managedScopes.containsKey(key)) { - return ancestor.managedScopes.get(key); - } - } + String result = ancestorScopes != null ? ancestorScopes.get(key) : null; if (depth == 1 && managedScopes != null && managedScopes.containsKey(key)) { - return managedScopes.get(key); + result = managedScopes.get(key); } - return null; + return result; } private boolean containsManagedOptional(Key key, MMap managedOptionals) { - for (AbstractDependencyManager ancestor : path) { - if (ancestor.managedOptionals != null && ancestor.managedOptionals.containsKey(key)) { - return true; - } + if (this.managedOptionals != null && this.managedOptionals.containsKey(key)) { + return true; + } + if (ancestorOptionals != null && ancestorOptionals.containsKey(key)) { + return true; } return managedOptionals != null && managedOptionals.containsKey(key); } private Boolean getManagedOptional(Key key) { - for (AbstractDependencyManager ancestor : path) { - if (ancestor.managedOptionals != null && ancestor.managedOptionals.containsKey(key)) { - return ancestor.managedOptionals.get(key); - } - } + Boolean result = ancestorOptionals != null ? ancestorOptionals.get(key) : null; if (depth == 1 && managedOptionals != null && managedOptionals.containsKey(key)) { - return managedOptionals.get(key); + result = managedOptionals.get(key); } - return null; + return result; } private boolean containsManagedLocalPath(Key key, MMap managedLocalPaths) { - for (AbstractDependencyManager ancestor : path) { - if (ancestor.managedLocalPaths != null && ancestor.managedLocalPaths.containsKey(key)) { - return true; - } + if (this.managedLocalPaths != null && this.managedLocalPaths.containsKey(key)) { + return true; + } + if (ancestorLocalPaths != null && ancestorLocalPaths.containsKey(key)) { + return true; } return managedLocalPaths != null && managedLocalPaths.containsKey(key); } /** * Gets the managed local path for system dependencies. - * Note: Local paths don't follow the depth=1 special rule like versions/scopes. - * - * @param key the dependency key - * @return the managed local path, or null if not managed + * Note: Local paths don't follow the depth=1 special rule like versions/scopes — + * own data is always checked (system path alignment across the graph). */ private String getManagedLocalPath(Key key) { - for (AbstractDependencyManager ancestor : path) { - if (ancestor.managedLocalPaths != null && ancestor.managedLocalPaths.containsKey(key)) { - return ancestor.managedLocalPaths.get(key); - } - } + String result = ancestorLocalPaths != null ? ancestorLocalPaths.get(key) : null; if (managedLocalPaths != null && managedLocalPaths.containsKey(key)) { - return managedLocalPaths.get(key); + result = managedLocalPaths.get(key); } - return null; + return result; } /** - * Merges exclusions from all levels in the dependency path. + * Returns merged exclusions from the cumulative ancestor map plus own exclusions. * Unlike other managed properties, exclusions are accumulated additively - * from root to current level throughout the entire dependency path. + * from all levels in the dependency path. * * @param key the dependency key * @return merged collection of exclusions, or null if none exist */ private Collection getManagedExclusions(Key key) { - ArrayList result = new ArrayList<>(); - for (AbstractDependencyManager ancestor : path) { - if (ancestor.managedExclusions != null && ancestor.managedExclusions.containsKey(key)) { - result.addAll(ancestor.managedExclusions.get(key).value); - } + Collection ancestorExcl = ancestorExclusions != null ? ancestorExclusions.get(key) : null; + Holder> ownExcl = managedExclusions != null ? managedExclusions.get(key) : null; + + if (ancestorExcl == null && ownExcl == null) { + return null; + } + if (ancestorExcl != null && ownExcl == null) { + return ancestorExcl; } - if (managedExclusions != null && managedExclusions.containsKey(key)) { - result.addAll(managedExclusions.get(key).value); + if (ancestorExcl == null) { + return ownExcl.value; } - return result.isEmpty() ? null : result; + // Both present: merge additively + ArrayList result = new ArrayList<>(ancestorExcl); + result.addAll(ownExcl.value); + return result; } @Override @@ -316,13 +435,22 @@ public DependencyManager deriveChildManager(DependencyCollectionContext context) return this; } + // Memoization: if this is the same managed dependencies list we saw last time + // (same object reference — guaranteed by DataPool's descriptor/list interning), + // return the cached result immediately. This turns the common case from O(managedDeps) + // iteration + Key hashing into a single O(1) identity check. + List managedDeps = context.getManagedDependencies(); + if (managedDeps == lastManagedDeps && lastDeriveResult != null) { + return lastDeriveResult; + } + MMap managedVersions = null; MMap managedScopes = null; MMap managedOptionals = null; MMap managedLocalPaths = null; MMap>> managedExclusions = null; - for (Dependency managedDependency : context.getManagedDependencies()) { + for (Dependency managedDependency : managedDeps) { Artifact artifact = managedDependency.getArtifact(); Key key = new Key(artifact); @@ -379,12 +507,47 @@ public DependencyManager deriveChildManager(DependencyCollectionContext context) } } - return newInstance( - managedVersions != null ? managedVersions.done() : null, - managedScopes != null ? managedScopes.done() : null, - managedOptionals != null ? managedOptionals.done() : null, - managedLocalPaths != null ? managedLocalPaths.done() : null, - managedExclusions != null ? managedExclusions.done() : null); + // Optimization: when no new management data was collected at this depth and management + // is already being applied (depth >= applyFrom), reuse this instance. This avoids creating + // unnecessarily distinct DependencyManager instances that would defeat the BF collector's + // pool cache — the pool key includes the manager, so distinct-but-semantically-equal + // managers cause pool misses, which in turn lets the skipper prune subtrees that should + // have been served from the cache. This is the common case for transitive dependencies + // whose POMs do not declare . + // + // However, we can only reuse `this` when it carries no management data of its own. + // If `this` has management data (e.g. managedVersions != null), returning `this` would + // hide that data from the child: getManagedVersion() only checks the parent chain (not + // `this.managedVersions`), so a reused instance's own rules become invisible. In that + // case we must create a new child with null maps, making `this` the parent and putting + // the management data on the parent chain where getManagedVersion() can find it. + // See https://github.com/apache/maven-resolver/issues/2013 + DependencyManager result; + if (managedVersions == null + && managedScopes == null + && managedOptionals == null + && managedLocalPaths == null + && managedExclusions == null + && isApplied() + && this.managedVersions == null + && this.managedScopes == null + && this.managedOptionals == null + && this.managedLocalPaths == null + && this.managedExclusions == null) { + result = this; + } else { + result = newInstance( + managedVersions != null ? managedVersions.done() : null, + managedScopes != null ? managedScopes.done() : null, + managedOptionals != null ? managedOptionals.done() : null, + managedLocalPaths != null ? managedLocalPaths.done() : null, + managedExclusions != null ? managedExclusions.done() : null); + } + + // Cache the result for future calls with the same managed deps list identity + lastManagedDeps = managedDeps; + lastDeriveResult = result; + return result; } @Override @@ -518,13 +681,21 @@ public boolean equals(Object obj) { } AbstractDependencyManager that = (AbstractDependencyManager) obj; + // Fast rejection: cascading hashCode reflects the entire ancestor chain, + // so a single int mismatch rejects without walking any parent pointers. + if (hashCode != that.hashCode) { + return false; + } // exclude managedLocalPaths - return Objects.equals(path, that.path) - && depth == that.depth + // Check cheap fields (depth) before expensive ones (maps, parent chain). + // Parent comparison is recursive but each level is hash-guarded, and + // shared parents (same identity) short-circuit via the this==obj check. + return depth == that.depth && Objects.equals(managedVersions, that.managedVersions) && Objects.equals(managedScopes, that.managedScopes) && Objects.equals(managedOptionals, that.managedOptionals) - && Objects.equals(managedExclusions, that.managedExclusions); + && Objects.equals(managedExclusions, that.managedExclusions) + && Objects.equals(parent, that.parent); } @Override @@ -537,18 +708,26 @@ public int hashCode() { * GACE = Group, Artifact, Classifier, Extension (excludes version for management purposes). */ protected static class Key { - private final Artifact artifact; + private final String groupId; + private final String artifactId; + private final String extension; + private final String classifier; private final int hashCode; /** * Creates a new key from the given artifact's GACE coordinates. + * Coordinate strings are cached eagerly to avoid repeated virtual dispatch + * through delegation wrappers like {@code RelocatedArtifact} during + * {@link #equals} comparisons in hash maps. * * @param artifact the artifact to create a key for */ Key(Artifact artifact) { - this.artifact = artifact; - this.hashCode = Objects.hash( - artifact.getArtifactId(), artifact.getGroupId(), artifact.getExtension(), artifact.getClassifier()); + this.groupId = artifact.getGroupId(); + this.artifactId = artifact.getArtifactId(); + this.extension = artifact.getExtension(); + this.classifier = artifact.getClassifier(); + this.hashCode = Objects.hash(artifactId, groupId, extension, classifier); } @Override @@ -559,10 +738,10 @@ public boolean equals(Object obj) { return false; } Key that = (Key) obj; - return artifact.getArtifactId().equals(that.artifact.getArtifactId()) - && artifact.getGroupId().equals(that.artifact.getGroupId()) - && artifact.getExtension().equals(that.artifact.getExtension()) - && artifact.getClassifier().equals(that.artifact.getClassifier()); + return artifactId.equals(that.artifactId) + && groupId.equals(that.groupId) + && extension.equals(that.extension) + && classifier.equals(that.classifier); } @Override @@ -572,7 +751,7 @@ public int hashCode() { @Override public String toString() { - return String.valueOf(artifact); + return groupId + ":" + artifactId + ":" + extension + (classifier.isEmpty() ? "" : ":" + classifier); } } diff --git a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/ClassicDependencyManager.java b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/ClassicDependencyManager.java index beefb1cb65..481f0baadb 100644 --- a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/ClassicDependencyManager.java +++ b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/ClassicDependencyManager.java @@ -18,7 +18,6 @@ */ package org.eclipse.aether.util.graph.manager; -import java.util.ArrayList; import java.util.Collection; import org.eclipse.aether.collection.DependencyCollectionContext; @@ -89,7 +88,7 @@ public ClassicDependencyManager(ScopeManager scopeManager) { @SuppressWarnings("checkstyle:ParameterNumber") private ClassicDependencyManager( - ArrayList path, + AbstractDependencyManager parent, int depth, int deriveUntil, int applyFrom, @@ -100,7 +99,7 @@ private ClassicDependencyManager( MMap>> managedExclusions, SystemDependencyScope systemDependencyScope) { super( - path, + parent, depth, deriveUntil, applyFrom, @@ -148,10 +147,8 @@ protected DependencyManager newInstance( MMap managedOptionals, MMap managedLocalPaths, MMap>> managedExclusions) { - ArrayList path = new ArrayList<>(this.path); - path.add(this); return new ClassicDependencyManager( - path, + this, depth + 1, deriveUntil, applyFrom, diff --git a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/DefaultDependencyManager.java b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/DefaultDependencyManager.java index 05fc72f9bc..a1b1297f77 100644 --- a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/DefaultDependencyManager.java +++ b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/DefaultDependencyManager.java @@ -18,7 +18,6 @@ */ package org.eclipse.aether.util.graph.manager; -import java.util.ArrayList; import java.util.Collection; import org.eclipse.aether.collection.DependencyManager; @@ -97,7 +96,7 @@ public DefaultDependencyManager(ScopeManager scopeManager) { @SuppressWarnings("checkstyle:ParameterNumber") private DefaultDependencyManager( - ArrayList path, + AbstractDependencyManager parent, int depth, int deriveUntil, int applyFrom, @@ -108,7 +107,7 @@ private DefaultDependencyManager( MMap>> managedExclusions, SystemDependencyScope systemDependencyScope) { super( - path, + parent, depth, deriveUntil, applyFrom, @@ -127,10 +126,8 @@ protected DependencyManager newInstance( MMap managedOptionals, MMap managedLocalPaths, MMap>> managedExclusions) { - ArrayList path = new ArrayList<>(this.path); - path.add(this); return new DefaultDependencyManager( - path, + this, depth + 1, deriveUntil, applyFrom, diff --git a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/MMap.java b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/MMap.java index 2d0079f86c..75f0a34101 100644 --- a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/MMap.java +++ b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/MMap.java @@ -122,10 +122,19 @@ public int hashCode() { @Override public boolean equals(Object o) { - if (!(o instanceof MMap)) { + if (this == o) { + return true; + } + if (!(o instanceof DoneMMap)) { + return false; + } + DoneMMap other = (DoneMMap) o; + // Fast rejection: hashCode is pre-computed, so a mismatch avoids + // the expensive deep map equality below (which triggers Key.equals + // for every entry). + if (hashCode != other.hashCode) { return false; } - MMap other = (MMap) o; return delegate.equals(other.delegate); } } diff --git a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/TransitiveDependencyManager.java b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/TransitiveDependencyManager.java index c45b64bc7e..3af9b2dad9 100644 --- a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/TransitiveDependencyManager.java +++ b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/TransitiveDependencyManager.java @@ -18,7 +18,6 @@ */ package org.eclipse.aether.util.graph.manager; -import java.util.ArrayList; import java.util.Collection; import org.eclipse.aether.collection.DependencyManager; @@ -96,7 +95,7 @@ public TransitiveDependencyManager(ScopeManager scopeManager) { @SuppressWarnings("checkstyle:ParameterNumber") private TransitiveDependencyManager( - ArrayList path, + AbstractDependencyManager parent, int depth, int deriveUntil, int applyFrom, @@ -107,7 +106,7 @@ private TransitiveDependencyManager( MMap>> managedExclusions, SystemDependencyScope systemDependencyScope) { super( - path, + parent, depth, deriveUntil, applyFrom, @@ -126,10 +125,8 @@ protected DependencyManager newInstance( MMap managedOptionals, MMap managedLocalPaths, MMap>> managedExclusions) { - ArrayList path = new ArrayList<>(this.path); - path.add(this); return new TransitiveDependencyManager( - path, + this, depth + 1, deriveUntil, applyFrom, diff --git a/maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/manager/DependencyManagerTest.java b/maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/manager/DependencyManagerTest.java index db5c4afe17..c9ed8105bd 100644 --- a/maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/manager/DependencyManagerTest.java +++ b/maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/manager/DependencyManagerTest.java @@ -18,8 +18,10 @@ */ package org.eclipse.aether.util.graph.manager; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.List; import org.eclipse.aether.RepositorySystemSession; import org.eclipse.aether.artifact.Artifact; @@ -204,6 +206,107 @@ void testTransitive() { assertNull(mngt); } + /** + * Verifies the instance-reuse optimization in {@link AbstractDependencyManager#deriveChildManager}: + * when no new management data is collected and management is already being applied + * (depth >= applyFrom), deriveChildManager should return the same instance. + * This is critical for BF collector pool cache transparency (issue #2013). + */ + @Test + void testDeriveChildManagerReusesInstanceWhenNoNewManagementData() { + DependencyManager manager = new TransitiveDependencyManager(null); + + // depth=0 → depth=1: root → first level (applyFrom=2, so not yet applied) + DependencyManager depth1 = manager.deriveChildManager(newContext()); + assertNotSame(manager, depth1, "depth 0→1: must return new instance (not yet applied)"); + + // depth=1 → depth=2: no managed deps, now at applyFrom=2 (applied) + DependencyManager depth2 = depth1.deriveChildManager(newContext()); + assertNotSame(depth1, depth2, "depth 1→2: must return new instance (crossing applyFrom boundary)"); + + // depth=2 → depth=3: no managed deps, already applied → should reuse + DependencyManager depth3 = depth2.deriveChildManager(newContext()); + assertSame(depth2, depth3, "depth 2→3 with no managed deps: should reuse instance"); + + // depth=3 → depth=4: still no managed deps → should keep reusing + DependencyManager depth4 = depth3.deriveChildManager(newContext()); + assertSame(depth3, depth4, "depth 3→4 with no managed deps: should reuse instance"); + + // depth=2 → depth=3 with managed deps: should NOT reuse + DependencyManager depth3WithMgmt = + depth2.deriveChildManager(newContext(new Dependency(new DefaultArtifact("new:dep:1.0"), "compile"))); + assertNotSame(depth2, depth3WithMgmt, "depth 2→3 with managed deps: must return new instance"); + } + + /** + * Verifies the instance-reuse optimization with {@link DefaultDependencyManager} (applyFrom=0). + * Since management is applied from depth 0, the optimization should fire at the very first + * derivation when no management data is collected. + */ + @Test + void testDeriveChildManagerReusesInstanceWithDefaultManager() { + DependencyManager manager = new DefaultDependencyManager(null); + + // DefaultDependencyManager has applyFrom=0, so isApplied() is true from the start. + // depth=0 → depth=1: no managed deps → should reuse (already applied) + DependencyManager depth1 = manager.deriveChildManager(newContext()); + assertSame(manager, depth1, "depth 0→1 with no managed deps: should reuse (applyFrom=0)"); + + // depth=0 → depth=1 with managed deps: should NOT reuse + DependencyManager depth1WithMgmt = + manager.deriveChildManager(newContext(new Dependency(new DefaultArtifact("mgd:dep:1.0"), "compile"))); + assertNotSame(manager, depth1WithMgmt, "depth 0→1 with managed deps: must return new instance"); + + // depth=1 (with mgmt) → depth=2: no managed deps but parent has management data, + // so a new instance must be created to move the management data onto the parent chain + // where getManagedVersion() can find it ("do not apply onto itself" semantics). + DependencyManager depth2 = depth1WithMgmt.deriveChildManager(newContext()); + assertNotSame(depth1WithMgmt, depth2, "depth 1→2 with parent mgmt: must create new child"); + + // depth=2 (no own mgmt) → depth=3: no managed deps → should reuse (no mgmt data) + DependencyManager depth3 = depth2.deriveChildManager(newContext()); + assertSame(depth2, depth3, "depth 2→3 with no managed deps: should reuse"); + } + + /** + * Verifies that the "nearer-to-root wins" invariant holds when the optimization + * reuses instances. Scenario: root contributes version management at depth 1, + * the optimization fires at depth 2→3 (empty context), and a deeper POM tries + * to override the same key — the root's version must still win. + */ + @Test + void testNearerToRootWinsAfterOptimizationReusesInstance() { + DependencyManager manager = new TransitiveDependencyManager(null); + + // depth=0 → depth=1: root contributes version 1.0 for artifact "x" + DependencyManager depth1 = + manager.deriveChildManager(newContext(new Dependency(new DefaultArtifact("test:x:1.0"), "compile"))); + + // depth=1 → depth=2: empty context (new instance because applyFrom boundary) + DependencyManager depth2 = depth1.deriveChildManager(newContext()); + assertNotSame(depth1, depth2, "depth 1→2: new instance (crossing applyFrom boundary)"); + + // depth=2 → depth=3: empty context, optimization fires + DependencyManager depth3 = depth2.deriveChildManager(newContext()); + assertSame(depth2, depth3, "depth 2→3: optimization should reuse instance"); + + // Verify root's version still applies after optimization reuse + DependencyManagement mngt = depth3.manageDependency(new Dependency(new DefaultArtifact("test:x:9.0"), null)); + assertNotNull(mngt, "management should apply at depth >= applyFrom"); + assertEquals("1.0", mngt.getVersion(), "root's version must apply after optimization reuse"); + + // depth=3 → depth=4: context tries to override "x" with 2.0, but containsManagedVersion + // finds it in ancestors → no new data collected → optimization fires again + DependencyManager depth4 = + depth3.deriveChildManager(newContext(new Dependency(new DefaultArtifact("test:x:2.0"), "compile"))); + assertSame(depth3, depth4, "optimization should fire when context only has already-managed keys"); + + // Root's version still wins ("nearer-to-root wins" invariant) + mngt = depth4.manageDependency(new Dependency(new DefaultArtifact("test:x:9.0"), null)); + assertNotNull(mngt, "management should still apply"); + assertEquals("1.0", mngt.getVersion(), "nearer-to-root version must win over deeper override attempt"); + } + @Test void testDefault() { DependencyManager manager = new DefaultDependencyManager(null); @@ -268,4 +371,106 @@ void testDefault() { // DO NOT APPLY ONTO ITSELF assertNull(mngt); } + + /** + * Verifies the memoization cache in {@link AbstractDependencyManager#deriveChildManager}: + * when the same managed dependencies list (same object reference) is passed multiple times, + * the result must be identical (same object reference). This simulates the BFS collector + * processing multiple siblings that share the same artifact descriptor — each call sees + * the same interned managed deps list from the DataPool. + */ + @Test + void testDeriveChildManagerMemoizationOnListIdentity() { + DependencyManager manager = new TransitiveDependencyManager(null); + + // Get to depth >= applyFrom (depth 2) so the optimization can fire + manager = + manager.deriveChildManager(newContext(new Dependency(new DefaultArtifact("root:dep:1.0"), "compile"))); + manager = manager.deriveChildManager(newContext()); + + // Create a SINGLE managed deps list — simulates interned list from DataPool + List sharedManagedDeps = Arrays.asList( + new Dependency(new DefaultArtifact("mgd:a:1.0"), "compile"), + new Dependency(new DefaultArtifact("mgd:b:2.0"), "runtime")); + + // First call: processes the list, caches the result + DependencyCollectionContext ctx1 = TestUtils.newCollectionContext(session, null, sharedManagedDeps); + DependencyManager result1 = manager.deriveChildManager(ctx1); + + // Second call with the SAME list object: should return the cached result + DependencyCollectionContext ctx2 = TestUtils.newCollectionContext(session, null, sharedManagedDeps); + DependencyManager result2 = manager.deriveChildManager(ctx2); + + assertSame(result1, result2, "memoization must return same instance for same list identity"); + + // Third call with a DIFFERENT list object (same content): must NOT use cache + // (identity mismatch — different object reference) + List differentList = new ArrayList<>(sharedManagedDeps); + DependencyCollectionContext ctx3 = TestUtils.newCollectionContext(session, null, differentList); + DependencyManager result3 = manager.deriveChildManager(ctx3); + + // result3 should be equal to result1 (same content), but NOT necessarily the same object + // (identity-based cache misses on different list objects) + assertEquals(result1, result3, "same content should produce equal results"); + } + + /** + * Verifies that the single-entry memoization cache correctly distinguishes between different + * managed dependency lists. A cached result for list L1 must not be returned for list L2. + * Since the cache is single-entry (last-seen), calling with L2 evicts L1. + */ + @Test + void testDeriveChildManagerMemoizationDistinguishesDifferentLists() { + DependencyManager manager = new TransitiveDependencyManager(null); + + // Get to depth >= applyFrom + manager = + manager.deriveChildManager(newContext(new Dependency(new DefaultArtifact("root:dep:1.0"), "compile"))); + manager = manager.deriveChildManager(newContext()); + + // Two different list objects with different content + List list1 = Collections.singletonList(new Dependency(new DefaultArtifact("mgd:a:1.0"), "compile")); + List list2 = Collections.singletonList(new Dependency(new DefaultArtifact("mgd:b:2.0"), "runtime")); + + DependencyManager result1 = manager.deriveChildManager(TestUtils.newCollectionContext(session, null, list1)); + DependencyManager result2 = manager.deriveChildManager(TestUtils.newCollectionContext(session, null, list2)); + + // Different lists should produce different managers (different managed data) + assertNotSame(result1, result2, "different lists must not share cached result"); + + // Single-entry cache: last call was list2, so list2 should hit + DependencyManager result2Again = + manager.deriveChildManager(TestUtils.newCollectionContext(session, null, list2)); + assertSame(result2, result2Again, "list2 should return memoized result (last seen)"); + + // list1 was evicted by list2, so calling with list1 recomputes (equal but not same object) + DependencyManager result1Again = + manager.deriveChildManager(TestUtils.newCollectionContext(session, null, list1)); + assertEquals(result1, result1Again, "list1 should produce equal result after eviction"); + } + + /** + * Verifies that the memoization cache for empty managed deps lists works correctly. + * This is the most common case (leaf artifacts without dependencyManagement). + */ + @Test + void testDeriveChildManagerMemoizationWithEmptyList() { + DependencyManager manager = new TransitiveDependencyManager(null); + + // Get to depth >= applyFrom + manager = manager.deriveChildManager(newContext()); + manager = manager.deriveChildManager(newContext()); + + // Use the SAME empty list (e.g. Collections.emptyList() is a singleton) + List emptyList = Collections.emptyList(); + + DependencyManager result1 = + manager.deriveChildManager(TestUtils.newCollectionContext(session, null, emptyList)); + DependencyManager result2 = + manager.deriveChildManager(TestUtils.newCollectionContext(session, null, emptyList)); + + // Both should return `this` (no new data + isApplied) and be memoized + assertSame(manager, result1, "empty list should return this"); + assertSame(result1, result2, "memoization should return same instance for empty list"); + } }