Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(feat) Maven plugin improvements #20695

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

AB-xdev
Copy link

@AB-xdev AB-xdev commented Dec 13, 2024

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...
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
@CLAassistant
Copy link

CLAassistant commented Dec 13, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
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.

@AB-xdev AB-xdev marked this pull request as ready for review December 13, 2024 12:11
@mshabarov mshabarov added the Contribution PRs coming from the community or external to the team label Dec 13, 2024
@mstahv
Copy link
Member

mstahv commented Dec 13, 2024

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

@AB-xdev
Copy link
Author

AB-xdev commented Dec 16, 2024

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

@mshabarov mshabarov requested a review from mcollovati December 16, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution PRs coming from the community or external to the team
Projects
None yet
4 participants