Consider migrating Spotless Gradle plugin to use Task Configuration Avoidance

[jbduncan]

I'm in the middle of migrating my personal project jupiter-collection-testers to use a new feature introduced in Gradle 4.9 called Task Configuration Avoidance, over at this feature branch. However, I am currently stuck on the Spotless Gradle plugin, because even if I don't run gradlew spotless[Check|Apply], the plugin seems to directly depend on and thus eagerly configure (create) a number of tasks which spotless[Check|Apply] needs but other tasks like help don't need.

The reason that I believe Spotless is eagerly configuring these tasks, whichever ones they are, is because when I comment out these lines, the command gradlew help -Dorg.gradle.internal.tasks.stats produces the following relatively small output,

1 actionable task: 1 executed
Task counts: Old API 1, New API 58, total 59
Task counts: created 3, avoided 56, %-lazy 95

Task types that were created with the old API
class com.github.benmanes.gradle.versions.updates.DependencyUpdatesTask 1

Task types that were registered with the new API but were created anyways
class org.gradle.configuration.Help 1
class org.gradle.api.tasks.bundling.Jar 1

as opposed to the following larger output,

1 actionable task: 1 executed
Task counts: Old API 12, New API 74, total 86
Task counts: created 75, avoided 11, %-lazy 13

Task types that were created with the old API
class org.gradle.api.DefaultTask 8
class com.diffplug.gradle.spotless.SpotlessTask 3
class com.github.benmanes.gradle.versions.updates.DependencyUpdatesTask 1

Task types that were registered with the new API but were created anyways
class org.gradle.api.tasks.compile.JavaCompile 23
class org.gradle.api.DefaultTask 13
class org.gradle.api.tasks.Delete 9
class org.gradle.language.jvm.tasks.ProcessResources 3
class org.gradle.api.plugins.quality.Pmd 3
class org.gradle.api.tasks.testing.Test 1
class org.gradle.kotlin.dsl.accessors.tasks.PrintAccessors 1
class org.gradle.api.tasks.javadoc.Javadoc 1
class org.gradle.plugins.ide.eclipse.GenerateEclipseJdt 1
class org.gradle.plugins.ide.idea.GenerateIdeaModule 1
class org.gradle.plugins.ide.eclipse.GenerateEclipseClasspath 1
class org.gradle.plugins.ide.eclipse.GenerateEclipseProject 1
class org.gradle.kotlin.dsl.accessors.tasks.UpdateProjectSchema 1
class org.gradle.configuration.Help 1
class org.gradle.plugins.ide.idea.GenerateIdeaWorkspace 1
class org.gradle.api.tasks.bundling.Jar 1
class org.gradle.plugins.ide.idea.GenerateIdeaProject 1

[jbduncan]

@nedtwigg I'm in the middle of playing around with the Spotless source code to see if I can resolve this issue myself, but I'm unsure how to build the Spotless plugin and then expose it to jupiter-collection-testers to see if my changes work.

Do you have any advice as to how I could do this?

[nedtwigg]

how to build the Spotless plugin and then expose it to jupiter-collection-testers

I would setup mavenLocal() as a plugin repository in jupiter-collection-testers, and then in Spotless run gradlew publishToMavenLocal.

I think it might also work to use composite builds and add include '../spotless' in the settings.gradle for jupiter-collection-testers.

[jbduncan]

Thanks @nedtwigg! I'll give mavenLocal() and gradlew publishToMavenLocal a shot (and composite builds if needed). :)

[jbduncan]

Hmm, small problem: since Task Configuration Avoidance is a Gradle 4.9 feature, if I try to swap out eager Tasks for their lazy equivalent - TaskProviders - in the Spotless source code, it causes tests like FreshMarkExtensionTest to fail because those tests call GradleRunner.create().withGradleVersion("2.14.1"). ... .build().

I've considered changing this line in GradleIntegrationTest so all the relevant tests run withGradleVersion("4.9") instead of version "2.14.1", but I don't know if you'd like that idea @nedtwigg, since you expressed in #161 that you still ran some stuff on Gradle 2.14.1 and couldn't migrate to future versions.

I've also considered using reflection, but I don't know if it's appropriate or even where to begin with it, as my knowledge of reflection is still beginner-level.

@nedtwigg Thoughts?

[jbduncan]

Would something like JOOR's Reflect.compile(String, String) help here?

[nedtwigg]

I would update gradle for your local build to 4.9, and not worry about compatibility. Then I would benchmark.

For gradle 4.9, how much faster is a build with task configuration avoidance than without it? My suspicion is that Spotless will gain little from it, because we already went to great lengths with the FormatterStep.createLazy machinations to keep task configuration lazy.

If we need to drop support for 2.14, 3, or even 4.8 so that we can get a significant performance improvement, then that might be worth it. But if we're looking at a 10ms perf gain on a 5,000ms build, then I don't think it's worth taking on the maintenance burden of supporting an incubating API.

If the performance gains are substantial, we can then evaluate whether we do reflection, drop compat, etc.

[jbduncan]

Cool, thanks for sharing your thoughts @nedtwigg! I'll see how far I can go with making Spotless depend on Gradle 4.9.

I'll profile things at a later date, but (as I'll explain in my next comment), I'll profile Spotless against my project jupiter-collection-testers as well, as I believe that will be the best indicator of whether it's worth supporting the incubating API.

[jbduncan]

tl;dr: I agree that Spotless itself won’t gain much from task configuration avoidance, but I think it'll help most in situations where users (such as myself) use Spotless with other plugins that “plug” into the check task.

To explain what I mean, my project jupiter-collection-testers has a custom buildSrc plugin for running a code refactoring tool called Refaster against the project's Java source code. This Refaster plugin has two unique tasks: refasterApply and refasterCheck.

Like Spotless, these two tasks respectively apply code refactorings and check if the code is already refactored. Also like Spotless, refasterCheck makes itself a subtask of check, so each time I run gradlew check on my project, it also runs refasterCheck.

However, my plugin actually runs Refaster n * k times every time refaster[Apply|Check] is run, where n is the number of Refaster templates, and k is the number of Java source sets in my Gradle project. This is because:

1. Refaster can only run with one Refaster template at a time.
2. Refaster builds upon error-prone, so it's basically a custom Java compiler that refactors code a side-effect. This means that each time Refaster runs, it needs not only the Java sources to refactor but the classpath needed to compile those sources as well. For this reason, my plugin runs Refaster once per Gradle source set, since each source set provides not just Java sources but those sources' classpath too.

Under the hood, my plugin creates one JavaCompile task for each run of Refaster. This means when I run gradlew refasterCheck, it's not just running one task, it's really running (n * k) + 1 tasks.

And since my project imports Spotless, it eagerly configures (creates) all these tasks each and every time, even if I’m running an unrelated task like gradlew dependencies. This is because when Spotless is imported, it directly configures (and thus creates) check, meaning all its subtasks get eagerly created in my project too, including my Refaster plugin’s refasterCheck task and its JavaCompile sub-tasks. This, I suspect, takes up a bit of time.

Long story short, if Spotless can avoid configuring check directly and do it lazily via task configuration avoidance, then it would in theory let my project avoiding creating refasterCheck if I run e.g. gradlew dependencies, saving time.

[jbduncan]

After benchmarking, I've come to the conclusion that the benefit of implementing Task Configuration Avoidance in Spotless is just not significant enough for jupiter-collection-testers. Thus I've closed PR #277, at least for now.

@nedtwigg Would you like to keep this issue open, or close it for now?

[nedtwigg]

My vote is to close it. Search will dig it up if it becomes relevant again. Thanks for the hard work implementing and benchmarking, good to know the results :)

[jbduncan]

You're very welcome, and cheers @nedtwigg. :)

[jbduncan]

I'm re-opening this issue for now, to hopefully remind me to run the benchmarks again, which I now believe are misleading after a helpful comment from @thc202 implied that I benchmarked the wrong Gradle command!

[litpho]

We have noticed a rather large difference when configuration a custom format (the difference with just java isn't as remarkable).
Scan without Task Avoidance

Scan with Task Avoidance

Changes in Spotless plugin
spotless.patch.txt
Spotless configuration in target project
spotless-config.txt

[jbduncan]

Thank you @litpho, that's good to know! I'm pretty sure now that I just messed up with my earlier benchmarking, so re-benchmarking this PR is at the top of my open-source todo list.

And apologies for the overall lack of activity. I started a new job recently, so I'm not sure yet when I'll have the time and energy to continue working on this PR. But stay tuned, nonetheless. :)

[nedtwigg]

Just an aside, #348 revealed an oopsie that was causing Spotless' configuration times to be way higher than necessary. With that fixed, and our built-in lazy evaluation, my skepticism that this is a worthwhile investment for Spotless is renewed.

[jbduncan]

Ah okay, thanks for the update @nedtwigg.

Since my interest in this issue and the PR I opened for it (#277) dwindled quite a while ago, it would be perfectly fine with me if you decided to close this issue again. :)

[nedtwigg]

Looks like grolifant added a utility library for this.

[nedtwigg]

This issue and its PR's have been a bit of a time sink, so I'm closing this issue. I'm happy to reopen iff you have a build where configuration avoidance would improve performance, with profiling data to back it up. Spotless already has built-in lazy configuration, so I'm skeptical that we gain anything with this change.

If we ever need to release a 4.0 version of Spotless, sure, we'll definitely adopt the gradle built-in API for this. But unless we can show that the built-in API is faster (of which I am skeptical), then there's no reason to pay the cost.

[jbduncan]

@nedtwigg Agreed. Let's wait until if or when task configuration avoidance becomes the only option available. :)

[dmoidl]

Hi @nedtwigg,

since this issue already exists, I'm not opening a new one, but I still think that Spotless plugin not playing well with Gradle's configuration avoidance is an issue.

I recently stumbled upon this when I tried to use configuration avoidance to simplify our build script and realized it's not working. After some debugging I came to the very same conclusion as OP did - Spotless plugin actually causes some tasks to get configured, while these tasks would not get configured in its absence. This doesn't seem right to me.

You see, I don't really care about how Spotless plugin is coded and whether or not it uses the configuration avoidance internally for it's own stuff, but I do think that it should not mess with other tasks which aren't associated with spotless in any way. Not only because it's not necessary, but because some people (such as myself) might actually want to benefit from configuration avoidance in some way and Spotless plugin breaks this.

I didn't do any in-depth analysis, but just a quick check of the source code, yet I found this part of the code to be the cause of the problem. Obviously, iterating over these tasks makes Gradle configure them even though it normally wouldn't (using Gradle 5.6, btw.). Knowing that, I came up with a workaround: setting enforceCheck to false on the Spotless extension and manually setting spotlessCheck task as a dependency of Gradle's built-in check task does the trick.

Since I haven't yet had the need to study Gradle's Plugin API, I can't tell how difficult it would be to fix this. Nevertheless, I think it should be fixed and wanted to bring this up as IMHO, this is not just a performance issue, but an issue in general.

[nedtwigg]

Thanks for making this distinction @davidmoidl. I've opened a different issue to handle this case. If it turns out that it is impossible to fix that other issue without implementing this issue as well, then I'll be happy to revisit the compatibility guarantees which have caused me to close this issue.

[nedtwigg]

For the benefit of issue subs, this has been released (partially, see #444 for details) in 3.25.0.