From 90778f72b1993aa5a24ede003fdb064044936d59 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Fri, 6 May 2022 13:04:23 -0700 Subject: [PATCH 1/8] GradleProvisioner throws `GradleException` instead of just `logger.error`, this way Gradle reports the root cause. --- .../com/diffplug/gradle/spotless/GradleProvisioner.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java index 4ef8521412..63697afca2 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java @@ -127,12 +127,10 @@ private static Provisioner forConfigurationContainer(Project project, Configurat if (!projName.isEmpty()) { projName = projName + "/"; } - logger.error( - "You need to add a repository containing the '{}' artifact in '{}build.gradle'.\n" + + throw new GradleException(String.format( + "You need to add a repository containing the '%s' artifact in '%sbuild.gradle'.\n" + "E.g.: 'repositories { mavenCentral() }'", - mavenCoords, projName, - e); - throw e; + mavenCoords, projName), e); } }; } From f32ebe876259e7b823dc1da7b3f9886391dfc827 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Fri, 6 May 2022 10:36:52 -0700 Subject: [PATCH 2/8] Fix caching memory leak in GitAttributesLineEndings. --- .../extra/GitAttributesLineEndings.java | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/lib-extra/src/main/java/com/diffplug/spotless/extra/GitAttributesLineEndings.java b/lib-extra/src/main/java/com/diffplug/spotless/extra/GitAttributesLineEndings.java index ae9f0ceec1..c4ca8edacb 100644 --- a/lib-extra/src/main/java/com/diffplug/spotless/extra/GitAttributesLineEndings.java +++ b/lib-extra/src/main/java/com/diffplug/spotless/extra/GitAttributesLineEndings.java @@ -15,8 +15,6 @@ */ package com.diffplug.spotless.extra; -import static com.diffplug.spotless.extra.LibExtraPreconditions.requireElementsNonNull; - import java.io.File; import java.io.FileInputStream; import java.io.IOException; @@ -78,8 +76,8 @@ public static LineEnding.Policy create(File projectDir, Supplier> static class RelocatablePolicy extends LazyForwardingEquality implements LineEnding.Policy { private static final long serialVersionUID = 5868522122123693015L; - final transient File projectDir; - final transient Supplier> toFormat; + transient File projectDir; + transient Supplier> toFormat; RelocatablePolicy(File projectDir, Supplier> toFormat) { this.projectDir = Objects.requireNonNull(projectDir, "projectDir"); @@ -88,8 +86,13 @@ static class RelocatablePolicy extends LazyForwardingEquality imp @Override protected CachedEndings calculateState() throws Exception { - Runtime runtime = new RuntimeInit(projectDir, toFormat.get()).atRuntime(); - return new CachedEndings(projectDir, runtime, toFormat.get()); + Runtime runtime = new RuntimeInit(projectDir).atRuntime(); + // LazyForwardingEquality guarantees that this will only be called once, and keeping toFormat + // causes a memory leak, see https://github.com/diffplug/spotless/issues/1194 + CachedEndings state = new CachedEndings(projectDir, runtime, toFormat.get()); + projectDir = null; + toFormat = null; + return state; } @Override @@ -146,8 +149,7 @@ static class RuntimeInit { final @Nullable File workTree; @SuppressFBWarnings("SIC_INNER_SHOULD_BE_STATIC_ANON") - RuntimeInit(File projectDir, Iterable toFormat) { - requireElementsNonNull(toFormat); + RuntimeInit(File projectDir) { ///////////////////////////////// // USER AND SYSTEM-WIDE VALUES // ///////////////////////////////// From 986cda366250925537005fe6072561043afa1f4b Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Fri, 6 May 2022 13:20:00 -0700 Subject: [PATCH 3/8] Added a lazy version of `createIndependentApplyTask`. --- .../gradle/spotless/FormatExtension.java | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java index d8c5525db6..94ae136429 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java @@ -38,6 +38,7 @@ import org.gradle.api.file.ConfigurableFileTree; import org.gradle.api.file.FileCollection; import org.gradle.api.plugins.BasePlugin; +import org.gradle.api.tasks.TaskProvider; import com.diffplug.common.base.Preconditions; import com.diffplug.spotless.FormatExceptionPolicyStrict; @@ -769,6 +770,11 @@ protected Project getProject() { return spotless.project; } + /** Eager version of {@link #createIndependentApplyTaskLazy(String)} */ + public SpotlessApply createIndependentApplyTask(String taskName) { + return createIndependentApplyTaskLazy(taskName).get(); + } + /** * Creates an independent {@link SpotlessApply} for (very) unusual circumstances. * @@ -782,19 +788,19 @@ protected Project getProject() { * * NOTE: does not respect the rarely-used {@code spotlessFiles} property. */ - public SpotlessApply createIndependentApplyTask(String taskName) { + public TaskProvider createIndependentApplyTaskLazy(String taskName) { Preconditions.checkArgument(!taskName.endsWith(SpotlessExtension.APPLY), "Task name must not end with " + SpotlessExtension.APPLY); - // create and setup the task - SpotlessTaskImpl spotlessTask = spotless.project.getTasks().create(taskName + SpotlessTaskService.INDEPENDENT_HELPER, SpotlessTaskImpl.class); - spotlessTask.init(spotless.getRegisterDependenciesTask().getTaskService()); - setupTask(spotlessTask); - // clean removes the SpotlessCache, so we have to run after clean - spotlessTask.mustRunAfter(BasePlugin.CLEAN_TASK_NAME); + TaskProvider spotlessTask = spotless.project.getTasks().register(taskName + SpotlessTaskService.INDEPENDENT_HELPER, SpotlessTaskImpl.class, task -> { + task.init(spotless.getRegisterDependenciesTask().getTaskService()); + setupTask(task); + // clean removes the SpotlessCache, so we have to run after clean + task.mustRunAfter(BasePlugin.CLEAN_TASK_NAME); + }); // create the apply task - SpotlessApply applyTask = spotless.project.getTasks().create(taskName, SpotlessApply.class); - applyTask.init(spotlessTask); - applyTask.dependsOn(spotlessTask); - + TaskProvider applyTask = spotless.project.getTasks().register(taskName, SpotlessApply.class, task -> { + task.dependsOn(spotlessTask); + task.init(spotlessTask.get()); + }); return applyTask; } From 9b5510af72cf560948d0886c0694cfee2f9a9764 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Fri, 6 May 2022 13:49:24 -0700 Subject: [PATCH 4/8] Fix caching memory leak in lazy FormatterSteps. --- .../java/com/diffplug/spotless/FormatterStepImpl.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/src/main/java/com/diffplug/spotless/FormatterStepImpl.java b/lib/src/main/java/com/diffplug/spotless/FormatterStepImpl.java index 061aff6af9..7663145f51 100644 --- a/lib/src/main/java/com/diffplug/spotless/FormatterStepImpl.java +++ b/lib/src/main/java/com/diffplug/spotless/FormatterStepImpl.java @@ -1,5 +1,5 @@ /* - * Copyright 2016-2020 DiffPlug + * Copyright 2016-2022 DiffPlug * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -39,7 +39,7 @@ abstract class FormatterStepImpl extends Strict stateSupplier; + transient ThrowingEx.Supplier stateSupplier; FormatterStepImpl(String name, ThrowingEx.Supplier stateSupplier) { this.name = Objects.requireNonNull(name); @@ -53,7 +53,11 @@ public String getName() { @Override protected State calculateState() throws Exception { - return stateSupplier.get(); + // LazyForwardingEquality guarantees that this will only be called once, and keeping toFormat + // causes a memory leak, see https://github.com/diffplug/spotless/issues/1194 + State state = stateSupplier.get(); + stateSupplier = null; + return state; } static final class Standard extends FormatterStepImpl { From a0a96e379c31b092085bc9f4cf7591ce4df8b458 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sun, 8 May 2022 18:11:52 -0700 Subject: [PATCH 5/8] Update changelogs. --- CHANGES.md | 1 + plugin-gradle/CHANGES.md | 3 +++ 2 files changed, 4 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index e7427e178d..7eb1c33afe 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -12,6 +12,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format ( ## [Unreleased] ### Fixed * Update the `black` version regex to fix `19.10b0` and earlier. (fixes [#1195](https://github.com/diffplug/spotless/issues/1195), regression introduced in `2.25.0`) +* `GitAttributesLineEndings$RelocatablePolicy` and `FormatterStepImpl` now null-out their initialization lambdas after their state has been calculated, which allows GC to collect variables which were incidentally captured but not needed in the calculated state. ([#1198](https://github.com/diffplug/spotless/pull/1198)) ## [2.25.2] - 2022-05-03 ### Changes diff --git a/plugin-gradle/CHANGES.md b/plugin-gradle/CHANGES.md index 963d6d06d7..b8531d3812 100644 --- a/plugin-gradle/CHANGES.md +++ b/plugin-gradle/CHANGES.md @@ -3,8 +3,11 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (starting after version `3.27.0`). ## [Unreleased] +### Added +* `FormatExtension.createIndependentApplyTaskLazy`, with same functionality as `createIndependentApplyTaskLazy` but returning `TaskProvider` ([#1198](https://github.com/diffplug/spotless/pull/1198)) ### Fixed * Update the `black` version regex to fix `19.10b0` and earlier. (fixes [#1195](https://github.com/diffplug/spotless/issues/1195), regression introduced in `6.5.0`) +* Improved daemon memory consumption ([#1198](https://github.com/diffplug/spotless/pull/1198) fixes [#1194](https://github.com/diffplug/spotless/issues/1194)) ## [6.5.2] - 2022-05-03 ### Changes From 2be12910f62f2883330e064f44a6cc2f74ac52bf Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Mon, 9 May 2022 16:51:07 -0700 Subject: [PATCH 6/8] JvmLocalCache should force state evaluation of any LazyForwardingEquality to give it a chance to null-out its initializing lambda. Addresses https://github.com/diffplug/spotless/issues/1194#issuecomment-1120744842 --- .../com/diffplug/spotless/LazyForwardingEquality.java | 7 ++++++- .../java/com/diffplug/gradle/spotless/JvmLocalCache.java | 8 +++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/src/main/java/com/diffplug/spotless/LazyForwardingEquality.java b/lib/src/main/java/com/diffplug/spotless/LazyForwardingEquality.java index 05e758d586..e29464bd91 100644 --- a/lib/src/main/java/com/diffplug/spotless/LazyForwardingEquality.java +++ b/lib/src/main/java/com/diffplug/spotless/LazyForwardingEquality.java @@ -1,5 +1,5 @@ /* - * Copyright 2016 DiffPlug + * Copyright 2016-2022 DiffPlug * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -111,4 +111,9 @@ static byte[] toBytes(Serializable obj) { } return byteOutput.toByteArray(); } + + /** Ensures that the lazy state has been evaluated. */ + public static void unlazy(LazyForwardingEquality in) { + in.state(); + } } diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/JvmLocalCache.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/JvmLocalCache.java index 9478e9e66f..3f03a5852b 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/JvmLocalCache.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/JvmLocalCache.java @@ -1,5 +1,5 @@ /* - * Copyright 2021 DiffPlug + * Copyright 2021-2022 DiffPlug * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -26,6 +26,7 @@ import org.gradle.api.Task; import com.diffplug.spotless.FileSignature; +import com.diffplug.spotless.LazyForwardingEquality; class JvmLocalCache { private static GradleException cacheIsStale() { @@ -53,6 +54,11 @@ static class LiveCacheKeyImpl implements LiveCache, Serializable { @Override public void set(T value) { + if (value instanceof LazyForwardingEquality) { + // whenever we cache an instance of LazyForwardingEquality, we want to make sure that we give it + // a chance to null-out its initialization lambda (see https://github.com/diffplug/spotless/issues/1194#issuecomment-1120744842) + LazyForwardingEquality.unlazy((LazyForwardingEquality) value); + } daemonState.put(internalKey, value); } From 424e42aa93c21f0d02efad28882453857abb1dde Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 10 May 2022 09:45:41 -0700 Subject: [PATCH 7/8] LazyForwardingEquality can now unlazy a `FormatterStep` which has a delegate field, a well as a `List`. Fixes https://github.com/diffplug/spotless/issues/1194#issuecomment-1122034178 --- .../spotless/DelegateFormatterStep.java | 32 +++++++++++++++++++ .../FilterByContentPatternFormatterStep.java | 14 ++------ .../spotless/FilterByFileFormatterStep.java | 12 ++----- .../spotless/LazyForwardingEquality.java | 13 ++++++-- 4 files changed, 49 insertions(+), 22 deletions(-) create mode 100644 lib/src/main/java/com/diffplug/spotless/DelegateFormatterStep.java diff --git a/lib/src/main/java/com/diffplug/spotless/DelegateFormatterStep.java b/lib/src/main/java/com/diffplug/spotless/DelegateFormatterStep.java new file mode 100644 index 0000000000..829652a5dc --- /dev/null +++ b/lib/src/main/java/com/diffplug/spotless/DelegateFormatterStep.java @@ -0,0 +1,32 @@ +/* + * Copyright 2022 DiffPlug + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.diffplug.spotless; + +import java.util.Objects; + +/** Superclass of all compound FormatterSteps necessary for {@link com.diffplug.spotless.LazyForwardingEquality#unlazy(java.lang.Object)}. */ +abstract class DelegateFormatterStep implements FormatterStep { + protected final FormatterStep delegateStep; + + DelegateFormatterStep(FormatterStep delegateStep) { + this.delegateStep = Objects.requireNonNull(delegateStep); + } + + @Override + public final String getName() { + return delegateStep.getName(); + } +} diff --git a/lib/src/main/java/com/diffplug/spotless/FilterByContentPatternFormatterStep.java b/lib/src/main/java/com/diffplug/spotless/FilterByContentPatternFormatterStep.java index 9b39361719..d9a6fe4093 100644 --- a/lib/src/main/java/com/diffplug/spotless/FilterByContentPatternFormatterStep.java +++ b/lib/src/main/java/com/diffplug/spotless/FilterByContentPatternFormatterStep.java @@ -1,5 +1,5 @@ /* - * Copyright 2016-2021 DiffPlug + * Copyright 2016-2022 DiffPlug * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -22,27 +22,19 @@ import javax.annotation.Nullable; -final class FilterByContentPatternFormatterStep implements FormatterStep { - private final FormatterStep delegateStep; +final class FilterByContentPatternFormatterStep extends DelegateFormatterStep { final Pattern contentPattern; FilterByContentPatternFormatterStep(FormatterStep delegateStep, String contentPattern) { - this.delegateStep = Objects.requireNonNull(delegateStep); + super(delegateStep); this.contentPattern = Pattern.compile(Objects.requireNonNull(contentPattern)); } - @Override - public String getName() { - return delegateStep.getName(); - } - @Override public @Nullable String format(String raw, File file) throws Exception { Objects.requireNonNull(raw, "raw"); Objects.requireNonNull(file, "file"); - Matcher matcher = contentPattern.matcher(raw); - if (matcher.find()) { return delegateStep.format(raw, file); } else { diff --git a/lib/src/main/java/com/diffplug/spotless/FilterByFileFormatterStep.java b/lib/src/main/java/com/diffplug/spotless/FilterByFileFormatterStep.java index 2fd221220c..04a06a4673 100644 --- a/lib/src/main/java/com/diffplug/spotless/FilterByFileFormatterStep.java +++ b/lib/src/main/java/com/diffplug/spotless/FilterByFileFormatterStep.java @@ -1,5 +1,5 @@ /* - * Copyright 2016 DiffPlug + * Copyright 2016-2022 DiffPlug * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,20 +20,14 @@ import javax.annotation.Nullable; -final class FilterByFileFormatterStep implements FormatterStep { - private final FormatterStep delegateStep; +final class FilterByFileFormatterStep extends DelegateFormatterStep { private final SerializableFileFilter filter; FilterByFileFormatterStep(FormatterStep delegateStep, SerializableFileFilter filter) { - this.delegateStep = Objects.requireNonNull(delegateStep); + super(delegateStep); this.filter = Objects.requireNonNull(filter); } - @Override - public String getName() { - return delegateStep.getName(); - } - @Override public @Nullable String format(String raw, File file) throws Exception { Objects.requireNonNull(raw, "raw"); diff --git a/lib/src/main/java/com/diffplug/spotless/LazyForwardingEquality.java b/lib/src/main/java/com/diffplug/spotless/LazyForwardingEquality.java index e29464bd91..e2d6ba8988 100644 --- a/lib/src/main/java/com/diffplug/spotless/LazyForwardingEquality.java +++ b/lib/src/main/java/com/diffplug/spotless/LazyForwardingEquality.java @@ -113,7 +113,16 @@ static byte[] toBytes(Serializable obj) { } /** Ensures that the lazy state has been evaluated. */ - public static void unlazy(LazyForwardingEquality in) { - in.state(); + public static void unlazy(Object in) { + if (in instanceof LazyForwardingEquality) { + ((LazyForwardingEquality) in).state(); + } else if (in instanceof DelegateFormatterStep) { + unlazy(((DelegateFormatterStep) in).delegateStep); + } else if (in instanceof Iterable) { + Iterable cast = (Iterable) in; + for (Object c : cast) { + unlazy(c); + } + } } } From 628c5fcbe3e1ed85c895c0d2fe5bf9ebadf47bd6 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 10 May 2022 13:32:28 -0700 Subject: [PATCH 8/8] Fix spotbugs warning about `\n` vs `%n`. --- .../java/com/diffplug/gradle/spotless/GradleProvisioner.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java index 63697afca2..97f051cd86 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/GradleProvisioner.java @@ -128,7 +128,7 @@ private static Provisioner forConfigurationContainer(Project project, Configurat projName = projName + "/"; } throw new GradleException(String.format( - "You need to add a repository containing the '%s' artifact in '%sbuild.gradle'.\n" + + "You need to add a repository containing the '%s' artifact in '%sbuild.gradle'.%n" + "E.g.: 'repositories { mavenCentral() }'", mavenCoords, projName), e); }