Fix #2013: optimize TransitiveDependencyManager performance

[gnodet]

Problem

TransitiveDependencyManager (deriveUntil=Integer.MAX_VALUE) creates a new child manager at every node in the dependency graph. On large multi-module projects (4,383 modules), JFR profiling showed this consuming 28% of total CPU time — methods like deriveChildManager, MMap.done(), Key.equals(), and AbstractMap.equals() that have zero samples under ClassicDependencyManager.

Changes

Four layered optimizations, each addressing a distinct hotspot:

1. Instance reuse in deriveChildManager() (ec77f73)

When no new management data is collected (the common case for transitive POMs without <dependencyManagement>), return this instead of creating a new instance. This restores pool cache transparency — the BF collector's DataPool.GraphKey includes the manager, so semantically-equal-but-distinct managers were defeating the cache and preventing subtree reuse.

2. Key class: cache GACE coordinate strings (69d7e39)

The Key inner class stored an Artifact reference and called getGroupId()/getArtifactId() etc. on every equals(). Through RelocatedArtifact (a delegation wrapper), each getter did a null-check + virtual dispatch — 9% of CPU in the JFR profile. Now caches the four coordinate strings eagerly in the constructor.

3. MMap.DoneMMap.equals(): identity + hashCode short-circuit (69d7e39)

Added this == o identity check, narrowed instanceof to DoneMMap, and added pre-computed hashCode mismatch rejection before the expensive delegate.equals() (which triggers Key.equals() for every map entry).

4. Replace ArrayList path with parent pointer — cons-list (52d9d4b)

The previous design stored all ancestors in an ArrayList<AbstractDependencyManager> that was O(depth) copied on every deriveChildManager(). The equals() method compared the entire list element-by-element, each element comparing its own maps — O(depth²) worst case.

Replaced with a single AbstractDependencyManager parent pointer (cons-list). Siblings share the same parent reference. Key properties:

• O(1) derive — no list copy, just set parent = this
• Cascading hashCode — each level incorporates its parent's pre-computed hash, so a single int comparison reflects the entire ancestor chain
• Hash-guarded recursive equals — parent comparison is recursive but each level short-circuits on hashCode mismatch, and shared parents (same == identity) short-circuit immediately

Benchmark Results

Test project: maven-multiproject-generator (4,383 modules), mvn validate -T1:

Configuration	Time	vs baseline
Maven 4 rc-6 unpatched (TransitiveDependencyManager)	793s	1×
Maven 4 + -Dmaven.resolver.dependencyManagerTransitivity=false	19s	42×
Maven 4 + this PR (all optimizations)	~3s	~270×

GC pressure also reduced: 10 → 8 GC events, 59ms → 43ms total pause time.

Notes

• All three manager subclasses updated: TransitiveDependencyManager, ClassicDependencyManager, DefaultDependencyManager
• Lookup semantics preserved: parent chain walked from leaf toward root, last hit = nearest-to-root wins
• managedLocalPaths intentionally excluded from equals()/hashCode() (unchanged)
• Full resolver test suite passes (BF + DF collectors, all three managers)

[gnodet]

Clean, well-targeted fix for the cache-transparency bug described in #2013. The optimization to reuse the DependencyManager instance when no new management data is collected is semantically correct — it's placed at exactly the right point in deriveChildManager (after all management-data collection but before newInstance()), and the isApplied() guard prevents freezing depth below the applyFrom threshold.

A couple of minor, non-blocking observations:

• The unit test only covers TransitiveDependencyManager. DefaultDependencyManager (applyFrom=0) also benefits from the optimization and could be tested with a similar assertSame/assertNotSame pattern. The existing testDefault test indirectly covers management correctness, so this is a minor coverage gap.
• An additional test where depth-1 context contributes management data (making this.managedVersions non-null), then the next depth has an empty context (optimization fires), and a subsequent depth has management data for the same key would explicitly document the "nearer-to-root wins" invariant — analysis of getManagedVersion confirms it iterates the path from root (first match wins), so this is functionally correct already.

Thread safety is fine: the optimization returns this without mutation, and newInstance creates new ArrayList copies of this.path, so concurrent calls from different BF collector branches are safe.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[gnodet]

Thanks for the review! Both suggestions addressed in 0802960:

1. DefaultDependencyManager coverage — added testDeriveChildManagerReusesInstanceWithDefaultManager which verifies the optimization fires at the very first derivation with applyFrom=0 (assertSame), and that it correctly creates new instances when management data is present (assertNotSame).

2. "Nearer-to-root wins" invariant — added testNearerToRootWinsAfterOptimizationReusesInstance which explicitly documents the invariant: root contributes x:1.0 at depth 1, the optimization reuses the instance at depth 2→3, then a deeper POM tries to override x with 2.0 — the test verifies containsManagedVersion finds it in ancestors, skips collection (so the optimization fires again), and getManagedVersion still returns 1.0.

[gnodet]

Re-review after new commit (0802960).

The new commit cleanly addresses both suggestions from the previous review:

1. DefaultDependencyManager coverage — new test verifies the optimization fires at the very first derivation (depth 0→1) since applyFrom=0 makes isApplied() true immediately. The assertSame/assertNotSame assertions are accurate.

2. Nearer-to-root-wins invariant test — verifies that when a depth-1 context contributes management data and the next depth has an empty context (optimization reuses this), a subsequent depth with management data for the same key correctly inherits the nearer-to-root value. The assertion chain through containsManagedVersion → skip local collection → all five managed variables stay null is sound.

Test coverage for the optimization is now comprehensive across both manager implementations, with and without management data, and across the applyFrom boundary. No issues found.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes #2013 by making AbstractDependencyManager.deriveChildManager() reuse the same DependencyManager instance when no new management data is introduced (and management is already applied), preventing BF collector pool-cache key churn and restoring cache transparency.

Changes:

• Added an instance-reuse fast path in AbstractDependencyManager.deriveChildManager() when derivation is a semantic no-op at applied depths.
• Added unit tests validating reuse behavior across TransitiveDependencyManager and DefaultDependencyManager, plus a “nearer-to-root wins” invariant test.
• Added an integration test scenario (with new artifact-description fixtures) reproducing the BF pool-cache transparency issue and asserting consistent graph structure.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java	Reuses manager instances for no-op derivations at applied depths to avoid BF pool-cache misses.
maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/manager/DependencyManagerTest.java	Adds unit tests for instance-reuse behavior and precedence invariants.
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/collect/bf/BfWithSkipperDependencyCollectorTest.java	Adds integration test ensuring pool-cache transparency with TransitiveDependencyManager.
maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_root_1.0.ini	Adds root fixture for diamond-like dependency graph.
maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b_1.0.ini	Adds b fixture depending on c.
maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b-alt_1.0.ini	Adds b-alt fixture depending on c.
maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_c_1.0.ini	Adds c fixture depending on d.
maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_d_1.0.ini	Adds d fixture with no dependencies.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gnodet]

Re-review after new commit 69d7e39 ("Optimize Key, MMap, and AbstractDependencyManager equality checks").

Three well-targeted performance optimizations backed by JFR profiling data:

1. Key class — caches GACE coordinate strings at construction to avoid repeated virtual dispatch through RelocatedArtifact wrappers.
2. MMap.DoneMMap.equals() — adds identity short-circuit and hashCode fast-rejection before expensive deep-map equality.
3. AbstractDependencyManager.equals() — adds hashCode fast-rejection and reorders comparisons to check cheap int (depth) before expensive ArrayList (path).

All three are correct: hashCode is pre-computed in each case so fast-rejection is sound, the DoneMMap instanceof narrowing is safe (stored maps are always null or DoneMMap via deriveChildManager/newInstance), and the Key.toString() format change only affects debug output. The claimed 42× speedup (793s → 19s) on a 4,383-module project is impressive.

Previous commits remain unchanged and were already approved. No issues found.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[gnodet]

Re-review after new commit 52d9d4b ("Replace ArrayList path with parent pointer (cons-list)").

Clean refactor that replaces the O(depth) ArrayList<AbstractDependencyManager> path with an O(1) parent-pointer cons-list. Key observations:

• Lookup semantics preserved: The old code iterated root-to-leaf with early return (first hit = nearest root wins). The new code walks leaf-to-root with overwrite (last hit = nearest root wins). Both correctly implement "nearest to root wins." Additionally, containsManagedXxx() guards during derive prevent duplicate keys in the chain, making walk direction irrelevant in practice.

• Cascading hash is correct: Uses parent.hashCode (pre-computed field) so a single int comparison reflects the entire ancestor chain.

• Recursive equals is safe: this == obj identity check short-circuits for shared parents (siblings), and the hash guard rejects most mismatches at the first level. Stack overflow risk is theoretical only — Maven dependency trees don't reach problematic depth, and the instance-reuse optimization prevents chain growth at empty-management levels.

• Natural fit: Siblings already share the same parent after the instance-reuse optimization from the earlier commit, making identity-based equality effectively O(1) in the common case.

No issues found.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet