Skip to content

Commit bb69d7d

Browse files
authored
Speed up java formatting tasks (#15386)
* Enable configuration cache for :help (as a test). * Enable configuration cache for :showTestsSeed. * Refactor ModularPathsExtension to enable configuration cache (compileJava works). * Make the test task support the configuration-cache. * Speed up google java format even on non-incremental runs by storing our own file state cache.
1 parent 146ef03 commit bb69d7d

File tree

5 files changed

+177
-48
lines changed

5 files changed

+177
-48
lines changed

build-tools/build-infra/src/main/java/org/apache/lucene/gradle/plugins/spotless/ApplyGoogleJavaFormatTask.java

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -45,29 +45,33 @@ public void formatSources(InputChanges inputChanges) throws IOException {
4545
WorkQueue workQueue = getWorkQueue();
4646
List<File> sourceFiles = getIncrementalBatch(inputChanges);
4747

48-
if (sourceFiles.isEmpty()) {
49-
return;
50-
}
48+
var fileStates = readFileStatesFrom(getFileStateCache());
49+
sourceFiles = maybeRefilter(fileStates, sourceFiles);
5150

5251
getLogger()
5352
.info(
5453
"Will apply formatting to {} source {} in this run.",
5554
sourceFiles.size(),
5655
sourceFiles.size() == 1 ? "file" : "files");
5756

58-
for (var batch : batchSourceFiles(sourceFiles)) {
59-
workQueue.submit(
60-
ApplyGoogleJavaFormatAction.class,
61-
params -> {
62-
params.getTargetFiles().setFrom(batch);
63-
});
57+
if (!sourceFiles.isEmpty()) {
58+
for (var batch : batchSourceFiles(sourceFiles)) {
59+
workQueue.submit(
60+
ApplyGoogleJavaFormatAction.class,
61+
params -> {
62+
params.getTargetFiles().setFrom(batch);
63+
});
64+
}
65+
66+
workQueue.await();
67+
68+
updateFileStates(fileStates, sourceFiles);
69+
writeFileStates(getFileStateCache(), fileStates);
6470
}
6571

6672
Path taskOutput = getOutputChangeListFile().get().getAsFile().toPath();
6773
Files.writeString(
6874
taskOutput, sourceFiles.stream().map(File::getPath).collect(Collectors.joining("\n")));
69-
70-
workQueue.await();
7175
}
7276

7377
public abstract static class ApplyGoogleJavaFormatAction

build-tools/build-infra/src/main/java/org/apache/lucene/gradle/plugins/spotless/CheckGoogleJavaFormatTask.java

Lines changed: 41 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -82,53 +82,60 @@ public CheckGoogleJavaFormatTask(ProjectLayout layout) {
8282

8383
@TaskAction
8484
public void formatSources(InputChanges inputChanges) throws IOException {
85-
WorkQueue workQueue = getWorkQueue();
8685
List<File> sourceFiles = getIncrementalBatch(inputChanges);
87-
if (sourceFiles.isEmpty()) {
88-
return;
89-
}
86+
87+
var fileStates = readFileStatesFrom(getFileStateCache());
88+
sourceFiles = maybeRefilter(fileStates, sourceFiles);
9089

9190
getLogger()
9291
.info(
9392
"Will check the formatting of {} source {} in this run.",
9493
sourceFiles.size(),
9594
sourceFiles.size() == 1 ? "file" : "files");
9695

97-
int outputSeq = 0;
98-
File tempDir = new File(getTemporaryDir(), "changes");
99-
if (Files.exists(tempDir.toPath())) {
100-
getFilesystemOperations().delete(spec -> spec.delete(tempDir));
101-
}
102-
Files.createDirectory(tempDir.toPath());
103-
104-
for (List<File> batch : batchSourceFiles(sourceFiles)) {
105-
final int seq = outputSeq;
106-
workQueue.submit(
107-
CheckGoogleJavaFormatAction.class,
108-
params -> {
109-
params.getTargetFiles().setFrom(batch);
110-
params.getOutputFile().set(new File(tempDir, "gjf-check-" + seq + ".txt"));
111-
});
112-
outputSeq++;
113-
}
114-
workQueue.await();
96+
if (!sourceFiles.isEmpty()) {
97+
int outputSeq = 0;
98+
File tempDir = new File(getTemporaryDir(), "changes");
99+
if (Files.exists(tempDir.toPath())) {
100+
getFilesystemOperations().delete(spec -> spec.delete(tempDir));
101+
}
102+
Files.createDirectory(tempDir.toPath());
103+
104+
WorkQueue workQueue = getWorkQueue();
105+
for (List<File> batch : batchSourceFiles(sourceFiles)) {
106+
final int seq = outputSeq;
107+
workQueue.submit(
108+
CheckGoogleJavaFormatAction.class,
109+
params -> {
110+
params.getTargetFiles().setFrom(batch);
111+
params.getOutputFile().set(new File(tempDir, "gjf-check-" + seq + ".txt"));
112+
});
113+
outputSeq++;
114+
}
115115

116-
// Check if there are any changes.
117-
try (Stream<Path> stream = Files.list(tempDir.toPath()).sorted()) {
118-
var allChanges = stream.toList();
116+
// Wait for all jobs.
117+
workQueue.await();
119118

120-
if (!allChanges.isEmpty()) {
121-
var errorMsg =
122-
"java file(s) have google-java-format violations (run './gradlew tidy' to fix).";
119+
// Check if there are any changes.
120+
try (Stream<Path> stream = Files.list(tempDir.toPath()).sorted()) {
121+
var allChanges = stream.toList();
123122

124-
if (getColorizedOutput().getOrElse(false)) {
125-
printChangesToColorizedTerminal(allChanges, errorMsg);
126-
} else {
127-
printChangesToLogger(allChanges, errorMsg);
128-
}
123+
if (!allChanges.isEmpty()) {
124+
var errorMsg =
125+
"java file(s) have google-java-format violations (run './gradlew tidy' to fix).";
126+
127+
if (getColorizedOutput().getOrElse(false)) {
128+
printChangesToColorizedTerminal(allChanges, errorMsg);
129+
} else {
130+
printChangesToLogger(allChanges, errorMsg);
131+
}
129132

130-
throw new GradleException(errorMsg);
133+
throw new GradleException(errorMsg);
134+
}
131135
}
136+
137+
updateFileStates(fileStates, sourceFiles);
138+
writeFileStates(getFileStateCache(), fileStates);
132139
}
133140

134141
Path taskOutput = getOutputChangeListFile().get().getAsFile().toPath();
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to You under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
package org.apache.lucene.gradle.plugins.spotless;
18+
19+
import java.io.IOException;
20+
import java.nio.file.Files;
21+
import java.nio.file.Path;
22+
23+
/**
24+
* The "state" of a source file. We'll just follow rsync and assume that a file with the same size
25+
* and the same last-update timestamp hasn't changed.
26+
*/
27+
record FileState(long size, long lastUpdate) {
28+
public static FileState of(Path path) throws IOException {
29+
return new FileState(Files.size(path), Files.getLastModifiedTime(path).toMillis());
30+
}
31+
}

build-tools/build-infra/src/main/java/org/apache/lucene/gradle/plugins/spotless/GoogleJavaFormatPlugin.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,16 +67,17 @@ public void apply(Project project) {
6767
tasks.named("tidy", tidy -> tidy.dependsOn(applyTask));
6868
tasks.named("check", check -> check.dependsOn(checkTask));
6969

70+
var fileStates = project.getLayout().getBuildDirectory().file("gjf-file-states.json");
7071
for (var t : List.of(applyTask, checkTask)) {
7172
t.configure(
7273
task -> {
7374
task.getBatchSize().set(batchSizeOption);
7475
task.dependsOn(":" + CheckEnvironmentPlugin.TASK_CHECK_JDK_INTERNALS_EXPOSED_TO_GRADLE);
76+
task.getFileStateCache().set(fileStates);
7577
});
7678
}
7779

7880
// Configure details depending on the project.
79-
8081
for (var t : List.of(applyTask, checkTask)) {
8182
t.configure(
8283
task -> {

build-tools/build-infra/src/main/java/org/apache/lucene/gradle/plugins/spotless/ParentGoogleJavaFormatTask.java

Lines changed: 88 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,18 @@
2222
import com.google.googlejavaformat.java.ImportOrderer;
2323
import com.google.googlejavaformat.java.JavaFormatterOptions;
2424
import com.google.googlejavaformat.java.RemoveUnusedImports;
25+
import groovy.json.JsonOutput;
26+
import groovy.json.JsonSlurper;
2527
import java.io.File;
28+
import java.io.IOException;
29+
import java.nio.file.Files;
30+
import java.nio.file.Path;
31+
import java.util.ArrayList;
2632
import java.util.List;
33+
import java.util.Map;
34+
import java.util.TreeMap;
35+
import java.util.function.Function;
36+
import java.util.stream.Collectors;
2737
import java.util.stream.StreamSupport;
2838
import javax.inject.Inject;
2939
import org.gradle.api.DefaultTask;
@@ -63,10 +73,18 @@ abstract class ParentGoogleJavaFormatTask extends DefaultTask {
6373
@Inject
6474
protected abstract WorkerExecutor getWorkerExecutor();
6575

76+
@Internal
77+
protected abstract RegularFileProperty getFileStateCache();
78+
79+
private Function<File, String> fileToKey;
80+
6681
public ParentGoogleJavaFormatTask(ProjectLayout layout, String gjfTask) {
6782
getOutputChangeListFile()
6883
.convention(layout.getBuildDirectory().file("gjf-" + gjfTask + ".txt"));
6984
getBatchSize().convention(1);
85+
86+
var projectPath = layout.getProjectDirectory().getAsFile().toPath();
87+
fileToKey = file -> projectPath.relativize(file.toPath()).toString();
7088
}
7189

7290
protected Iterable<List<File>> batchSourceFiles(List<File> sourceFiles) {
@@ -120,10 +138,78 @@ protected static String applyFormatter(Formatter formatter, String input)
120138
return input;
121139
}
122140

141+
protected void writeFileStates(
142+
RegularFileProperty pathProvider, TreeMap<String, FileState> fileStates) throws IOException {
143+
if (pathProvider.isPresent()) {
144+
Files.writeString(
145+
pathProvider.get().getAsFile().toPath(),
146+
JsonOutput.prettyPrint(JsonOutput.toJson(fileStates)));
147+
}
148+
}
149+
150+
protected final TreeMap<String, FileState> readFileStatesFrom(RegularFileProperty pathProvider) {
151+
var checksums = new TreeMap<String, FileState>();
152+
if (pathProvider.isPresent()) {
153+
Path path = pathProvider.get().getAsFile().toPath();
154+
if (Files.exists(path)) {
155+
try {
156+
@SuppressWarnings("unchecked")
157+
var saved = (Map<String, Map<String, Object>>) new JsonSlurper().parse(path);
158+
var restored =
159+
saved.entrySet().stream()
160+
.collect(
161+
Collectors.toMap(
162+
Map.Entry::getKey,
163+
e ->
164+
new FileState(
165+
((Number) e.getValue().get("size")).longValue(),
166+
((Number) e.getValue().get("lastUpdate")).longValue())));
167+
checksums.putAll(restored);
168+
} catch (Exception _) {
169+
getLogger().warn("Could not deserialize changes file: {}, ignoring it.", path);
170+
}
171+
}
172+
}
173+
174+
return checksums;
175+
}
176+
177+
protected final List<File> maybeRefilter(
178+
TreeMap<String, FileState> fileStates, List<File> sourceFiles) throws IOException {
179+
if (!fileStates.isEmpty()) {
180+
List<File> filteredSourceFiles = new ArrayList<>(sourceFiles);
181+
for (var it = filteredSourceFiles.iterator(); it.hasNext(); ) {
182+
var file = it.next();
183+
var p = file.toPath();
184+
var state = fileStates.get(fileToKey.apply(file));
185+
if (state != null && FileState.of(p).equals(state)) {
186+
it.remove();
187+
}
188+
}
189+
190+
if (sourceFiles.size() != filteredSourceFiles.size()) {
191+
getLogger()
192+
.info(
193+
"Reduced the number of files to check from {} to {} based on stored file states.",
194+
sourceFiles.size(),
195+
filteredSourceFiles.size());
196+
}
197+
198+
sourceFiles = filteredSourceFiles;
199+
}
200+
return sourceFiles;
201+
}
202+
203+
protected final void updateFileStates(
204+
TreeMap<String, FileState> fileStates, List<File> sourceFiles) throws IOException {
205+
for (var file : sourceFiles) {
206+
fileStates.put(fileToKey.apply(file), FileState.of(file.toPath()));
207+
}
208+
}
209+
123210
@Internal
124211
protected WorkQueue getWorkQueue() {
125-
// TODO: maybe fork a separate jvm so that we can pass open-module settings there and fine-tune
126-
// the jvm for the task?
212+
// Keep workers within the same JVM.
127213
return getWorkerExecutor().noIsolation();
128214
}
129215
}

0 commit comments

Comments
 (0)