Repository navigation
Make spotless tasks all cacheable with the Gradle Build Cache #280
Description
Activity
I'm skeptical that this will be a performance improvement. I'd want to see a benchmark before merging.
I'm guessing that the performance benefits won't be that large on a project with few sources, but on a project with thousands of files, I'm guessing that the build cache will provide a nice short circuit for an initial checkout and run of
spotlessCheck.@nedtwigg We have a customer that is currently working at optimizing their build using Gradle Enterprize. At this stage when they run
gradle clean buildthe majority of time is spent incom.diffplug.gradle.spotless.SpotlessTask. That meansclean buildruns in 4s instead of 1s, because everything else is available in the local cache.I can share a GE screenshot directly with you if necessary.
I did a quick investigation, and it looks like we can probably support this usecase by bumping our minimum-required gradle to
3.0, and we can certainly support it if we bump to3.5. We have to go all the way to4.9to get completely native config-avoidance APIs, which is the other sticking point we've had.No need for a screenshot, a gradle engineer with a real customer usecase is proof-enough for me that there's value in bumping the minimum. I'm definitely on-board with bumping to
3.0(3.5if necessary), and if this heavy-duty usecase can also show that we get measurable benefit from4.9then I'm okay with that too. Implementing this is not high on my apocalypse-todo-list, but releasing good PRs quickly is always high on my todo list, even in an apocalypse.Reacted by Jonathan LeitschuhThanks @nedtwigg. I took a deeper look and it's not going to be as simple as I'd hoped. It looks like
SpotlessTaskis used in 2 different "modes": check and apply, but in both cases the source input files are also registered as being output files for the task.I don't think this model is really correct:
- when doing a
spotlessJavaCheck, the java source files are inputs to the task, but they should not be part of the task outputs. - when doing a
spotlessJavaApply, the java source files are indeed both input and output of the task.
Caching for the check case makes sense, with the cache key being made up of the hash of all or the source files, plus any other input parameters.
Caching for the apply case doesn't really make sense. Since the task modifies it's inputs in place, it's not really sensible to consider caching the outputs for a set of inputs.What do you think? Would it make sense to split these 2 cases into 2 separate task implementations, thus allowing the inputs/outputs to be more correctly modelled?
- when doing a
I agree that it doesn't make sense to cache
apply. Forcheck, the only output that can be cached is effectivelyUnit- either the code is formatted correctly or it is not.I also agree that Spotless does not model the task correctly. I'm grateful to have a gradle dev look at it - the reason for the weird model that we chose is a limitation (as I understand it) in the Gradle incremental API. The downside of breaking it into two tasks is that you duplicate incremental-change tracking in
checkvsapply. For example, let's say you runcheckon a big project with no cache, it chugs for 3s and then passes. Then, after changing no files, you now runapply. Thatapplyought to know "since check is now up-to-date, so am I" and it can complete immediately, and as implemented today that happens. It's easy to create a situation wherecheckis now instantaneous for a clean checkout of master, but the day-to-day performance is strictly worse, becausecheckandapplycan no longer take advantage of each other's up-to-date knowledge.A best-of-both-worlds answer might be this:
- define
@CacheableTask SpotlessTaskasf(config, files) = filenames_with_dirty_formatting, and it writes out those filenames in a newline-delimited filebuild/spotless-dirty.txtas its only output file. SpotlessCheckTaskdepends onSpotlessTask, and its only input isbuild/spotless-dirty.txt, and it just throws an error if it's not empty.SpotlessApplyTaskdepends onSpotlessTask, and its only input is alsobuild/spotless-dirty.txt, but it will modify those files (although they aren't declared as outputs), which will trigger the incremental mechanism in@CacheableTask SpotlessTaskon the next run.
The hard part of the structure I outlined above is that
@CacheableTask SpotlessTaskwill have to usebuild/spotless-bad-formatting.txtas an input (not just an output), because it needs to remember that just because a file wasn't changed and thus passed along by the gradle incremental update system, that doesn't mean that now it's clean. To keep caching happy, probably it cannot be listed as an official input. This might open up a complicated bug, but it seems doable...I'm happy to split
SpotlessTask, but I think it's important forapplyandcheckto be in the same "up-to-date" pass, since they are linked so tightly (don't want to optimize for fresh checkout performance at the expense of normal usage performance).- define
Thanks for the detailed analysis.
First up, I would caution against making too strong assumptions when optimizing for "normal usage". In my experience, people tend to use tools in many varied and unexpected ways!
- I'm seeing fresh checkouts and ephemeral build agents used more and more frequently. (Yesterday I encountered a second customer who had 8s out of 16s build times taken up by
SpotlessTask) - A user may work on a small part of a large multiproject build: whenever retrieving other changes from VCS they would benefit greatly from caching of checks for other modules.
- It's possible that people run
check100s of times more often thanapply
Proposed solution
I think the solution you outline could work. What I like about this mechanism is that there's no single task that has the same inputs and outputs. However, if I understand correctly, one downside is that the analysis for any badly formatted file would need to be executed twice: once to generate the
spotless-dirty.txtfile and again to actually modify the target files.An alternative
Another possible solution would be for the main
SpotlessTaskto do the analysis for each changed file, and generate a patch to correct any file that has bad formatting. These patch files would be regular build outputs, and their generation would be easily cacheable. Then, thechecktask would simply fail if there were any patch files present, and theapplytask would apply the changes to the main source tree. Neither thecheckorapplytasks would need to be cacheable.Instead of using patch files, it would also be possible for the main
SpotlessTaskto simply write the "fixed" content for each bad file into it's build output directory. Then, theapplytask could simply copy these files into the main source tree.WDYT?
- I'm seeing fresh checkouts and ephemeral build agents used more and more frequently. (Yesterday I encountered a second customer who had 8s out of 16s build times taken up by
^^^ All excellent points. A few comments:
I love the patch idea, but I think that just writing-out the plain fixed content is probably easier to implement, debug, and maintain.
If you're writing out the "clean" state for every file, whether it is dirty or not, then you're copying the entire source tree on every run of
applyorcheck, and you're caching it as an output too (not just hashing as input). If you try to optimize a bit by not writing a file for clean files, as a way to signal that they are clean, then it gets tricky to do up-to-date checking. Gradle handles the 1:1 transformation of.java -> .classvery well, but.java -> either clean.java or nothingis a little less straightforward. For that latter case, you might still needbuild/spotless-dirty.txtto track that yourself.My instinct is that the formatter runs fast, and the disk is slow, but your caching results show that I'm wrong about that. I'm sure a gradle engineer has better heuristics around that than me. I'm always happy to merge performance-enhancing PRs with a real-world usecase that shows a speed increase. I definitely believe that caching
build/spotless-dirty.txtwill improve initial-checkout performance, with a small hit to "single-changed-file" performance, which is already very fast. I'm a little skeptical that caching the entire source-tree as an input and an output is a performance improvement, but I would take your word for it if you told me that it was.An issue that we're going to have (just popped up in #559) is that most of our
FormatterStepcache keys are machine-dependent at the moment. The task reorganization that @bigdaz has proposed is a definite improvement, and will make the local buildcache useful to users. But in order for Spotless to be path-independent (necessary for shared buildcache to be useful), we need to makeFileSignaturepath and machine independent. I think we should discuss and implement this separately, the issue lives at #566.Reacted by Jonathan Bluett-DuncanJust did a little experiment. Now that @bigdaz has implemented #576, it makes sense to mark
SpotlessTaskas@CacheableTask. And lucky for us, we can do that, and Gradle 2.x doesn't seem to mind. So we don't have to bump the minimum Gradle to support the buildcache. However, the buildcache will not work for the vast majority of cases until #566 is resolved. So rather than thrash bandwidth on the buildcache where we know the keys will never match, we might as well wait for #566 to get resolved. So whatever PR solved #566 can just add that one annotation and we should be good!However, the buildcache will not work for the vast majority of cases until #566 is resolved
Do you mean that even local cache doesn't work in many instances?
Because from what I've seen, runningcleanbuilds with a populated local cache is a common use case. In many cases there's no real reason forclean(it's hard to wean users offclean!). Another common use case involving the local cache is switching between local branches.Local cache will work in all instances. My impression was that the value came from the using the remote cache results on dev builds. I assumed that it was expensive to fill a remote build cache with stuff that will never hit anywhere besides the CI checkout path it was built on.
But it's very easy to turn on, so here it is! 971e32c. Hopefully we'll have a fix soon, so that even if we are thrashing a cache, we won't be for long.
- pinned this issue
on May 14, 2020 - unpinned this issue
on Jun 4, 2020 As of
plugin-gradle 4.5.0, the remote buildcache should be working. If anyone has problems with cache misses that they think should hit, please let us know in this issue.
This should be as simple as adding the
@CachableTaskannotation to all of the relevant tasks.