From 2097cadb878ff3d2c063e8a5d8e4390f3ab06b69 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 14 Dec 2021 14:04:39 -0800 Subject: [PATCH 1/5] Improve the performance of GitRatchet. --- .../com/diffplug/spotless/extra/GitRatchet.java | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lib-extra/src/main/java/com/diffplug/spotless/extra/GitRatchet.java b/lib-extra/src/main/java/com/diffplug/spotless/extra/GitRatchet.java index 5ce8173845..6b8d5a9f41 100644 --- a/lib-extra/src/main/java/com/diffplug/spotless/extra/GitRatchet.java +++ b/lib-extra/src/main/java/com/diffplug/spotless/extra/GitRatchet.java @@ -63,6 +63,8 @@ public boolean isClean(Project project, ObjectId treeSha, File file) throws IOEx return isClean(project, treeSha, relativePath); } + private Map dirCaches = new HashMap<>(); + /** * This is the highest-level method, which all the others serve. Given the sha * of a git tree (not a commit!), and the file in question, this method returns @@ -72,13 +74,16 @@ public boolean isClean(Project project, ObjectId treeSha, File file) throws IOEx public boolean isClean(Project project, ObjectId treeSha, String relativePathUnix) throws IOException { Repository repo = repositoryFor(project); - // TODO: should be cached-per-repo if it is thread-safe, or per-repo-per-thread if it is not - DirCache dirCache = repo.readDirCache(); - + DirCacheIterator dirCacheIteratorInit; + synchronized (this) { + // each DirCache is thread-safe, and we compute them one-to-one based on `repositoryFor` + DirCache dirCache = dirCaches.computeIfAbsent(repo, Errors.rethrow().wrap(Repository::readDirCache)); + dirCacheIteratorInit = new DirCacheIterator(dirCache); + } try (TreeWalk treeWalk = new TreeWalk(repo)) { treeWalk.setRecursive(true); treeWalk.addTree(treeSha); - treeWalk.addTree(new DirCacheIterator(dirCache)); + treeWalk.addTree(dirCacheIteratorInit); treeWalk.addTree(new FileTreeIterator(repo)); treeWalk.setFilter(AndTreeFilter.create( PathFilter.create(relativePathUnix), @@ -216,7 +221,7 @@ public synchronized ObjectId rootTreeShaOf(Project project, String reference) { RevCommit mergeBase = revWalk.next(); treeSha = Optional.ofNullable(mergeBase).orElse(ratchetFrom).getTree(); } - rootTreeShaCache.put(repo, reference, treeSha); + rootTreeShaCache.put(repo, reference, treeSha.copy()); } return treeSha; } catch (IOException e) { @@ -241,7 +246,7 @@ public synchronized ObjectId subtreeShaOf(Project project, ObjectId rootTreeSha) TreeWalk treeWalk = TreeWalk.forPath(repo, subpath, rootTreeSha); subtreeSha = treeWalk == null ? ObjectId.zeroId() : treeWalk.getObjectId(0); } - subtreeShaCache.put(project, subtreeSha); + subtreeShaCache.put(project, subtreeSha.copy()); } return subtreeSha; } catch (IOException e) { From e6561907ab3866936eecb1593fef3f9d2b7700ff Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 14 Dec 2021 16:31:22 -0800 Subject: [PATCH 2/5] SpotlessTask no longer hangs onto a GitRatchet reference. --- .../java/com/diffplug/gradle/spotless/IdeHook.java | 5 +++-- .../com/diffplug/gradle/spotless/SpotlessTask.java | 13 ++----------- .../diffplug/gradle/spotless/SpotlessTaskImpl.java | 12 ++++++++---- 3 files changed, 13 insertions(+), 17 deletions(-) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/IdeHook.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/IdeHook.java index dd62cd523a..cfb9817d63 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/IdeHook.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/IdeHook.java @@ -41,9 +41,10 @@ static void performHook(SpotlessTaskImpl spotlessTask) { return; } if (spotlessTask.getTarget().contains(file)) { + GitRatchetGradle ratchet = spotlessTask.getTaskService().get().getRatchet(); try (Formatter formatter = spotlessTask.buildFormatter()) { - if (spotlessTask.getRatchet() != null) { - if (spotlessTask.getRatchet().isClean(spotlessTask.getProjectDir().get().getAsFile(), spotlessTask.getRootTreeSha(), file)) { + if (ratchet != null) { + if (ratchet.isClean(spotlessTask.getProjectDir().get().getAsFile(), spotlessTask.getRootTreeSha(), file)) { dumpIsClean(); return; } diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java index fe5628bb83..613e00f0ac 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java @@ -23,8 +23,6 @@ import java.util.Locale; import java.util.Objects; -import javax.annotation.Nullable; - import org.eclipse.jgit.lib.ObjectId; import org.gradle.api.DefaultTask; import org.gradle.api.file.DirectoryProperty; @@ -44,6 +42,7 @@ import com.diffplug.spotless.Formatter; import com.diffplug.spotless.FormatterStep; import com.diffplug.spotless.LineEnding; +import com.diffplug.spotless.extra.GitRatchet; public abstract class SpotlessTask extends DefaultTask { @Internal @@ -76,9 +75,6 @@ public void setLineEndingsPolicy(LineEnding.Policy lineEndingsPolicy) { this.lineEndingsPolicy.set(lineEndingsPolicy); } - /*** API which performs git up-to-date tasks. */ - @Nullable - private transient GitRatchetGradle ratchet; /** The sha of the tree at repository root, used for determining if an individual *file* is clean according to git. */ private transient ObjectId rootTreeSha; /** @@ -93,7 +89,7 @@ public void setLineEndingsPolicy(LineEnding.Policy lineEndingsPolicy) { public void setupRatchet(String ratchetFrom) { this.ratchetFrom = ratchetFrom; if (!ratchetFrom.isEmpty()) { - ratchet = getTaskService().get().getRatchet(); + GitRatchet ratchet = getTaskService().get().getRatchet(); File projectDir = getProjectDir().get().getAsFile(); rootTreeSha = ratchet.rootTreeShaOf(projectDir, ratchetFrom); subtreeSha = ratchet.subtreeShaOf(projectDir, rootTreeSha); @@ -105,11 +101,6 @@ public void setupRatchet(String ratchetFrom) { @Internal abstract DirectoryProperty getProjectDir(); - @Internal - GitRatchetGradle getRatchet() { - return ratchet; - } - @Internal ObjectId getRootTreeSha() { return rootTreeSha; diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java index a9bf428caa..e191e3e8fe 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java @@ -21,6 +21,7 @@ import java.nio.file.Path; import java.nio.file.StandardCopyOption; +import javax.annotation.Nullable; import javax.inject.Inject; import org.gradle.api.GradleException; @@ -37,6 +38,7 @@ import com.diffplug.common.base.StringPrinter; import com.diffplug.spotless.Formatter; import com.diffplug.spotless.PaddedCell; +import com.diffplug.spotless.extra.GitRatchet; @CacheableTask public abstract class SpotlessTaskImpl extends SpotlessTask { @@ -53,10 +55,12 @@ void init(Provider service) { @TaskAction public void performAction(InputChanges inputs) throws Exception { - getTaskService().get().registerSourceAlreadyRan(this); + SpotlessTaskService taskService = getTaskService().get(); + taskService.registerSourceAlreadyRan(this); if (target == null) { throw new GradleException("You must specify 'Iterable target'"); } + GitRatchetGradle ratchet = taskService.getRatchet(); if (!inputs.isIncremental()) { getLogger().info("Not incremental: removing prior outputs"); @@ -71,18 +75,18 @@ public void performAction(InputChanges inputs) throws Exception { deletePreviousResult(input); } else { if (input.isFile()) { - processInputFile(formatter, input); + processInputFile(ratchet, formatter, input); } } } } } - private void processInputFile(Formatter formatter, File input) throws IOException { + private void processInputFile(@Nullable GitRatchet ratchet, Formatter formatter, File input) throws IOException { File output = getOutputFile(input); getLogger().debug("Applying format to " + input + " and writing to " + output); PaddedCell.DirtyState dirtyState; - if (getRatchet() != null && getRatchet().isClean(getProjectDir().get().getAsFile(), getRootTreeSha(), input)) { + if (ratchet != null && ratchet.isClean(getProjectDir().get().getAsFile(), getRootTreeSha(), input)) { dirtyState = PaddedCell.isClean(); } else { dirtyState = PaddedCell.calculateDirtyState(formatter, input); From 5842d8336fceefffc371ba320ad5d301f500ed90 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 14 Dec 2021 16:36:48 -0800 Subject: [PATCH 3/5] Update changelogs. --- CHANGES.md | 2 ++ plugin-gradle/CHANGES.md | 2 ++ plugin-maven/CHANGES.md | 2 ++ 3 files changed, 6 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index a187f62b23..25cf0ccdea 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -10,6 +10,8 @@ This document is intended for Spotless developers. We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (starting after version `1.27.0`). ## [Unreleased] +### Fixed +* Performance improvements to GitRatchet ([#1038](https://github.com/diffplug/spotless/pull/1038)). ## [2.20.2] - 2021-12-05 ### Changed diff --git a/plugin-gradle/CHANGES.md b/plugin-gradle/CHANGES.md index 5b1a0a8adf..a1f7a7e730 100644 --- a/plugin-gradle/CHANGES.md +++ b/plugin-gradle/CHANGES.md @@ -3,6 +3,8 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (starting after version `3.27.0`). ## [Unreleased] +### Fixed +* `ratchetFrom` is now faster and uses less memory ([#1038](https://github.com/diffplug/spotless/pull/1038)). ## [6.0.4] - 2021-12-07 ### Fixed diff --git a/plugin-maven/CHANGES.md b/plugin-maven/CHANGES.md index bf76b48938..1da76e4fd6 100644 --- a/plugin-maven/CHANGES.md +++ b/plugin-maven/CHANGES.md @@ -3,6 +3,8 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (starting after version `1.27.0`). ## [Unreleased] +### Fixed +* `ratchetFrom` is now faster ([#1038](https://github.com/diffplug/spotless/pull/1038)). ## [2.17.6] - 2021-12-05 ### Changed From 1093781a0e8320a3bbdc67f576f57aa79e2ca29c Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 15 Dec 2021 13:06:50 -0800 Subject: [PATCH 4/5] GitRatchetGradle is meant to be null sometimes, depending on how the task is setup. --- .../src/main/java/com/diffplug/gradle/spotless/IdeHook.java | 2 +- .../main/java/com/diffplug/gradle/spotless/SpotlessTask.java | 5 +++++ .../java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/IdeHook.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/IdeHook.java index cfb9817d63..8d24e2230e 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/IdeHook.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/IdeHook.java @@ -41,7 +41,7 @@ static void performHook(SpotlessTaskImpl spotlessTask) { return; } if (spotlessTask.getTarget().contains(file)) { - GitRatchetGradle ratchet = spotlessTask.getTaskService().get().getRatchet(); + GitRatchetGradle ratchet = spotlessTask.getRatchet(); try (Formatter formatter = spotlessTask.buildFormatter()) { if (ratchet != null) { if (ratchet.isClean(spotlessTask.getProjectDir().get().getAsFile(), spotlessTask.getRootTreeSha(), file)) { diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java index 613e00f0ac..6f2279b2e0 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java @@ -101,6 +101,11 @@ public void setupRatchet(String ratchetFrom) { @Internal abstract DirectoryProperty getProjectDir(); + @Internal + GitRatchetGradle getRatchet() { + return ObjectId.zeroId().equals(getRatchetSha()) ? null : getTaskService().get().getRatchet(); + } + @Internal ObjectId getRootTreeSha() { return rootTreeSha; diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java index e191e3e8fe..1b9cda204c 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTaskImpl.java @@ -60,7 +60,6 @@ public void performAction(InputChanges inputs) throws Exception { if (target == null) { throw new GradleException("You must specify 'Iterable target'"); } - GitRatchetGradle ratchet = taskService.getRatchet(); if (!inputs.isIncremental()) { getLogger().info("Not incremental: removing prior outputs"); @@ -69,6 +68,7 @@ public void performAction(InputChanges inputs) throws Exception { } try (Formatter formatter = buildFormatter()) { + GitRatchetGradle ratchet = getRatchet(); for (FileChange fileChange : inputs.getFileChanges(target)) { File input = fileChange.getFile(); if (fileChange.getChangeType() == ChangeType.REMOVED) { From 80afe4bfcbd7a74dd39e046ae6a17a7927043d2b Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 15 Dec 2021 14:34:25 -0800 Subject: [PATCH 5/5] Exclude maven fom npmTest because its timing out too often. --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 4667ee2fc5..2fe588a448 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -109,7 +109,7 @@ jobs: - *restore_cache_deps - run: name: gradlew npmTest - command: ./gradlew npmTest --build-cache + command: export SPOTLESS_EXCLUDE_MAVEN=true && ./gradlew npmTest --build-cache - store_test_results: path: testlib/build/test-results/NpmTest - store_test_results: