[MNG-8507] Reduce allocation pressure in model building pipeline - #12540

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.

        Scm scm = model.getScm();
        if (scm != null) {
            builder.scm(Scm.newBuilder(scm)
                    .url(/sitelet?url=https%3A%2F%2Fgithub.com%2Fapache%2Fmaven%2Fpull%2Fnormalize%28scm.getUrl%28)))
                    .connection(normalize(scm.getConnection()))
                    .developerConnection(normalize(scm.getDeveloperConnection()))
                    .build());
        }

        DistributionManagement dist = model.getDistributionManagement();
        if (dist != null) {
            Site site = dist.getSite();
            if (site != null) {
                builder.distributionManagement(dist.withSite(site.withurl(/sitelet?url=https%3A%2F%2Fgithub.com%2Fapache%2Fmaven%2Fpull%2Fnormalize%28site.getUrl%28)))));
            }
        }

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

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

This is imnportant; ask me about handshoe sometime

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

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

test failing:

[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.952 s -- in org.apache.maven.it.MavenIT0008SimplePluginTest
[INFO]
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] MavenITmng7038RootdirTest.testRootdir:69 project.rootDirectory ==> expected: </Users/runner/work/maven/maven/its/core-it-suite/target/test-classes/mng-7038-rootdir/module-a> but was: </Users/runner/work/maven/maven/its/core-it-suite/target/test-classes/mng-7038-rootdir>

Re-review after 4 new commits + elharo's CHANGES_REQUESTED

🔴 Confirmed regression: rootDirectory hoisting

The rootLocator.findRoot() call was hoisted out of the per-module loop in DefaultProjectBuilder.java (~line 560), but the assumption that all reactor modules share the same root directory is incorrect. Maven 4 allows sub-modules to declare root="true" in their POM (e.g., mng-7038-rootdir/module-a/pom.xml), making findRoot() return different results per module.

Computing root once from the top-level POM overrides per-module roots, causing MavenITmng7038RootdirTest.testRootdir to fail on all 9 CI matrix combinations:

expected: <.../module-a> but was: <.../mng-7038-rootdir>

Fix: Revert this single hoisting — keep rootLocator.findRoot(pom.getParentFile().toPath()) inside the loop.

✅ Other optimizations look good

• Settings decryption fix (094f0b5): Correct — moves the indexOf('$') < 0 fast-path to the model interpolator, preserving {...} decryption in settings.
• hashCode varargs avoidance (26b636f): Textbook-correct manual hash computation matching Objects.hash() semantics.
• Scope validation pre-computation, HashSet reuse, ConcurrentHashMap cache, and other micro-optimizations are all safe and well-crafted.

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

Claude Code on behalf of gnodet

Re-review: rootLocator regression fixed ✅

Commit 4a51d64d correctly reverts the rootLocator.findRoot() hoisting — it's back inside the per-module loop, computed from each module's pom.getParentFile().toPath(). This resolves the MavenITmng7038RootdirTest failure.

All other optimizations in this PR remain sound. Looks good to merge once CI confirms.

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

Claude Code on behalf of gnodet