Reduce unnecessary immutable object allocations in model building

[gnodet]

Summary

Maven 4's immutable model pattern creates excessive intermediate objects during effective model construction. This PR addresses the root causes through optimizations in both the Modello-generated templates and the hand-written pipeline code, plus builder-passing SPI changes.

Changes

merger.vm — reduce false-positive change detection (highest impact)

• String fields: use Objects.equals() instead of != reference check — prevents false positives when parent and child define the same string value as different object instances
• Properties/Map: add equality check after merging — skip builder setter when merged result equals target, avoiding unnecessary immutable object rebuilds
• List merge: detect no-op merges by comparing content after merge and return original target reference, preserving Builder.build() short-circuit
• Composite fields (mult=1): already had merged != target.getField() identity guard (unchanged)
• Boolean/int fields: already had primitive value comparison (unchanged)

DefaultModelNormalizer: avoid always-build pattern

• mergeDuplicates(): only create builder and call build() when duplicate plugins/dependencies are actually found
• injectDefaultValues(): only build when scope injection changes something
• injectPlugin(): skip rebuild when plugin dependencies don't need scope injection

DefaultProfileInjector: guard sub-object rebuilds

• Add newBuild != build identity check before setting Build on Model.Builder
• Fix multi-profile injection: process all profiles into intermediate models, then transfer accumulated result to the passed builder (prevents loss of earlier profiles' properties when the last profile lacks corresponding fields)

DefaultModelBuilder: replace nested with() chains*

• Replace patterns like model.withParent(parent.withRelativePath(x)) (2 full immutable copies) with single-builder patterns
• Consolidate withProfiles(List.of()).withParent(null) into single builder call
• Add identity check before withPomFile()

DefaultPluginConfigurationExpander: skip unchanged rebuilds

• Check if plugin expansion actually changed anything before rebuilding Build/Model objects
• Original code unconditionally called model.withBuild(build) even when nothing expanded

DefaultModelUrlNormalizer: guard normalization

• Only set URL/SCM/Site on builder when normalization actually changes the value

ImmutableCollections.copy(Map): detect JDK unmodifiable maps

• copy(Map) only recognized custom AbstractImmutableMap as already-immutable
• Now also recognizes JDK internal unmodifiable maps (Map.of(), Map.copyOf()) to avoid redundant wrapping

SPI changes — Pass Model.Builder through pipeline

• Changed ProfileInjector.injectProfiles() and InheritanceAssembler.assembleModelInheritance() to accept Model.Builder parameter, avoiding build-then-rebuild round-trips
• DefaultModelNormalizer, DefaultPluginConfigurationExpander, DefaultModelUrlNormalizer also adapted to builder-passing pattern

Timing log

• Added [TIMING] INFO log in buildBuildPom() that reports loadFromRoot and buildEffectiveModels durations for the reactor

Impact

These changes target the ~16-20 intermediate Model objects created per POM during buildEffectiveModel() / readEffectiveModel(). The build() short-circuit (if base != null && all fields unchanged → return base) was being defeated by false-positive field assignments in the merger, causing unnecessary object allocation chains (Model → Build → Plugin → Dependency → ...).

Benchmark results — reactor model building (Apache Camel, 676 modules)

Measured with [TIMING] log in buildBuildPom(), mvn validate -o, interleaved runs, outliers excluded:

Metric	Baseline (maven-4.0.x)	Optimized (this PR)	Change
buildEffectiveModels	4012 ms avg	3867 ms avg	-3.6%
total (load+build)	4704 ms avg	4447 ms avg	-5.5%

Benchmark results — full build GC pressure (Apache Camel, ~1000 modules, mvn validate -o)

Metric	Baseline	Optimized	Change
Wall clock time	82.0s avg	81.4s avg	-0.7% (within noise)
GC events	210	194	-7.6%
Total GC pause time	3235ms	2327ms	-28%
Young gen collections	142	134	-5.6%
Peak heap before GC	~1530M	~1399M	-8.5%

Wall-clock improvement is moderate because model building is a fraction of total build time (which includes dependency resolution, reactor sorting, plugin init). The primary benefits are: (1) reduced GC pressure — 28% less time in GC pauses, fewer allocations, lower peak heap; (2) faster model building phase — ~3.6% less CPU time in buildEffectiveModels.

Test plan

• mvn verify — all modules pass (with spotless enabled)
• mvn test -pl impl/maven-impl — all tests pass
• mvn test -pl compat/maven-model-builder — all tests pass
• Full CI verification — all 22 jobs green (pending re-run after merger.vm changes)

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR reduces intermediate immutable Model object allocations during effective model construction (Maven 4), primarily by improving generated merger/model templates and by switching several pipeline SPIs/implementations to a builder-passing pattern to avoid build-then-rebuild round-trips.

Changes:

• Optimizes Modello-generated templates (model.vm, merger.vm) to avoid false-positive builder writes and reduce map/list copying during build() and location computation.
• Updates ImmutableCollections.copy(Map) to recognize JDK immutable map implementations and skip redundant wrapping.
• Refactors model-building pipeline components (injectors/normalizers/translators/assemblers/expanders) and their SPIs to write into a provided Model.Builder instead of returning rebuilt Model instances.

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/mdo/model.vm	Reduces redundant collection copying and optimizes location map merging.
src/mdo/merger.vm	Adds identity/value guards to reduce unnecessary builder field writes during merges.
src/mdo/java/ImmutableCollections.java	Skips copying for JDK immutable map implementations.
pom.xml	Updates resolverVersion from snapshot to release.
impl/maven-testing/src/main/java/org/apache/maven/testing/plugin/MojoExtension.java	Adapts path alignment to builder-passing translator API.
impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelPathTranslatorTest.java	Updates tests for builder-passing path translation API.
impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultInheritanceAssemblerTest.java	Updates tests for builder-passing inheritance assembly API.
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultProfileInjector.java	Implements builder-passing profile injection and adds no-op caching.
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultPluginManagementInjector.java	Switches plugin management injection to builder-passing.
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelPathTranslator.java	Switches path translation to builder-passing and avoids unchanged sub-object sets.
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelNormalizer.java	Switches duplicate merge/default injection to builder-passing and avoids unchanged rebuilds.
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java	Updates the effective-model pipeline to create/pass builders through each stage.
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultInheritanceAssembler.java	Switches inheritance assembly to builder-passing and merges into provided builder.
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultDependencyManagementInjector.java	Switches dependency management injection to builder-passing.
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginConfigurationExpander.java	Switches plugin configuration expansion to builder-passing and avoids unchanged sets.
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelUrlNormalizer.java	Switches URL normalization to builder-passing.
api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/ProfileInjector.java	Updates SPI to accept Model.Builder.
api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/PluginManagementInjector.java	Updates SPI to accept Model.Builder.
api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/PluginConfigurationExpander.java	Updates SPI to accept Model.Builder.
api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/ModelUrlNormalizer.java	Updates SPI to accept Model.Builder.
api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/ModelPathTranslator.java	Updates SPI to accept Model.Builder.
api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/ModelNormalizer.java	Updates SPI to accept Model.Builder.
api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/InheritanceAssembler.java	Updates SPI to accept Model.Builder.
api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/DependencyManagementInjector.java	Updates SPI to accept Model.Builder.

Comments suppressed due to low confidence (2)

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelUrlNormalizer.java:63

• The SCM URLs are normalized and written via a newly-built Scm even when normalization is a no-op. Because the model builders use identity checks for their short-circuit, writing a value-equal but different String instance can force unnecessary immutable copies. Only rebuild/set scm when at least one normalized field actually changes (including value-equality).

        Scm scm = model.getScm();
        if (scm != null) {
            builder.scm(Scm.newBuilder(scm)
                    .url(normalize(scm.getUrl()))
                    .connection(normalize(scm.getConnection()))
                    .developerConnection(normalize(scm.getDeveloperConnection()))
                    .build());
        }

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelUrlNormalizer.java:71

• Site URL normalization is applied via withSite(site.withUrl(...)) without checking whether the normalized URL actually differs. If normalization returns a value-equal String (or no-op), this can still allocate new immutable objects. Guard the update so DistributionManagement/Site are only rebuilt when the URL truly changes (including value-equality).

        DistributionManagement dist = model.getDistributionManagement();
        if (dist != null) {
            Site site = dist.getSite();
            if (site != null) {
                builder.distributionManagement(dist.withSite(site.withUrl(normalize(site.getUrl()))));
            }
        }

---

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

[gnodet]

Solid performance optimization — the benchmark results (28% less GC pause time, 7.6% fewer GC events) demonstrate meaningful improvement. The shared-builder pattern is well-designed. A few observations on behavioral changes embedded in the refactoring:

1. Report plugin expansion bug fix: In DefaultPluginConfigurationExpander, the old code called expandReport(reporting.getPlugins()) but discarded the return value — report plugin configuration-to-report-set merging was silently broken. The new code correctly captures and applies the expanded report plugins. This is a genuine bug fix worth calling out explicitly in the PR description so testers are aware of the changed behavior.

2. MojoExtension test model alignment: In MojoExtension.beforeEach, the code changed from aligning tmodel (raw parsed POM) to aligning model (merged with defaults). When tmodel == null, the old code stored null; the new code stores the aligned default model. Both changes look like intentional improvements, but the behavioral delta may affect test expectations.

3. SPI interface changes: Seven SPI interfaces (ProfileInjector, InheritanceAssembler, etc.) changed from returning Model to accepting Model.Builder and returning void. This is source/binary-incompatible for any third-party implementations, though acceptable since Maven 4 is pre-release and these are all @since 4.0.0.

Minor notes:

• The resolverVersion bump from 2.0.21-SNAPSHOT to 2.0.21 in pom.xml is unrelated to the optimization work.
• The ImmutableCollections JDK-internal class name detection is fail-safe (falls through to defensive copy), which is good.

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

Claude Code on behalf of gnodet

[elharo]

This is imnportant; ask me about handshoe sometime

[elharo]

Is this guaranteed by the JDK or could the actual class names used here change in the future?

Is there any other way we could test this? Maybe addAll(Collections.emptyList()) and see if it throws or not.

[gnodet]

Good catch — the JDK internal class names are indeed not guaranteed and could change between versions. Rather than switching to a mutation-based test (try/catch on put() has its own downsides: performance overhead on the hot path, reliance on exception semantics), I've simply removed the check entirely. In practice, JDK Map.of()/Map.copyOf() maps rarely appear in the model builder — most maps are either our own AbstractImmutableMap (already short-circuited) or HashMap from merging. The wrapping cost for the occasional JDK immutable map is a single array copy of the entries, which is negligible.

[gnodet]

Re-review after new commits (83a37f7, 7adf18c, 2f1c0a5)

Looks great — the new commits cleanly address elharo's feedback (fragile JDK class name check removed) and the merger template false-positive fix (Objects.equals instead of reference check for String fields) is an important correctness improvement.

Two minor observations:

1. Timing log level (DefaultModelBuilder.java:838): The [TIMING] log uses INFO level, which means every Maven 4 reactor build prints internal timing info. All other timing/diagnostic logging in DefaultModelBuilder uses DEBUG — consider aligning this one too, unless you want it visible in production builds.

2. Minor cache note (DefaultProfileInjector.java:155): doInjectSingleProfile always returns true for non-null profiles, so the cache never short-circuits single-profile no-op injections. Not a correctness issue — just a minor missed optimization opportunity.

Overall this is a well-executed performance optimization with solid benchmark results.

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]

Suggestion: this timing log uses INFO level, but all other timing/diagnostic logging in this class uses DEBUG. Consider changing to logger.debug(...) to keep production build output clean — unless you specifically want this visible for users.

Suggested change

	logger.info(
	logger.debug(

[gnodet]

Re-review after commit 357cf69 ("Short-circuit interpolation for strings without variable references")

Clean optimization — skipping the full substVars pipeline (HashSet allocation, delimiter scanning, unescape processing) for strings without $ is sound. The null guard is consistent with existing behavior, and the $ check correctly covers both ${...} placeholders and $__ escape markers.

Looks good. 👍

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

Claude Code on behalf of gnodet