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

Convert most classes to use @CompileStatic #46

Open
wolfs opened this issue May 18, 2018 · 5 comments
Open

Convert most classes to use @CompileStatic #46

wolfs opened this issue May 18, 2018 · 5 comments

Comments

@wolfs
Copy link
Collaborator

wolfs commented May 18, 2018

The Gretty plugin can have a negative impact on configuration time since all of it currently uses dynamic Groovy. Using CompileStatic on a significant number if not on all classes would improve configuration time already a lot.

@javabrett
Copy link
Member

Noting that Gretty 2.x still claims in docs to support Gradle 1.10 !!, although surely hardly anyone relies on that (nor should they) and we certainly don't test it. I understand @CompileStatic would require Groovy 2.x making Gradle 2.x minimum. So it's another one to add to the support-matrix deprecation list for the next-release, and depending on the branch/version we might deprecate Gradle 2.x support too.

@javabrett
Copy link
Member

javabrett commented May 21, 2018

Hoping that @wolfs or @rwinch can validate an initial/basic approach for this, and the results, or anyone else with @CompileStatic or Gradle/Groovy performance-tuning experience.

Here's what I tried:

Baseline data:

  • There's 324 *.groovy files in the project, including a handful of atypical source files for build etc.
  • Full integration-test for a specific test-config ranges from 10m42s to 17m15s for a range of containers.

Approach

  • Add a config.groovy containing ast(groovy.transform.CompileStatic) and reference it.
  • For each location using dynamic Groovy, or other code which causes a static-compilation Java check to fail, just annotate the entire class with @CompileStatic(TypeCheckingMode.SKIP). No attempt to limit this to specific methods at this point.
  • The above results in 60 classes that are skipped and not statically-compiled, retaining dynamic Groovy.
  • Make other minor fixes e.g. adding required casts to fix remaining static-compilation errors.

Results

Completely unscientific, anecdotal from a single run.

  • Jetty 7 test failed with a java.lang.IllegalAccessError. Not a huge concern as Jetty 7 support is scheduled for deprecation. Another container failed but with a known fragile-test issue and passed on re-run.
  • Test execution times (single run, these times are also sometimes a blend of containers and not all containers run all tests):
Container Dynamic Compilation Selective Static Compilation Change
jetty7 10m42s failed X
jetty8 15m40s 15m12s -22s
jetty9.2 14m51s 13m15s -1m36s
tomcat7 13m17 13m30s +13s
tomcat8 17m15s 12m16s -4m59s
tomcat8.5 12m30s 11m56s -34s
tomcat9 13m21s 12m49s -32s

Comment

Seems like an overall improvement, but apart from one dynamic change, nothing spectacular.

Questions

  • Is the above approach reasonable, or how could it be improved?
  • Any way of further validating what has been done?
  • Any point in doing hotspot analysis to try and apply more static-compilation by being more selective with exclusion using method-annotations, or "fixing" bits of code so they work statically?

@wolfs
Copy link
Collaborator Author

wolfs commented May 21, 2018

@javabrett Thank you for trying it out!

Regarding your numbers above: I don't understand why these improvements would be caused by adding CompileStatic: CompileStatic will only improve the execution time of scripts, since less dynamic invocations need to be done, which may be expensive.

So the improvement would be in configuration time of the build. I guess all the above projects are small, right? So you wouldn't even notice configuration time improvements. Did you restart the daemon between runs? If not, then the time improvement is due to having a warm daemon.

If you want to do some performance measurements, please use gradle-profiler. That would allow you to get more accurate measurements.

Moreover, for testing the configuration time improvements, you would need a sufficiently big project (maybe 100 sub-projects), each applying the Jetty plugin.

Note that it is not necessary to convert all classes to CompileStatic, only the ones which are executed when the plugin is applied (as opposed to when the plugin's tasks are executed) would be enough.

@javabrett
Copy link
Member

Thanks @wolfs for the comments and help.

Note that it is not necessary to convert all classes to CompileStatic

I think that was my reaction to the issue summary "Convert most classes to use @CompileStatic" and "Using CompileStatic on a significant number if not on all classes" :).

... only the ones which are executed when the plugin is applied (as opposed to when the plugin's tasks are executed) would be enough.

So that might be in GrettyPlugin#apply and everything called from there? Note that this had compile-errors when compiled Java static, so it was excluded for the first-pass approach.

There's also https://github.com/gretty-gradle-plugin/gretty/blob/master/integrationTests/buildSrc/gretty-integrationTest/src/main/groovy/org/akhikhl/gretty/internal/integrationTests/BasePlugin.groovy#L109 for dev.

So the improvement would be in configuration time of the build.

I think this refers to a Gradle build of a project which applies the Gretty project once or more (rather than the build of the Gretty project itself).

Regarding your numbers above: I don't understand why these improvements would be caused by adding CompileStatic: CompileStatic will only improve the execution time of scripts, since less dynamic invocations need to be done, which may be expensive. So the improvement would be in configuration time of the build. I guess all the above projects are small, right? So you wouldn't even notice configuration time improvements. Did you restart the daemon between runs? If not, then the time improvement is due to having a warm daemon.

The timing numbers refer to builds which run integrationTests, which run on Travis, where I believe the Gradle Daemon is disabled by-default, or at least each build runs in a clean Docker. Per above at least one important class has been disabled. Probably any reduction is coindidental.

Moreover, for testing the configuration time improvements, you would need a sufficiently big project (maybe 100 sub-projects), each applying the Jetty plugin.

I was tempted to ask what a canonical test for performance-improvements might look like before I started - sounds like this is it.

@javabrett
Copy link
Member

The configuration-time performance improvement from adding @CompileStatic should now be available in Gretty 2.3.0-SNAPSHOT and 3.0.0-SNAPSHOT. A 2.3.0 release shouldn't be too far away.

A subsequent buildscan for spring-security is available at https://scans.gradle.com/s/o6jlskwqa7k3y .

Thanks @rwinch for the report and @wolfs and @oehme for the profiler support.

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

No branches or pull requests

2 participants