feat: Maven plugin improvements #20695

AB-xdev · 2024-12-13T12:07:52Z

aka a (bit early) Christmas present from XDEV to Vaadin 🎄

Description

This PR reworks the Vaadin Maven Plugin to improve performance.
The changes primarily target the prepare- and build-frontend goals (all other goals are irrelevant for us) and are based on some experimental changes we have done in internal projects.

Fixes #19596
Fixes #20359

Overview of the changes:

A core problem of the plugin has always been that it scans/loads all classes of the entire project. This might be ok for a small Vaadin starter but if you have a big project with a few hundred dependencies this takes forever.
If you combine this situation with frequent (re)builds due to e.g. not a good working IDEs, this results in further problems like increased coffee consumption in between builds.
The improvement here is to only scan Vaadin dependencies by default.
The user can further customize and include/exclude certain dependencies if they want.
A lot of additional optimizations have been introduced, that enable the user to disable additional checks which might not be required.
Made dependency onto org.codehaus.plexus.build.BuildContext optional (not everyone uses Eclipse ;) )
Most things in the plugin can now be replaced/overridden if required

Example configuration

<configuration>
	<!-- We don't require React -->
	<reactEnable>false</reactEnable>
	<!-- Only needed when updating -->
	<frontendIgnoreVersionChecks>true</frontendIgnoreVersionChecks>
	<!-- We don't use Hilla -->
	<hillaAvailable>false</hillaAvailable>
	<!-- We know how to use a BOM -->
	<checkPluginFlowCompatibility>false</checkPluginFlowCompatibility>
	<!-- We also don't use that -->
	<supportDAU>false</supportDAU>
	<fastReflectorIsolation>
		<includeFromTargetDirectory>false</includeFromTargetDirectory>
		<includes>
			<additional>
				<selector>
					<groupId>...</groupId>
					<artifactId>web...*</artifactId>
				</selector>
				<!-- Required because of interface usage -->
				<selector>
					<groupId>...</groupId>
					<artifactId>logger-...</artifactId>
					<scan>false</scan>
				</selector>
			</additional>
		</includes>
	</fastReflectorIsolation>
	<!-- Build-Frontend specific -->
	<!-- This project contains no things that require a license during build -->
	<performLicenseCheck>false</performLicenseCheck>
	<!-- We know how to use a BOM -->
	<checkRuntimeDependency>false</checkRuntimeDependency>
</configuration>

In a example project (with ~40k LoC and ~150 dependencies) with above configuration we now have the following times:

	`prepare-frontend`	`build-frontend`
Vaadin Default	~1600ms	~5100ms
with improvements (this PR)	~900ms (~75% faster)	~2000ms (~250% faster)

Type of change

Bugfix
Feature

Checklist

I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
I have added a description following the guideline.
The issue is created in the corresponding repository and I have referenced it.
I have added tests to ensure my change is effective and works as intended.

Fixed existing tests so that they work. Feel free to add more tests if you like.
New and existing tests are passing locally with my change.

There seem to be some tests that don't work locally, e.g.:
[main] ERROR com.vaadin.flow.server.frontend.FrontendToolsLocator - Failed to execute the command '[C:\Program Files\maven\apache-maven-3.9.8\bin\mvn, -v]' java.io.IOException: Cannot run program "C:\Program Files\maven\apache-maven-3.9.8\bin\mvn": CreateProcess error=193, %1 ist keine zulässige Win32-Anwendung or DefaultInstantiatorI18NTest.translationFileOnClasspath_instantiateDefaultI18N:122 expected:<[Default lang]> but was:<[deutsch]>
I have performed self-review and corrected misspellings.

Additional for `Feature` type of change

Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

PS:

If you see Matti, please give him our best regards
Would be great if the Formatter & Co is checked into the IntelliJ IDE settings (yes that's possible - have a look at our repos) so you/I don't have to set it up manually or correct it constantly ;)

Make everything accessible/overrideable downstram

Yes not everyone is using Eclipse or want's to have Eclipse specific dependencies...

CLAassistant · 2024-12-13T12:07:58Z

All committers have signed the CLA.

CLAassistant · 2024-12-13T12:07:59Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

mstahv · 2024-12-13T15:18:37Z

Awesome @AB-xdev 👏 Does this also fix performance regressions in the development mode 🤔

AB-xdev · 2024-12-16T07:33:26Z

Awesome @AB-xdev 👏 Does this also fix performance regressions in the development mode 🤔

@mstahv We only changed the Maven Plugin and nothing noteworthy of the flow-server or similar.
If you use the plugin during "development mode" to e.g. re-generate the bundle then maybe yes (I'm not quite sure what you guys count to "development mode" and what not).

mcollovati · 2024-12-20T09:02:22Z

@AB-xdev thank you so much for the contribution. The PR contains many changes, so it will take some time to review it.

A couple of questions and notes:

what's the reason to make all fields protected?
why remove the logTroubleshootingHints method?
what's the benefit of the supportDAU flag? Is it only to skip the application identifier hashing?
the original solution included only a subset of provided dependencies; however, with this change, the filter is gone. I need to check the reasons behind the filter and see if they are still valid.
all public classes and methods required Javadoc

mcollovati

I did a first review round and added some comments.
I still need to test the changes locally because I currently see several test failures.
I'll continue the review as soon as I get the validation pass locally.

mcollovati · 2024-12-20T12:37:43Z

flow-server/src/main/java/com/vaadin/flow/internal/ReflectTools.java

-    public static Method findMethod(Class<?> cls, String methodName,
-            Class<?>... parameterTypes) throws ExceptionInInitializerError {


Why is this public method removed? If it is not used anywhere, we can consider deprecating it, but it cannot be removed.

Why is this public method removed?

It was only used in the plugin and is also marked as internal (in package)

If it is not used anywhere, we can consider deprecating it, but it cannot be removed.

We can change it back if you like

Even if it is internal, usually we try not to remove public API in minor or releases.
So, yes, please bring the method back and mark it as @Deprecated(forRemoval). Also, the Javadoc @deprecation should state that there will be no replacement.

Restored and marked as deprecated

mcollovati · 2024-12-20T12:42:31Z

...-plugins/flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/BuildFrontendMojo.java

-        if (missingDependencyMessage == null) {
-            missingDependencyMessage = text -> {
-            };
-        }


Would instantiating an Optional be more performant than having a no-op consumer?
Otherwise, I would leave the code as is.

I used an Optional further down in the ifs as I found it more readable and it makes more sense there (the previously created consumer may have never been used in the ifs below).

As an alternative one can also do a null check, like:

if(...) { if (missingDependencyMessage != null) { // Potential replacement for Optional missingDependencyMessage.accept(...); } }

To be honest, I would prefer the null check in this case, since there is no pipeline built on top of the Optional.

mcollovati · 2024-12-20T12:44:53Z

flow-plugins/flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/DefaultReflector.java

+import java.util.Set;
+
+
+public class DefaultReflector implements Reflector {


Need Javadoc, both on type definition and on public and protected methods.

Can we make the class final?

Can we make the class final?

What's the advantage of this?
Currently one can override the class if required. If it's final that's no longer possible.

mcollovati · 2024-12-20T12:46:11Z

flow-plugins/flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/DefaultReflector.java

+        this.isolatedClassLoader = Objects.requireNonNull(isolatedClassLoader);
+    }
+
+    public DefaultReflector(final Object copyFromOtherClassLoader) {


I would make this constructor package-protected, as it looks like to me, it should be used only internally.

I would make this constructor package-protected, as it looks like to me, it should be used only internally.

Would also keep it public so one can override/instantiate it if required.

mcollovati · 2024-12-20T12:49:46Z

flow-plugins/flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/DefaultReflector.java

+            final Class<?> classFinderImplClass = getIsolatedClassLoader().loadClass(
+                    ReflectionsClassFinder.class.getName());
+            classFinder = classFinderImplClass
+                    .getConstructor(ClassLoader.class, URL[].class)
+                    .newInstance(
+                            isolatedClassLoader,
+                            isolatedClassLoader.urlsToScan());
+        }


Any reasons to get the class loader instance both by field access and getIsolatedClassLoader() method?
If not, I would use the field, so we are sure to use the exactly same instance.

+1

Overlooked that, it should probably use the method too like on all other occurrence.
I would prefer the method since it can be easier overridden and is part of the interface.

mcollovati · 2024-12-20T14:26:39Z

...ugins/flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/FlowModeAbstractMojo.java


-    private Consumer<File> buildContextRefresher;


Honestly, I would prefer to preserve this consumer here at the moment. I don't see any performance impact on having it. Or do you have any evidence that keeping this field negatively impacts on performance?

mcollovati · 2024-12-20T14:27:55Z

...ow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/FastReflectorIsolationConfig.java

+import java.util.List;
+
+
+public class FastReflectorIsolationConfig {


nit: I wonder if we can find a better name. However, I don't have a suggestion right now: I'll think about ir.

Javadocs are missing

Renamed it to FastReflectorConfig and created Isolation subclass

mcollovati · 2024-12-20T14:29:54Z

...ow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/FastReflectorIsolationConfig.java

+    }
+
+    public static class ArtifactSelectors {
+        private boolean defaults = true;


What is the purpose of this field?

Implemented the field correctly.

This defines if the defaults should be used, like
https://github.com/AB-xdev/flow/blob/d316e63d594e8ab979b9011019be0075ed434741/flow-plugins/flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/DefaultReflectorProvider.java#L41-L48

mcollovati · 2024-12-20T14:36:28Z

...ow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/FastReflectorIsolationConfig.java

+         * Please note that this only works for inclusions, not exclusions.
+         * </p>
+         */
+        private boolean scan = true;


I can't find where this flag is used. Can you please explain it to me?

Improved the JavaDoc.

Generally speaking if this is false than it will only be used for class loading, otherwise if it's true it will be used for scanning + class loading

mcollovati · 2024-12-20T14:40:02Z

...flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/DefaultReflectorController.java

+    protected boolean checkIfArtifactSelectorMatches(
+            final FastReflectorIsolationConfig.ArtifactSelector selector,
+            final Artifact artifact) {
+        if (selector.getGroupId() != null && !compareSelector(selector.getGroupId(), artifact.getGroupId())) {
+            return false;
+        }
+        return selector.getArtifactId() == null
+                || compareSelector(selector.getArtifactId(), artifact.getArtifactId());
+    }
+
+    protected boolean compareSelector(final String selector, final String target) {
+        if (selector == null || target == null) {
+            return false;
+        }
+
+        if (selector.endsWith("*") && selector.startsWith("*")) {
+            return target.contains(selector.substring(1, selector.length() - 1));
+        } else if (selector.endsWith("*")) {
+            return target.startsWith(selector.substring(0, selector.length() - 1));
+        } else if (selector.startsWith("*")) {
+            return target.endsWith(selector.substring(1));
+        } else {
+            return selector.equals(target);
+        }
+    }


I wonder if these methods can be moved to ArtifactSelector. Something like ArtifactSelector.matches(Artifact).
It would also simplify adding tests for the logic.

I would keep this here since the Config class is just a data holder which contains no logic.

mcollovati · 2024-12-23T09:46:21Z

I created #20776, rebased on main, to test the changes.
After a first run, it looks like that the change is potentially breaking existing projects, most likely because it filters out too many dependencies by default.

Error: Failed to execute goal com.vaadin:flow-maven-plugin:999.99-SNAPSHOT:prepare-frontend (default) on project test-prod-bundle: Execution default of goal com.vaadin:flow-maven-plugin:999.99-SNAPSHOT:prepare-frontend failed: A required class was missing while executing com.vaadin:flow-maven-plugin:999.99-SNAPSHOT:prepare-frontend: org/codehaus/plexus/build/BuildContext

I'll continue the investigation, pushing changes on the linked PR.

mcollovati · 2024-12-23T10:19:04Z

...flow-maven-plugin/src/main/java/com/vaadin/flow/plugin/maven/DefaultReflectorController.java

+
+        // If a jar is inside a target folder, it's likely a sibling project
+        if (fastReflectorIsolationConfig.isIncludeFromTargetDirectory()
+                && "target".equals(artifact.getFile().getParentFile().getName())) {


Output directory can be configured in maven build, so it would be better to avoid hard-coding "target" and get the value from the Maven project.

mcollovati · 2024-12-23T15:03:15Z

Currently, the changes in the PR will break projects using vaadin-maven-plugin because of the removal of static methods used by hilla-maven-plugin. Even re-adding the missing methods, Hilla projects seems not to work out of the box: they require manual configuration of the plugin to include dependencies that are excluded by this change (e.g. swagger, jackson, classgraph). Given that existing projects should not require any configuration update if this one gets merged, I would make the dependency filtering an opt-in and not active by default.

The PR is quite complex and introduces multiple performance improvements, so I would suggest splitting it into several smaller PR (the best option would be one per each added configuration) so that we can measure the effective impact of every single change.
This way, it would also be simpler to find alternative solutions for the changes that skip important build steps, like license checking.

mcollovati · 2024-12-23T15:34:32Z

Also note that Hilla plugin recently removed the need for executing configure goal (vaadin/hilla#2616). That change should also improve the overall performance of vaadin-maven-plugin.

AB-xdev · 2025-01-13T12:27:33Z

Wow, that's a lot of comments. I will try to answer them all.

what's the reason to make all fields protected?

So it's possible that one can override them without getting having to use reflection or copying code ^^
This also includes methods, classes, etc.

why remove the logTroubleshootingHints method?

The problems are reported directly by DefaultReflectorController this way there is no need to store them as a variable
The other messages ("The build process encountered an error: .." and "To diagnose the issue, please re-run Maven with the -X ...") are nearly the same as Maven itself reports when the build fails. There is no need to duplicate them.

what's the benefit of the supportDAU flag? Is it only to skip the application identifier hashing?

Correct

the original solution included only a subset of provided dependencies; however, with this change, the filter is gone. I need to check the reasons behind the filter and see if they are still valid.

Yes, the original only contained portlet-api and javax.servlet-api.
I removed it because

the code looked very ancient
- There seems to have been also a VaadinPortlet in the past.
there was no documentation why it's still there
Vaadin nowadays requires jakarta (which was not present in the regex)

all public classes and methods required Javadoc

I didn't know that.

Currently, the changes in the PR will break projects using vaadin-maven-plugin because of the removal of static methods used by hilla-maven-plugin. Even re-adding the missing methods, Hilla projects seems not to work out of the box: they require manual configuration of the plugin to include dependencies that are excluded by this change (e.g. swagger, jackson, classgraph). Given that existing projects should not require any configuration update if this one gets merged, I would make the dependency filtering an opt-in and not active by default.

The PR is quite complex and introduces multiple performance improvements, so I would suggest splitting it into several smaller PR (the best option would be one per each added configuration) so that we can measure the effective impact of every single change.
This way, it would also be simpler to find alternative solutions for the changes that skip important build steps, like license checking.

I already considered breaking it down into smaller bits, however since the reflector is like 90% of the change and the other changes sometimes depend on one another I discarded this option.
But you can try to do so yourself if you find a good way to do it :)

Also note that Hilla plugin recently removed the need for executing configure goal (vaadin/hilla#2616). That change should also improve the overall performance of vaadin-maven-plugin.

That's nice.

WIP

mcollovati · 2025-01-14T08:38:19Z

An additional note about the changes in visibility: these changes are significantly increasing the API surface that we would be promising to keep stable between versions. Before committing to something like that, we would want to better understand what potential benefits you expect to get from that.

About the provided scoped dependencies:

Yes, the original only contained portlet-api and javax.servlet-api.
I removed it because

the code looked very ancient
There seems to have been also #6769.
there was no documentation why it's still there
Vaadin nowadays requires jakarta (which was not present in the regex)

My concern is that this modification might add more dependencies to the class loader, potentially leading to behavior changes compared to the original code and introducing a breaking change. However, it could make sense to consider also provided dependency. Needs to be investigated.

This check is only needed once during migration and never again.

* Reflector now uses a Builder-like pattern and can be replaced with a custom implementation if required * Default implementation only includes/scans only actually used dependencies and not everything else. Special customization may still be required for certain projects * Replaced``getOrCreateReflector`` with ``getClassFinder`` * Removed some unused methods * Simplified and extended logging

See vaadin#20359

…s work

AB-xdev · 2025-01-14T14:55:02Z

Thank you for the additional feedback.
I think I responded now to all/most topics.

An additional note about the changes in visibility: these changes are significantly increasing the API surface that we would be promising to keep stable between versions. Before committing to something like that, we would want to better understand what potential benefits you expect to get from that.

Generally speaking I want to be able to overwrite/customize your code without constantly having to copy it when an update happens because some method is private or something like that.

Since I had this problem now for the like 50th time, I decided to write a short blog post about it.

these changes are significantly increasing the API surface that we would be promising to keep stable between versions

I was not aware that you have to keep the plugin "stable" (you know since the average Vaadin user will likely never extend it).

My concern is that this modification might add more dependencies to the class loader, potentially leading to behavior changes compared to the original code and introducing a breaking change. However, it could make sense to consider also provided dependency. Needs to be investigated.

It should make sense to include provided dependencies as the class-loading will otherwise very likely fail (e.g. if a class extends from another one that is inside a provided dependency).
In case of emergency we can also provide an option to exclude / include provided dependencies :)

Currently, the changes in the PR will break projects using vaadin-maven-plugin because of the removal of static methods used by hilla-maven-plugin.

My opinion here is that the hilla Plugin can implement these itself since only hilla needs them.

Even re-adding the missing methods, Hilla projects seems not to work out of the box: they require manual configuration of the plugin to include dependencies that are excluded by this change (e.g. swagger, jackson, classgraph).

And now my extensibility effort from above pays off: Hilla can simply use a derivative of DefaultReflectorProvider where the default excludes/includes are slightly different and work for them.

Given that existing projects should not require any configuration update if this one gets merged, I would make the dependency filtering an opt-in and not active by default.

The new system is currently designed in a way that it's compatible to the old system and only some - very special - projects might break.
I think if the changes are also ported over into Hilla there should be nothing speaking against this being enabled out of the box.
Additionally a troubleshooting guide can also be provided, which instructs users to disable the fastReflector on errors.

Anyway I would recommended to bundle these changes only with the next minor/major release and not inside a patch release.

mshabarov · 2025-01-16T11:49:23Z

Would be great if the Formatter & Co is checked into the IntelliJ IDE settings

Yes, I agree with this concern. We have this rather old ticket, but still relevant nowadays #8163. In short, would be nice to include an automatic formatting step into all pull requests automation.

AB-xdev added 3 commits December 13, 2024 10:39

Change all occurences of private to protected

57a0ead

Make everything accessible/overrideable downstram

Only use BuildContext where it's needed

bac89bf

Assume that BuildContext can be null

dce600d

Yes not everyone is using Eclipse or want's to have Eclipse specific dependencies...

AB-xdev marked this pull request as ready for review December 13, 2024 12:11

mshabarov added the Contribution PRs coming from the community or external to the team label Dec 13, 2024

mshabarov requested a review from mcollovati December 16, 2024 12:35

mcollovati requested changes Dec 20, 2024

View reviewed changes

mcollovati changed the title ~~(feat) Maven plugin improvements~~ feat: Maven plugin improvements Dec 22, 2024

mcollovati mentioned this pull request Dec 23, 2024

WIP: testing "Maven plugin improvements" PR #20776

Draft

mcollovati reviewed Dec 23, 2024

View reviewed changes

AB-xdev added 10 commits January 14, 2025 15:03

Remove productionMode check

4d5c23c

This check is only needed once during migration and never again.

Log accurately how long a goal needs for execution

a127fa1

Remove unused code

dcfc957

Introduce optimization for FlowModeAbstractMojo

6cf2c8a

Cleanup

39ff81f

Introduce optimization for BuildFrontendMojo

f8cc082

Cleanup

6d01e74

Remove not needed dependency

f30e3cd

See vaadin#20359

Fix tests

61a6e14

AB-xdev added 12 commits January 14, 2025 15:03

Fix test - Use same logik as in plugin

98c7342

Also set default values to fields (not just annotations) so that test…

7d9808f

…s work

Fix format

01a7983

Fix format

9193f12

Rename ReflectorController -> ReflectorProvider

782b288

Cleanup

dc28229

Rework config

9256be9

Use method instead of field access

288be36

Rename of -> createNew

37e7afd

Use simpler if instead of Optional

137d820

Update docs

3f5ed55

Restore unused code and deprecate

1fc551a

AB-xdev force-pushed the maven-plugin-improvements branch from d316e63 to 1fc551a Compare January 14, 2025 14:07

Make constants configurable

390e08f

		public static Method findMethod(Class<?> cls, String methodName,
		Class<?>... parameterTypes) throws ExceptionInInitializerError {

		import java.util.Set;


		public class DefaultReflector implements Reflector {

		import java.util.List;


		public class FastReflectorIsolationConfig {

feat: Maven plugin improvements #20695

Are you sure you want to change the base?

feat: Maven plugin improvements #20695

Conversation

AB-xdev commented Dec 13, 2024 • edited Loading

Description

Type of change

Checklist

Additional for Feature type of change

CLAassistant commented Dec 13, 2024 • edited Loading

CLAassistant commented Dec 13, 2024

mstahv commented Dec 13, 2024

AB-xdev commented Dec 16, 2024

mcollovati commented Dec 20, 2024 • edited Loading

mcollovati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AB-xdev Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollovati commented Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollovati commented Dec 23, 2024

mcollovati commented Dec 23, 2024

AB-xdev commented Jan 13, 2025 • edited Loading

mcollovati commented Jan 14, 2025

AB-xdev commented Jan 14, 2025

mshabarov commented Jan 16, 2025

AB-xdev commented Dec 13, 2024 •

edited

Loading

Additional for `Feature` type of change

CLAassistant commented Dec 13, 2024 •

edited

Loading

mcollovati commented Dec 20, 2024 •

edited

Loading

AB-xdev Jan 13, 2025 •

edited

Loading

mcollovati commented Dec 23, 2024 •

edited

Loading

AB-xdev commented Jan 13, 2025 •

edited

Loading