Fix #2013: optimize TransitiveDependencyManager performance

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<DependencyNode> children = args.pool.getChildren(speculativeKey);
+        if (children != null) {
+            child.setChildren(children);
+            return;
+        }
 
-        List<DependencyNode> children = args.pool.getChildren(key);
-        if (children == null) {
-            boolean skipResolution = args.skipper.skipResolution(child, parentContext.parents);
-            if (!skipResolution) {
-                List<DependencyNode> 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<DependencyNode> 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);
         }
     }
 

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<Exclusion> 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.
+     * <p>
+     * Scenario (from <a href="https://github.com/apache/maven-resolver/issues/2013">#2013</a>):
+     * <pre>
+     *   root
+     *   ├── b      → c → d
+     *   └── b-alt  → c → d   (c is a shared transitive dependency)
+     * </pre>
+     * 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.
+     * <p>
+     * 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/"));

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

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

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

maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_d_1.0.ini

@@ -0,0 +1 @@
+[dependencies]

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