-
Notifications
You must be signed in to change notification settings - Fork 49
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
Improvement: Add instrumentation to allow warning on files that take long to format #989
base: develop
Are you sure you want to change the base?
Changes from 1 commit
620e22c
146ca0f
480a2e8
0b8928d
8e91a7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,7 @@ final class CommandLineOptions { | |
private final Optional<String> assumeFilename; | ||
private final boolean reflowLongStrings; | ||
private final boolean outputReplacements; | ||
private final Optional<Long> warnOnExpensiveFileDurationMillis; | ||
|
||
CommandLineOptions( | ||
ImmutableList<String> files, | ||
|
@@ -65,7 +66,8 @@ final class CommandLineOptions { | |
boolean setExitIfChanged, | ||
Optional<String> assumeFilename, | ||
boolean reflowLongStrings, | ||
boolean outputReplacements) { | ||
boolean outputReplacements, | ||
Optional<Long> warnOnExpensiveFileDurationMillis) { | ||
this.files = files; | ||
this.inPlace = inPlace; | ||
this.lines = lines; | ||
|
@@ -85,6 +87,7 @@ final class CommandLineOptions { | |
this.assumeFilename = assumeFilename; | ||
this.reflowLongStrings = reflowLongStrings; | ||
this.outputReplacements = outputReplacements; | ||
this.warnOnExpensiveFileDurationMillis = warnOnExpensiveFileDurationMillis; | ||
} | ||
|
||
/** The files to format. */ | ||
|
@@ -185,6 +188,11 @@ boolean outputReplacements() { | |
return outputReplacements; | ||
} | ||
|
||
/** Returns the number of millis to allow processing of a single file to take before warning. */ | ||
Optional<Long> warnOnExpensiveFileDurationMillis() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be |
||
return warnOnExpensiveFileDurationMillis; | ||
} | ||
|
||
static Builder builder() { | ||
return new Builder(); | ||
} | ||
|
@@ -210,6 +218,7 @@ static final class Builder { | |
private Optional<String> assumeFilename = Optional.empty(); | ||
private boolean reflowLongStrings = true; | ||
private boolean outputReplacements = false; | ||
private Optional<Long> warnOnExpensiveFileDurationMillis = Optional.empty(); | ||
|
||
private Builder() {} | ||
|
||
|
@@ -305,6 +314,11 @@ Builder outputReplacements(boolean outputReplacements) { | |
return this; | ||
} | ||
|
||
Builder warnOnExpensiveFileDurationMillis(long warnOnExpensiveFileDurationMillis) { | ||
this.warnOnExpensiveFileDurationMillis = Optional.of(warnOnExpensiveFileDurationMillis); | ||
return this; | ||
} | ||
|
||
CommandLineOptions build() { | ||
Preconditions.checkArgument(!aosp || !palantirStyle, "Cannot use both aosp and palantir style"); | ||
return new CommandLineOptions( | ||
|
@@ -326,7 +340,8 @@ CommandLineOptions build() { | |
setExitIfChanged, | ||
assumeFilename, | ||
reflowLongStrings, | ||
outputReplacements); | ||
outputReplacements, | ||
warnOnExpensiveFileDurationMillis); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
package com.palantir.javaformat.java; | ||
|
||
import com.palantir.javaformat.java.InstrumentedFormatFileCallable.FormatFileResult; | ||
import java.time.Duration; | ||
import java.util.concurrent.Callable; | ||
import org.immutables.value.Value.Immutable; | ||
|
||
/** | ||
* Instrumentation on top of {@link FormatFileCallable} to track the time spent formatting a given file, and produce | ||
* warnings | ||
*/ | ||
public class InstrumentedFormatFileCallable implements Callable<FormatFileResult> { | ||
|
||
private final FormatFileCallable delegate; | ||
|
||
public InstrumentedFormatFileCallable(FormatFileCallable delegate) { | ||
this.delegate = delegate; | ||
} | ||
|
||
@Override | ||
public FormatFileResult call() throws Exception { | ||
long start = System.currentTimeMillis(); | ||
String result = delegate.call(); | ||
Duration duration = Duration.ofMillis(System.currentTimeMillis() - start); | ||
Comment on lines
+37
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you have Guava on the classpath already, use |
||
return FormatFileResult.of(result, duration); | ||
} | ||
|
||
@Immutable | ||
public interface FormatFileResult { | ||
String result(); | ||
|
||
Duration duration(); | ||
|
||
static FormatFileResult of(String result, Duration duration) { | ||
return ImmutableFormatFileResult.builder() | ||
.result(result) | ||
.duration(duration) | ||
.build(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ | |
import static java.nio.charset.StandardCharsets.UTF_8; | ||
|
||
import com.google.common.io.ByteStreams; | ||
import com.palantir.javaformat.java.InstrumentedFormatFileCallable.FormatFileResult; | ||
import com.palantir.javaformat.java.JavaFormatterOptions.Style; | ||
import java.io.IOException; | ||
import java.io.InputStream; | ||
|
@@ -110,7 +111,7 @@ private int formatFiles(CommandLineOptions parameters, JavaFormatterOptions opti | |
ExecutorService executorService = Executors.newFixedThreadPool(numThreads); | ||
|
||
Map<Path, String> inputs = new LinkedHashMap<>(); | ||
Map<Path, Future<String>> results = new LinkedHashMap<>(); | ||
Map<Path, Future<FormatFileResult>> results = new LinkedHashMap<>(); | ||
boolean allOk = true; | ||
|
||
for (String fileName : parameters.files()) { | ||
|
@@ -123,18 +124,23 @@ private int formatFiles(CommandLineOptions parameters, JavaFormatterOptions opti | |
try { | ||
input = new String(Files.readAllBytes(path), UTF_8); | ||
inputs.put(path, input); | ||
results.put(path, executorService.submit(new FormatFileCallable(parameters, input, options))); | ||
results.put( | ||
path, | ||
executorService.submit(new InstrumentedFormatFileCallable( | ||
new FormatFileCallable(parameters, input, options)))); | ||
} catch (IOException e) { | ||
errWriter.println(fileName + ": could not read file: " + e.getMessage()); | ||
allOk = false; | ||
} | ||
} | ||
|
||
for (Map.Entry<Path, Future<String>> result : results.entrySet()) { | ||
for (Map.Entry<Path, Future<FormatFileResult>> result : results.entrySet()) { | ||
Path path = result.getKey(); | ||
String formatted; | ||
try { | ||
formatted = result.getValue().get(); | ||
FormatFileResult fileResult = result.getValue().get(); | ||
formatted = fileResult.result(); | ||
warnIfDurationExceedsThreshold(parameters, path, fileResult); | ||
} catch (InterruptedException e) { | ||
errWriter.println(e.getMessage()); | ||
allOk = false; | ||
|
@@ -177,6 +183,16 @@ private int formatFiles(CommandLineOptions parameters, JavaFormatterOptions opti | |
return allOk ? 0 : 1; | ||
} | ||
|
||
private void warnIfDurationExceedsThreshold(CommandLineOptions parameters, Path path, FormatFileResult fileResult) { | ||
if (parameters.warnOnExpensiveFileDurationMillis().isPresent() | ||
&& parameters.warnOnExpensiveFileDurationMillis().get() | ||
> fileResult.duration().toMillis()) { | ||
errWriter.println(path + ": took " + fileResult.duration().toMillis() + "ms to format, " | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: put |
||
+ "which is longer than the threshold of " | ||
+ parameters.warnOnExpensiveFileDurationMillis().get() + "ms"); | ||
} | ||
} | ||
|
||
private int formatStdin(CommandLineOptions parameters, JavaFormatterOptions options) { | ||
String input; | ||
try { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably long term want to be able to set a
failIfDurationExceeded
option as well. Ideas for if this flag should be altered or if it's just another config?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just another config, imo