Optimize model building pipeline: defer Dependency.build() and reduce allocations

[gnodet]

Summary

• Add Builder getters to generated model classes (model.vm) — enables reading builder fields through the base chain without calling build(), supporting deferred materialization across pipeline stages
• Add *ToBuilder merger variants (merger.vm) — returns Builder instead of calling build(), letting callers accumulate changes across multiple merge passes without intermediate allocations
• Defer build() in DefaultDependencyManagementInjector — accumulates merged dependencies as Builder objects, calling build() only once per modified dependency at the end of the loop
• Optimize computeLocations() — replaces Stream.concat().collect(Collectors.toUnmodifiableMap(...)) with HashMap.putAll() + Map.copyOf(), and returns oldlocs directly when newlocs is empty (avoids unnecessary Map.copyOf since the base map is already immutable)
• Precompute locationsHashCode — cached at build time in the generated constructor, used by DefaultModelObjectPool.PoolKey for fast inequality checks before full map comparison
• Optimize PoolKey.locationsEqual() — uses direct getLocations() map comparison instead of iterating individual keys, with fast-path for both-empty maps (common for imported deps)
• Add addLocationInformation API to XmlReaderRequest — wired through DefaultModelXmlFactory for future use in skipping location tracking on imported BOM POMs

Context

JFR profiling on a 4,383-module reactor shows 37% CPU in ModelObjectPool (dependency interning), 18% in Dependency immutable builders, and 2.3 GB allocated in KeyValueHolder from computeLocations(). Each Dependency passes through ~7 pipeline stages, each calling build() which allocates a new immutable object + recomputes location maps + calls processObject().

This PR targets the three hottest code paths:

1. Redundant build() calls — deferred via Builder getters and ToBuilder merger variants
2. computeLocations() stream overhead — replaced with HashMap merge + early return
3. PoolKey location comparison — precomputed hash + direct map equality

Test plan

• mvn verify -B passes across all reactor modules (all tests green)
• FileToRawModelMergerTest updated to exclude new *ToBuilder methods from reflection-based override check
• Benchmark with JFR on large reactor to measure allocation reduction

🤖 Generated with Claude Code

[gnodet]

Benchmark Results (4383-module project, mvn validate)

Added commit 568b44b that eliminates unnecessary builder.build() calls in pipeline stages by using targeted builder getters instead of full model materialization.

Changes

model.vm: Added getBuilt*() methods for model-object list fields. When the builder field is null (unmodified), returns base.getXxx() at zero cost. When modified, builds just that field's builders — avoids full model build.

5 pipeline stages fixed to avoid builder.build():

• DefaultPluginConfigurationExpander → builder.getBuild() / builder.getReporting()
• DefaultModelNormalizer (2 methods) → builder.getBuild() / builder.getBuiltDependencies()
• DefaultDependencyManagementInjector → builder.getDependencyManagement() / builder.getBuiltDependencies()
• DefaultPluginManagementInjector → builder.getBuild()
• DefaultModelPathTranslator → builder.getBuild() / builder.getReporting()

Results (3 runs each, average)

Version	Average	vs rc-6
rc-6 baseline	22,114ms	—
PR #12652 (pool+sort)	18,617ms	-15.8%
PR #12653 (before this commit)	17,739ms	-19.8%
PR #12653 (with this commit)	17,956ms	-18.8%

Performance is within run-to-run variance. The fix eliminates the unnecessary full-model materializations while maintaining the same performance level. All 1130 tests pass (550 in maven-impl, 580 in maven-core).

[gnodet]

Updated benchmark: getModifiable() + two-field optimization

Following up on the previous fix — the pipeline stages now use getModifiable*() to work directly with mutable Dependency.Builder lists, mutating in place instead of round-tripping through immutable objects.

What changed (commit 443dcc5)

model.vm (two-field optimization):

• Builder now stores both Collection<X.Builder> (for getModifiable*()) and Collection<X> Imm (for the setter)
• The dependencies(Collection<Dependency>) setter stores the immutable list directly — zero wrapping overhead
• getModifiable*() lazily wraps from whichever source is set (immutable field, or base)
• build() constructor uses whichever field is set — immutable list path has no builder overhead
• forceCopy constructor stores immutable reference instead of wrapping

Pipeline stages (getModifiable pattern):

• DefaultModelNormalizer.injectDefaultValues — iterates getModifiableDependencies(), sets scope on builders in place
• DefaultModelNormalizer.mergeDuplicates — deduplicates builder list in place using management key computed from builder getters
• DefaultDependencyManagementInjector — new mergeManagementIntoBuilders() method merges managed dep fields (version, scope, type, classifier, systemPath, exclusions + locations) directly into Dependency.Builder objects

Cost model

Before (v1): each pipeline stage that touches deps does a full wrap + unwrap cycle:

builder.getBuiltDependencies()  → N unwrap allocations
builder.dependencies(newList)   → N wrap allocations

With 3 stages touching deps: 6N allocations per module (3 wraps + 3 unwraps)

After (v2): ONE lazy wrap + ONE final unwrap for the entire pipeline segment:

builder.getModifiableDependencies()  → lazy wrap (cached across stages)
  stage 1: modify builders in place   → 0 allocations
  stage 2: modify builders in place   → 0 allocations
  stage 3: modify builders in place   → 0 allocations
builder.build()                      → final unwrap

Total: 2N allocations per module (1 wrap + 1 unwrap)

Benchmark results (4383-module project, mvn validate --threads 1)

Build	Average	vs rc-6	vs PR #12652
rc-6 baseline	20,234ms	—	—
PR #12652 (pool+sort)	14,936ms	-26.2%	—
PR #12653 v1 (getBuilt)	14,784ms	-26.9%	-1.0%
PR #12653 v2 (getModifiable)	13,385ms	-33.8%	-10.4%

The v2 optimization delivers a clear 10% improvement over PR #12652 alone, and 34% over rc-6.

Branch: optimize-model-builder-pipeline-fix (commits 568b44b861 + 443dcc5b51)

[gnodet]

JFR-guided optimization (v3): withImportedFrom + ConcurrentHashMap.newKeySet

JFR profiling on the 4383-module benchmark identified two remaining hotspots, now fixed in commit d30efb5:

CPU profile comparison (v2 → v3)

Component	v2 samples	v2 %	v3 samples	v3 %	Change
DependencyMgmtImporter	442	43.0%	143	20.4%	-68%
↳ updateWithImportedFrom	354	34.4%	44	6.3%	-88%
CopyOnWriteArraySet	68	6.6%	0	0.0%	-100%
ModelValidator	71	6.9%	66	9.4%	same
I/O (XML, Zip, Jar)	78	7.6%	86	12.3%	same
Other	369	35.9%	406	57.9%	same
Total CPU samples	1028		701		-32%

What was fixed

1. updateWithImportedFrom (was 35% of CPU)

Every BOM-imported dependency was rebuilt through Dependency.newBuilder(dep, true).importedFrom(loc).build() — triggering forceCopy (wraps all sub-lists into builders), build (unwraps back to immutable), and object pool interning — just to set one metadata field.

Added withImportedFrom(InputLocation) + private copy constructor to model.vm. Shares all model fields by reference, bypasses Builder and object pool entirely.

2. CopyOnWriteArraySet (was 6.6% of CPU)

ConsumerPomArtifactTransformer.deferDeleteFile() added to a CopyOnWriteArraySet<Path> → O(n) indexOfRange scan + array copy per add. With 4383 modules: O(n²). Replaced with ConcurrentHashMap.newKeySet() → O(1).

Wall-clock benchmark (4383 modules, mvn validate)

Wall-clock difference between v2 and v3 is within noise (~0.3s on a 14s benchmark) because:

• BOM import runs once on the root POM, not on the critical path for parallel child processing
• The parallel 4383-module build is I/O and scheduling dominated

The cumulative improvement from rc-6 baseline remains ~27% faster.

[gnodet]

New commit: Store single model-object fields as sub-builders

Added commit 4ade9fe that extends the deferred-build pattern from list-of-model-object fields to single model-object fields (multiplicity=1).

What changed

model.vm — single model-object fields now use sub-builders:

Builder classes now store fields like Build, Reporting, DependencyManagement, Parent, Scm, Organization, Prerequisites, IssueManagement, CiManagement, DistributionManagement, PluginManagement, Activation, ActivationOS, ActivationFile, ActivationProperty, BuildBase, Relocation, Site, DeploymentRepository, RepositoryPolicy as X.Builder instead of immutable X. This means:

Before	After
Build build; in Builder	Build.Builder build; in Builder
Setter stores immutable directly	Setter wraps: X.newBuilder(val, false)
Getter returns field directly	Getter calls .build() on sub-builder
No mutable access	New getModifiable*() returns X.Builder
build() short-circuit: identity check	build() short-circuit: null check
forceCopy copies immutable ref	forceCopy wraps into sub-builder

Pipeline stages updated to use getModifiable*():

• DefaultModelPathTranslator: mutates Build.Builder / Reporting.Builder sub-builders in place — eliminates Build.newBuilder(build).xyz().build() intermediate allocations (10+ field mutations per call)
• DefaultModelNormalizer: sets deduplicated plugins directly on Build.Builder via getModifiableBuild().plugins(...)
• DefaultPluginConfigurationExpander: mutates Build.Builder and nested PluginManagement.Builder sub-builders in place
• DefaultPluginManagementInjector: extracts merged plugin list and sets it directly on Build.Builder, refactored to expose mergeManagedPluginList() returning just the list

Impact

Each pipeline stage that modifies Build, Reporting, or PluginManagement now avoids:

1. Materializing the immutable sub-object (no Build.newBuilder(build))
2. Building a new immutable (no .build())
3. Wrapping it back in the Model.Builder setter (no builder.build(newBuild))

On a 4383-module reactor, each module passes through ~7 pipeline stages. Previously, every stage that touched Build would: get immutable → create Builder → set fields → build() → set back on Model.Builder. Now they: get sub-builder → set fields. One allocation instead of three per stage.

🤖 Generated with Claude Code

[gnodet]

LGTM ✅ — Well-structured optimization PR that defers Model.build() calls and reduces allocations in the model-building pipeline. All changes maintain semantic equivalence with the original code.

Key observations:

• The SPI default methods on the 10 interfaces follow a clean build-then-reset bridge pattern, providing backwards compatibility for existing implementations.
• The computeLocations() optimization removing Map.copyOf(oldlocs) is safe: base.locations is always unmodifiable.
• The hash code change in DefaultModelObjectPool is correct: getLocationKeys() was redundant with getLocations() since Map.hashCode() inherently incorporates the key set.
• The DefaultProfileInjector.injectProfiles(Builder, ...) is correct: mergeModelBase operates only on ModelBase-level fields and doesn't touch the Build field.
• The mixin loop in readEffectiveModel() correctly uses getBuiltMixins() and builder.getProperties() reflects merged state after each assembleModelInheritance call.
• The null guard on importMgmts is semantically equivalent to the old code.

Minor note: In DefaultModelBuilder.java around line 2466, the new interpolateModel(Builder, ...) method uses builder.getProperties() (post-interpolation) for parent version interpolation, while the original used model.getProperties() (pre-interpolation). This produces identical results because Interpolator.interpolate() performs recursive resolution, but a brief inline comment explaining this equivalence could help future readers.

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]

Well-structured optimization that threads Model.Builder through the pipeline to avoid intermediate immutable allocations. The 40% benchmark improvement is substantive, and the changes preserve semantic equivalence.

Key design observations:

• The 10 SPI default methods follow a clean build-then-reset bridge pattern that maintains full backward compatibility
• The computeLocations() optimization (returning oldlocs directly when newlocs.isEmpty()) is safe — base.locations is always unmodifiable
• Hash code inlining and precomputed locationsHashCode are both mathematically correct

One design note (non-blocking):

The getModifiable*() methods for single model-object fields (e.g., getModifiableDependencyManagement()) unconditionally create an empty sub-builder even when the base value is null. This sets the builder's field to non-null, which means build() will produce a non-null but empty sub-object where null existed before. All current call sites are properly guarded with null checks on the getter before calling getModifiable*(), and the methods are annotated @Nonnull to document this contract — so this is not a bug today, but a subtle footgun for future callers who might call getModifiable*() as a "peek" without realizing it changes the observable state.

Own PR — LGTM.

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

Claude Code on behalf of Guillaume Nodet

[gnodet]

Review — well-structured model-building pipeline optimization

Large but well-organized optimization that defers Model.build() calls and reduces allocations by threading a Model.Builder through pipeline stages. The builder-accepting SPI additions via default methods are backward-compatible with third-party implementations.

Key strengths:

• computeLocations() optimization replacing Stream.concat().collect() with HashMap.putAll() + Map.copyOf() is correct; early return of oldlocs is safe since the base map is already immutable
• locationsHashCode precomputation with fast inequality checks before full map comparison is well-targeted
• Inlined dependencyHashCode avoids varargs Object[] allocation on every call
• Removal of getLocationKeys() from equality/hash is correct since getLocations().equals() already covers both keys and values

Minor observations (all non-blocking)

Pre/post-interpolation properties — The new interpolateModel(Builder) uses builder.getProperties() after interpolation for parent version resolution, while the original uses pre-interpolation properties. This is actually more correct (fully resolved values) but is a subtle semantic change worth noting in the commit message.

Pre-existing expandReport bug — Both old and new expandPluginConfiguration drop the expandReport() return value. Not a regression from this PR.

Builder cache bypass — The builder variant of injectProfiles correctly skips the CACHE since builders are mutated in-pipeline where each instance is unique and transient.

Theoretical list rebuild — The build() short-circuit for model-object lists could create an unnecessary container if getModifiable*() was called without modification. Doesn't occur in practice since that method is only called with intent to modify.

---

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

Claude Code on behalf of Guillaume Nodet

[gnodet]

Integration test failure after merge conflict resolution:

MavenITmng4403LenientDependencyPomParsingTest.testit:67 
expected: <[org.apache.maven.its.mng4403:a:jar:0.1, org.apache.maven.its.mng4403:b:jar:0.1, org.apache.maven.its.mng4403:c:jar:0.1]>
but was: <[org.apache.maven.its.mng4403:a:jar:0.1, org.apache.maven.its.mng4403:b:jar:0.1]>

The conflict was in DefaultModelObjectPool.java where the PR used dep.getLocationsHashCode() (a precomputed hash) while master now uses getLocationKeys() + locationsHashCode() helper. I resolved by taking master's approach, but this may have broken the deduplication logic causing dependency c to be incorrectly deduplicated.

@gnodet — please review the conflict resolution.