Fix #2013: optimize TransitiveDependencyManager performance by gnodet · Pull Request #2014 · apache/maven-resolver

gnodet · 2026-07-24T23:44:20Z

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)

…ta is collected AbstractDependencyManager.deriveChildManager() now returns `this` when no new management data (versions, scopes, optionals, local paths, exclusions) was collected at the current depth AND management is already being applied (depth >= applyFrom). This avoids creating distinct-but-semantically-equal DependencyManager instances that defeat the BfDependencyCollector pool cache — the pool key includes the manager, so unique managers cause pool misses, which lets the skipper prune subtrees that should have been served from the cache. The isApplied() guard is necessary: at depth < applyFrom, returning `this` would freeze the depth counter and prevent management rules from ever being applied to descendants. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

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

…tests Add two tests suggested by review on PR #2014: 1. testDeriveChildManagerReusesInstanceWithDefaultManager — verifies the optimization also works with DefaultDependencyManager (applyFrom=0), where instance reuse fires at the very first derivation. 2. testNearerToRootWinsAfterOptimizationReusesInstance — documents that the "nearer-to-root wins" invariant holds after the optimization reuses an instance: root's version management at depth 1 still wins when a deeper POM tries to override the same key, because containsManagedVersion finds it in ancestors and skips collection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gnodet · 2026-07-28T07:54:49Z

Thanks for the review! Both suggestions addressed in 0802960:

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).
"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:

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.
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.

+        // 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());


+        if (managedVersions == null
+                && managedScopes == null
+                && managedOptionals == null
+                && managedLocalPaths == null
+                && managedExclusions == null
+                && isApplied()) {
+            return this;
+        }


JFR profiling on a 4,383-module project showed three additional hotspots beyond the deriveChildManager instance-reuse fix: - RelocatedArtifact.getArtifactId (9% CPU): Key.equals() called artifact.getArtifactId() through virtual dispatch on every HashMap comparison. Fix: cache the four GACE coordinate strings directly in Key at construction time, eliminating the delegation overhead. - AbstractMap.equals (9% CPU): MMap.DoneMMap.equals() went straight to HashMap.equals() without checking identity or hashCode first. Fix: add identity short-circuit and pre-computed hashCode fast-rejection before the expensive deep map equality. - AbstractDependencyManager.equals: compared the path ArrayList (O(depth)) before the cheap depth int check. Fix: add hashCode fast-rejection as the first guard, check depth before path, and move path comparison last. Combined with the instance-reuse optimization, these changes eliminate all resolver-related CPU hotspots, reducing build time from 793s to 19s on the test project (42x speedup). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gnodet

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

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

Key class — caches GACE coordinate strings at construction to avoid repeated virtual dispatch through RelocatedArtifact wrappers.
MMap.DoneMMap.equals() — adds identity short-circuit and hashCode fast-rejection before expensive deep-map equality.
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

Replace the ArrayList<AbstractDependencyManager> path field with a single AbstractDependencyManager parent pointer, forming a linked list from leaf toward root. This eliminates three O(depth) costs that dominated JFR profiles on large multi-module projects with TransitiveDependencyManager: 1. deriveChildManager() no longer copies the entire ancestor list — siblings now share the same parent reference (O(1) derive). 2. hashCode is cascading: each level incorporates its parent's pre-computed hash, so a single int comparison reflects the entire ancestor chain without walking it. 3. equals() compares parents recursively, but each level is hash-guarded and shared parents (same identity) short-circuit immediately via the this==obj check. Lookup semantics are preserved: the parent chain is walked from leaf toward root, keeping the last hit (closest to root) to maintain "nearest to root wins" precedence. Since containsManagedXxx() blocks duplicate entries during derive, there is typically at most one hit per key in the chain. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

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

gnodet

Re-review after new commit 7166e29.

Two well-layered, correctness-preserving optimizations:

Speculative pool key check in BfDependencyCollector.doRecurse() — tries the pool with parentContext.depManager first (betting deriveChildManager returns this), falling back to recomputing the key when the identity check fails. Null-parent case handled correctly.
IdentityHashMap memoization cache in AbstractDependencyManager for deriveChildManager results — sound because the result depends only on the managed dependencies list and current manager state (fixed per-instance). Keying on getManagedDependencies() with identity-based lookup is appropriate given DataPool's list interning. A miss is only a performance concern, not a correctness issue.

Thread safety of the IdentityHashMap is correctly guaranteed by the BFS collector's single-threaded traversal loop. The three new tests are well-structured: identity-based cache hits, cache discrimination between different lists, and the common empty-list case. assertSame vs assertEquals usage is precisely correct.

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

Use a single-entry (last-seen) memoization cache in deriveChildManager to skip the O(managedDependencies) loop when the same interned list is seen consecutively — the common case in BFS dependency collection where siblings share a parent descriptor. A single-entry cache uses constant memory (two fields) and avoids the retention issue of an unbounded IdentityHashMap that would prevent GC of derived manager subtrees. In BfDependencyCollector.doRecurse(), perform a speculative pool cache check using the parent manager BEFORE calling deriveChildManager. When the derived manager equals the parent (the common case for transitive deps without dependencyManagement), the speculative key matches and we skip derivation entirely on pool hits.

gnodet

Re-review after new commit 6205d17.

Two refinements to the optimization stack:

Speculative pool key check in BfDependencyCollector — avoids calling deriveChildManager on cache hits. When deriveChildManager returns this (common case), the speculative key matches. When the manager changes, falls back correctly. Null case handled properly.
Single-entry memoization cache (lastManagedDeps / lastDeriveResult) replaces the unbounded IdentityHashMap — well-suited to BFS traversal where consecutive siblings share the same interned managed-dependency list. Using identity (==) is correct given DataPool's list interning guarantee. Placement after the isDerived() guard ensures correctness at all depths.

Choosing a single-entry cache over an unbounded map is a good design decision — avoids retaining every derived DependencyManager and the dependency subtrees they reference, trading a small miss rate for bounded memory.

Three new test methods provide good coverage: identity-based cache hits, cache misses on different list objects, single-entry eviction semantics, and the empty-list 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

…ontainsManaged* and fix reuse optimization The containsManagedVersion/Scope/Optional/LocalPath methods were missing a check against this.managedVersions (the current instance's own management data). This regression was introduced in d4035d3 when the parameter shadowed the instance field. Additionally, the "reuse this" optimization (returning this when no new management data is collected) was hiding management data from children: getManagedVersion() only walks the parent chain, so a reused instance's own rules became invisible. Now the optimization only fires when both the context AND the instance carry no management data, ensuring that management rules always propagate through the parent chain. Fixes the testDependencyManagement_TransitiveDependencyManager and testDependencyManagement_DefaultDependencyManager test failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Pre-build cumulative HashMaps at construction time that flatten the full parent chain into a single map per property (versions, scopes, optionals, localPaths, exclusions). When the parent has no per-level data, the child shares the parent's reference (zero copy). This eliminates all O(depth) parent chain walks from containsManaged* and getManaged* lookups, making every lookup O(1) regardless of dependency tree depth. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gnodet

Re-review after commits 071bbe0 and the ancestor-maps refactor.

Two important changes:

Correctness fix (071bbe0) — the containsManagedXxx() guards now also check this.managedVersions (and peers), fixing a bug where the reuse optimization could hide own management data from children. The corresponding test update (flipping assertSame to assertNotSame for DefaultDependencyManager depth 1→2 with parent management) correctly validates this.
O(1) cumulative ancestor HashMaps — replaces O(depth) parent-chain walks in getManagedXxx() with pre-built cumulative maps constructed incrementally at construction time. Zero-copy sharing when the parent has no per-level data. Since containsManagedXxx() prevents duplicate entries during derivation, the HashMap has at most one entry per key, so putAll ordering is irrelevant in practice — preserving "nearest to root wins" semantics.

Exclusion additive merge in mergeAncestorExclusions() correctly uses HashMap.merge() to combine collections from multiple levels, and getManagedExclusions() returns shared references safely since the caller wraps them in a new LinkedHashSet.

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

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gnodet marked this pull request as ready for review July 24, 2026 23:50

gnodet requested a review from cstamas July 24, 2026 23:50

gnodet commented Jul 27, 2026

View reviewed changes

gnodet commented Jul 28, 2026

View reviewed changes

elharo requested a review from Copilot July 29, 2026 11:00

elharo approved these changes Jul 29, 2026

View reviewed changes

Copilot AI reviewed Jul 29, 2026

View reviewed changes

Copilot started reviewing on behalf of elharo July 29, 2026 11:39 View session

gnodet commented Jul 31, 2026

View reviewed changes

gnodet changed the title ~~Fix #2013: reuse DependencyManager instance to restore pool cache transparency~~ Fix #2013: optimize TransitiveDependencyManager performance Jul 31, 2026

gnodet commented Aug 1, 2026

View reviewed changes

gnodet force-pushed the fix/2013 branch from 7166e29 to 6205d17 Compare August 2, 2026 09:05

gnodet commented Aug 2, 2026

View reviewed changes

gnodet and others added 2 commits August 2, 2026 14:38

gnodet commented Aug 2, 2026

View reviewed changes

Retrigger CI: Jenkins flaky failure unrelated to changes

936dcf5

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix #2013: optimize TransitiveDependencyManager performance - #2014

Fix #2013: optimize TransitiveDependencyManager performance#2014
gnodet wants to merge 8 commits into
masterfrom
fix/2013

gnodet commented Jul 24, 2026 •

edited

Loading

Uh oh!

gnodet left a comment

Uh oh!

gnodet commented Jul 28, 2026

Uh oh!

gnodet left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

gnodet left a comment

Uh oh!

gnodet left a comment

Uh oh!

gnodet left a comment

Uh oh!

gnodet left a comment

Uh oh!

gnodet left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

gnodet commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Changes

1. Instance reuse in deriveChildManager() (ec77f73)

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

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

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

Benchmark Results

Notes

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet commented Jul 28, 2026

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gnodet commented Jul 24, 2026 •

edited

Loading

1. Instance reuse in `deriveChildManager()` (`ec77f73`)

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

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

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