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

Use task configuration avoidance internally (Spotless already supports externally) #544

Closed
dcabasson opened this issue Mar 23, 2020 · 7 comments
Labels

Comments

@dcabasson
Copy link

See https://docs.gradle.org/current/userguide/task_configuration_avoidance.html for more context

Currently the spotless gradle plugin is eagerly creating tasks, which is not the recommended approach anymore.

I think for the most part, the change should be https://github.com/diffplug/spotless/blob/master/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java#L631 to use register rather than create and deal with the fact that this will be a TaskProvider rather than a Task.

@jbduncan
Copy link
Member

jbduncan commented Mar 23, 2020

Hi @dcabasson! This is a duplicate of #269, but I wouldn't blame you for not finding it; it was closed because although I opened a PR for it a long time ago, I never found the time and motivation to finish things off, and IIRC the issue was closed due to the lack of any promising speed improvement.

If you felt up to picking things up where I left off, then feel free to copy the code in my old PR and to create a new one. :)

@nedtwigg would be the best person to advise if the old issue should be reopened or not.

@dcabasson
Copy link
Author

Ah indeed. I went through the open tickets but I didn't find one which is why I created that one. I will work on the issue and propose a fix, time permitting.

It's more of a clean up exercise to ensure you are using the current recommended Gradle approach. Indeed the speed gain will be modest, but every little bit helps (or hurts depending on the point of view)

@nedtwigg
Copy link
Member

As of 3.25.0, Spotless does support config avoidance, in that we do not trigger other plugins which have implemented config avoidance (#463). If you look at that PR, note that there is one class SpotlessPluginConfigAvoidance and another SpotlessPluginLegacy, and we call one or the other based on what version of gradle is running.

Spotless has had internal config avoidance even before Gradle provided support for that. When you specify a config file to Spotless, Spotless does not read or parse it until/unless it is needed. I am very skeptical that there will be any performance benefit to adopting Gradle config avoidance, although I am all for adopting any Gradle feature if we can do so without bumping requirements.

Right now, we support all the way back to gradle 2.14. If we migrate to task config avoidance, that will bump our minimum gradle up to 4.9 (#504). As an example of the benefits of back-compat, we just recently had a long-outstanding bug which affected Spotless with all versions of gradle get fixed by a user who is stuck on Gradle 3.x.

I think it is good for us to break back-compat iff:

  • there is a new feature which allows greater performance
  • we are using an old API which is generating warning messages in the latest gradle.

Because of how Spotless is implemented, the perf gains to adopting config avoidance are too small to measure. The only way we could implement it without bumping requirements would be to have two implementations (as we did #463), which is needlessly complicated if we're not getting anything useful in return.

I'm leaving this open, but tagging it with wontfix. If something else happens which causes us to bump our minimum required gradle, then I'll happily merge this change. But I'm not going to bump our minimum requirements from 2.14 to 4.9 unless we get something for it, and at the moment we don't get anything. If someone figures out a clever way to implement this which doesn't require us to bump the minimum, and also doesn't make the implementation way more complicated, then I'd be open to merging a PR anyway, but I suspect it will require a lot of complication, all for little-to-no performance improvement.

@jbduncan
Copy link
Member

@nedtwigg Wow, I'd completely forgotten about that part of the whole story, so thank you for the thorough reminder and explanation. 👍

@nedtwigg nedtwigg changed the title Support task configuration avoidance Use task configuration avoidance internally (Spotless already supports externally) Mar 24, 2020
@bigdaz
Copy link
Contributor

bigdaz commented May 12, 2020

As a further data point, I just worked with a Gradle Enterprise customer that has a very large, very well optimized build. While I couldn't extract the actual impact on configuration time of the Spotless plugin, I did observe that out of 10,000 tasks being eagerly created by the build, 7000 of them were created by the Spotless plugin!

This was a multi-project build with around 500 subprojects. With 4 source sets configured for checks, Spotless eagerly creates 14 tasks per subproject.

I would suggest:

  • It would be worthwhile switching to use tasks.register in Spotless for Gradle versions where this is available.
  • If required it should be possible to maintain compatibility with older versions of Gradle, by using tasks.create when and older version of Gradle is detected.

@nedtwigg
Copy link
Member

Thanks for the datapoint! Here's my proposed roadmap.

@nedtwigg
Copy link
Member

nedtwigg commented Jun 5, 2020

See #601

@nedtwigg nedtwigg closed this as completed Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants