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

maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/providers/FileLockNamedLockFactory.java

@@ -29,6 +29,9 @@
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.nio.file.StandardOpenOption;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
+import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 
@@ -99,23 +102,56 @@ public class FileLockNamedLockFactory extends NamedLockFactorySupport {
 
     private static final long SLEEP_MILLIS = Long.parseLong(System.getProperty(SYSTEM_PROP_SLEEP_MILLIS, "50"));
 
+    /**
+     * Maximum number of idle (not currently locked) FileChannels to keep open for reuse. Keeping channels open
+     * avoids repeated {@code open}/{@code creat} syscalls, but each open channel consumes a file descriptor.
+     * On systems with low FD limits (e.g., macOS defaults to 256), large reactor builds with thousands of
+     * unique artifacts can exhaust the limit. This cap bounds idle channel retention; channels for actively
+     * held locks are never evicted.
+     *
+     * @configurationSource {@link System#getProperty(String, String)}
+     * @configurationType {@link java.lang.Integer}
+     * @configurationDefaultValue 200
+     */
+    public static final String SYSTEM_PROP_MAX_CACHED_CHANNELS = "aether.named.file-lock.maxCachedChannels";
+
+    private static final int MAX_CACHED_CHANNELS =
+            Integer.parseInt(System.getProperty(SYSTEM_PROP_MAX_CACHED_CHANNELS, "200"));
+
     private final ConcurrentMap<NamedLockKey, FileChannel> fileChannels;
 
+    /**
+     * LRU pool of idle (unlocked) channels available for reuse. Access-ordered: the least recently used
+     * entry is evicted first when the pool exceeds {@link #MAX_CACHED_CHANNELS}. Guarded by its own
+     * monitor; never held while performing I/O.
+     */
+    private final LinkedHashMap<NamedLockKey, FileChannel> idleChannels;
+
     public FileLockNamedLockFactory() {
         this.fileChannels = new ConcurrentHashMap<>();
+        this.idleChannels = new LinkedHashMap<>(64, 0.75f, true); // access-order
     }
 
     @Override
     protected NamedLockSupport createLock(final NamedLockKey key) {
         Path path = Paths.get(URI.create(key.name()));
-        FileChannel fileChannel = fileChannels.computeIfAbsent(key, k -> openFileChannel(key, path));
-        if (!fileChannel.isOpen()) {
-            // Channel was closed externally (I/O error, NFS hiccup, etc.). Evict the stale entry
-            // and open a fresh one. remove(key, fileChannel) is atomic: it only removes if the
-            // value is still this exact (stale) instance, avoiding races with other threads that
-            // may have already replaced it.
-            fileChannels.remove(key, fileChannel);
+        // Try to reclaim an idle channel first (avoids open syscall)
+        FileChannel fileChannel;
+        synchronized (idleChannels) {
+            fileChannel = idleChannels.remove(key);
+        }
+        if (fileChannel != null && fileChannel.isOpen()) {
+            fileChannels.put(key, fileChannel);
+        } else {
             fileChannel = fileChannels.computeIfAbsent(key, k -> openFileChannel(key, path));
+            if (!fileChannel.isOpen()) {
+                // Channel was closed externally (I/O error, NFS hiccup, etc.). Evict the stale entry
+                // and open a fresh one. remove(key, fileChannel) is atomic: it only removes if the
+                // value is still this exact (stale) instance, avoiding races with other threads that
+                // may have already replaced it.
+                fileChannels.remove(key, fileChannel);
+                fileChannel = fileChannels.computeIfAbsent(key, k -> openFileChannel(key, path));
+            }
         }
         return new FileLockNamedLock(key, fileChannel, this);
     }
@@ -157,10 +193,30 @@ private FileChannel openFileChannel(NamedLockKey key, Path path) {
 
     @Override
     protected void destroyLock(final NamedLock namedLock) {
-        // Keep the FileChannel open in the fileChannels map for reuse by future createLock() calls.
-        // Opening a FileChannel is a syscall (open/creat) that shows up as a hotspot when locks are
-        // acquired and released frequently (e.g., per-artifact resolution in primed builds).
-        // Channels are closed on factory shutdown via doShutdown().
+        NamedLockKey key = namedLock.key();
+        FileChannel channel = fileChannels.remove(key);
+        if (channel == null) {
+            return;
+        }
+        // Move the channel to the idle pool for reuse by future createLock() calls.
+        // Evict the least recently used idle channel if the pool is full.
+        FileChannel evicted = null;
+        synchronized (idleChannels) {
+            idleChannels.put(key, channel);
+            if (idleChannels.size() > MAX_CACHED_CHANNELS) {
+                Iterator<Map.Entry<NamedLockKey, FileChannel>> it =
+                        idleChannels.entrySet().iterator();
+                evicted = it.next().getValue();
+                it.remove();
+            }
+        }
+        if (evicted != null) {
+            try {
+                evicted.close();
+            } catch (IOException e) {
+                logger.warn("Failed to close evicted file channel", e);
+            }
+        }
     }
 
     @Override
@@ -173,5 +229,15 @@ protected void doShutdown() {
             }
         }
         fileChannels.clear();
+        synchronized (idleChannels) {
+            for (FileChannel channel : idleChannels.values()) {
+                try {
+                    channel.close();
+                } catch (IOException e) {
+                    logger.warn("Failed to close idle file channel", e);
+                }
+            }
+            idleChannels.clear();
+        }
     }
 }