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

Filtering the dependencies to be considered during the vaadinPrepareFrontend task. #120

Closed
jwn-css opened this issue Sep 10, 2021 · 16 comments · Fixed by #125
Closed

Filtering the dependencies to be considered during the vaadinPrepareFrontend task. #120

jwn-css opened this issue Sep 10, 2021 · 16 comments · Fixed by #125
Assignees
Labels
enhancement New feature or request

Comments

@jwn-css
Copy link

jwn-css commented Sep 10, 2021

Is your feature request related to a problem? Please describe.

If the number of dependencies of the project is very large and the types of dependencies are very different, the execution of the vaadinPrepareFrontend task can take a relatively long time.

Describe the solution you'd like

Filtering on specific file names or extensions would be helpful, if only to reduce the number of dependencies to consider.

Describe alternatives you've considered

Alternatively, it is possible to partially exclude the task and execute it only when needed. However, it is then inevitably necessary that all participants in the project know exactly what the background of the task is and when one must execute the task and when not.

Additional context

It would be very helpful if this feature, if considered, would also be included in the version for the current LTS i.e. Vaadin 14.

@mstahv
Copy link
Member

mstahv commented Sep 23, 2021

Hi,

Would you add some log output regarding the scanning that Vaadin does. Something like "Visited {} classes. Took {} ms."

You can also maybe use blacklist/whitelist to define what gets scanned:
https://vaadin.com/docs/v14/flow/integrations/spring/tutorial-spring-configuration#special-configuration-parameters

cheers,
matti

@mvysny
Copy link
Member

mvysny commented Nov 16, 2021

Hi all, couple of thoughts:

  • If I remember correctly, only zip-type files are considered (so no .dlls for example); on top of that only the .jar files are considered (files with the jar extension). Please see could not create Vfs.Dir from url, no matching UrlType #115 for more details.
  • @mstahv I think that groupId+artifactId-based filtering would be more beneficial in this particular case. The vaadin.blacklisted-packages setting can not modify the classpath and thus the only option is to ignore certain Java packages; however vaadinPrepareFrontend task works in much earlier stages and we could squeeze even more performance by ignoring entire jars; also thinking in terms of dependency jars instead of packages makes more sense to me in the context of a Gradle plugin. Also I believe that's what the original reporter also wanted. @jwn-css what are your thoughts on this please?
  • A combined include/exclude mechanism similar to Gradle Copy task could be used, to provide multitude of ways of configuration: https://docs.gradle.org/current/dsl/org.gradle.api.tasks.Copy.html#org.gradle.api.tasks.Copy:includes
  • Would you add some log output regarding the scanning that Vaadin does. Something like "Visited {} classes. Took {} ms." - let me investigate on this.

@mvysny mvysny self-assigned this Nov 22, 2021
@mvysny mvysny added the enhancement New feature or request label Nov 22, 2021
@mvysny
Copy link
Member

mvysny commented Nov 22, 2021

Would you add some log output regarding the scanning that Vaadin does. Something like "Visited {} classes. Took {} ms."

Unfortunately the class processing happens internally in the NodeTasks class (provided by the flow-server.jar) which then uses it internally to discover classes (for example see the FrontendWebComponentGenerator class for more details). Looks like it's impossible for the Gradle plugin itself to dump any performance stats; but we can ask the Flow team to add some kind of debugging support to the NodeTasks class @mstahv

That being said, the Gradle plugin dumps the set of jar files to be sent to NodeTasks, using the logger INFO level: Passing this classpath to NodeTasks.Builder: [flow-server.jar, ...]

@mvysny
Copy link
Member

mvysny commented Nov 23, 2021

The API proposal: the build.gradle.kts snippets could look like the following:

vaadin {
  filterClasspath {
    include("com.vaadin:*")
    exclude("org.slf4j:*")
    exclude("commons-codec:commons-codec")
  }
}

The com.vaadin:flow-server will always be present and applied, regardless of the combination of the above. The API must work both with build.gradle and build.gradle.kts buildscripts (both Groovy and Kotlin).

@jwn-css what do you think?

@mstahv
Copy link
Member

mstahv commented Nov 23, 2021

@caalador What do you think, would it be feasible (what Martin suggested)? I don't know if we know anything about artifactId or groupId in the right place 🤔

@mvysny
Copy link
Member

mvysny commented Nov 23, 2021

@mstahv not sure what you mean 🤔 I believe the only place to implement a groupId/artifactId-based filtering is the Plugin code itself; at runtime the groupId/artifactId information is lost and not available. I am prototyping the solution code in the Plugin code itself, it should be ready by tomorrow 👍

mvysny added a commit that referenced this issue Nov 24, 2021
mvysny added a commit that referenced this issue Nov 24, 2021
@pleku
Copy link

pleku commented Nov 24, 2021

@mvysny I thought the discussion was on how to make it happen for Spring Boot where it is very slow to scan things 😄

I'm afraid the plugin code itself is not a full solution for all of the scanning, but it is a start. It would make sense to make it work for all occasions and not have different API/implementation for different technologies. But maybe testing it first with the gradle plugin is the best way to get forward and then expand based on the experience there 👍🏻

mvysny added a commit that referenced this issue Nov 24, 2021
mvysny added a commit that referenced this issue Nov 24, 2021
@mvysny
Copy link
Member

mvysny commented Nov 24, 2021

@pleku i see, thank you for the explanation. The customer explicitly mentioned the vaadinPrepareFrontend task, therefore I judged the performance problem should be solved within the Plugin code. However, if we should have similar performance boosts for Spring Boot, then you're right that the plugin code change will not be enough...

@jwn-css the support for artifact filtering is implemented in the PR #125 . Could you please try out the new plugin whether it works for you? You can find the new documentation at https://github.com/vaadin/vaadin-gradle-plugin/blob/feature/120-filter-dependencies/README.md ; then check out the branch to your machine and use the plugin with your project by taking advantage of Gradle composite builds as documented at https://github.com/vaadin/vaadin-gradle-plugin/blob/14.x/CONTRIBUTING.md

@jwn-css
Copy link
Author

jwn-css commented Nov 24, 2021

I'll take a look and get back to you if it works for us.

@mvysny mvysny added the question Further information is requested label Nov 25, 2021
@jwn-css
Copy link
Author

jwn-css commented Nov 26, 2021

I was able to test it yesterday. It seems to fit our requirements exactly. We were able to drastically reduce the execution time and packages that are looked at:

Previously:
Reflections took 10284 ms to scan 316 urls, producing 21378 keys and 132278 values.

Now:
Reflections took 550 ms to scan 69 urls, producing 574 keys and 3656 values

@mvysny
Copy link
Member

mvysny commented Nov 26, 2021

Wow, thank you for letting me know. That's a speed improvement of over 20x; the speedup comes directly from constructing ReflectionsClassFinder in VaadinUtils.kt with a smaller set of jar files; that causes Reflections.scan() to go through a smaller set of jar files much more quickly.

@mvysny mvysny removed the question Further information is requested label Nov 26, 2021
@mvysny
Copy link
Member

mvysny commented Nov 26, 2021

Since the PR has been verified, I'll merge the PR and release a new plugin version.

@mvysny
Copy link
Member

mvysny commented Nov 29, 2021

The new plugin has been released, please try it out: the 0.14.7.5 version 👍

@jwn-css
Copy link
Author

jwn-css commented Oct 21, 2022

Will the feature be transferred to the newer versions (23.x) as well?

@mvysny
Copy link
Member

mvysny commented Oct 21, 2022

The feature should be there; if not, please open a feature request at https://github.com/vaadin/flow/issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants