Skip to content

Commit 9182725

Browse files
committed
Stop deeply nested calls from taking exponential time to format
Since palantir#1535 (2.89.0), the levels above the innermost eight first try to break only their last inner level and fall back to breaking normally. That attempt can lay out all of the inner level before it fails, and in deeply nested code it nearly always fails once the closing parentheses no longer fit in the line, so every level above repeats it for each layout it tries. Nine nested Map.ofEntries(Map.entry(...)) took 69.5 s to format, and the file from palantir#1632 took 34 s with the jar and 60 s with the native binary (#35). A failed attempt leaves nothing behind but the failure, so a level now remembers the states it failed from and answers at once when asked again. The key holds what the attempt reads: its arguments and the state's column, indents, pending break and taken break tags. The new test formats the nested calls under a 10 s limit, which the old code exceeds. They now take 0.45 s, and that file 1.05 s with the jar and 0.73 s with the native binary. The layout does not change: the output for both inputs and for the 15,747 files of the JDK 21 sources is byte for byte the same, and formatting those sources takes no longer.
1 parent 894f4a5 commit 9182725

3 files changed

Lines changed: 62 additions & 1 deletion

File tree

‎open-java-format/src/main/java/com/palantir/javaformat/doc/Level.java‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,11 @@
4141
import java.lang.annotation.RetentionPolicy;
4242
import java.lang.annotation.Target;
4343
import java.util.ArrayList;
44+
import java.util.HashSet;
4445
import java.util.List;
4546
import java.util.Optional;
4647
import java.util.OptionalInt;
48+
import java.util.Set;
4749
import java.util.stream.Collector;
4850
import java.util.stream.Collectors;
4951
import java.util.stream.Stream;
@@ -67,6 +69,13 @@ public final class Level extends Doc {
6769
@SuppressWarnings("Immutable") // Effectively immutable
6870
private final ImmutableSupplier<Integer> memoizedMaxDepth = Suppliers.memoize(() -> computeMaxDepth(docs))::get;
6971

72+
/**
73+
* Where {@link #tryBreakLastLevel} has already failed. An attempt can lay out all of the last inner level before it
74+
* fails, and each level above repeats it for every layout it tries, so without this deeply nested calls took time
75+
* exponential in their depth to format.
76+
*/
77+
private final Set<LastLevelAttempt> failedLastLevelAttempts = new HashSet<>();
78+
7079
/** The immutable characteristics of this level determined before the level contents are available. */
7180
private final OpenOp openOp;
7281

@@ -364,9 +373,21 @@ private Optional<State> tryBreakLastLevel(
364373
}
365374
Level innerLevel = ((Level) getLast(docs));
366375

367-
return tryBreakInnerLevel(commentsHelper, maxWidth, state, explorationNode, innerLevel, isSimpleInliningSoFar);
376+
LastLevelAttempt attempt = new LastLevelAttempt(maxWidth, isSimpleInliningSoFar, state.layoutInputs());
377+
if (failedLastLevelAttempts.contains(attempt)) {
378+
return Optional.empty();
379+
}
380+
Optional<State> result =
381+
tryBreakInnerLevel(commentsHelper, maxWidth, state, explorationNode, innerLevel, isSimpleInliningSoFar);
382+
if (result.isEmpty()) {
383+
failedLastLevelAttempts.add(attempt);
384+
}
385+
return result;
368386
}
369387

388+
/** A call of {@link #tryBreakLastLevel}, by everything besides this level that decides its outcome. */
389+
private record LastLevelAttempt(int maxWidth, boolean isSimpleInliningSoFar, State.LayoutInputs inputs) {}
390+
370391
@SuppressWarnings("for-rollout:NullAway")
371392
private Optional<State> tryInlineSuffix(
372393
CommentsHelper commentsHelper,

‎open-java-format/src/main/java/com/palantir/javaformat/doc/State.java‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import java.lang.annotation.Retention;
2727
import java.lang.annotation.RetentionPolicy;
2828
import java.lang.annotation.Target;
29+
import java.util.Objects;
2930
import org.immutables.value.Value;
3031
import org.immutables.value.Value.Parameter;
3132

@@ -221,6 +222,27 @@ State withTokState(Comment comment, TokState tokState) {
221222
.build();
222223
}
223224

225+
/**
226+
* The part of this state that laying out a level from here reads. It leaves out the line count, which is only ever
227+
* compared with another count from the same starting point; the branching coefficient, which nothing reads; and the
228+
* states of breaks, levels and comments, since a layout only reads those of the docs it has laid out itself.
229+
*/
230+
LayoutInputs layoutInputs() {
231+
return new LayoutInputs(lastIndent(), indent(), column(), mustBreak(), breakTagsTaken());
232+
}
233+
234+
/** See {@link #layoutInputs()}. */
235+
record LayoutInputs(int lastIndent, int indent, int column, boolean mustBreak, Set<BreakTag> breakTagsTaken) {
236+
/**
237+
* Leaves out the taken break tags: hashing them on every lookup costs more than comparing them on the lookups
238+
* that match everything else. Equality still compares them.
239+
*/
240+
@Override
241+
public int hashCode() {
242+
return Objects.hash(lastIndent, indent, column, mustBreak);
243+
}
244+
}
245+
224246
public static class Builder extends ImmutableState.Builder {}
225247

226248
public static Builder builder() {

‎open-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import static com.palantir.javaformat.java.JavaFormatterOptions.Style;
2020
import static org.assertj.core.api.Assertions.assertThatCode;
2121
import static org.assertj.core.api.Assertions.assertThatThrownBy;
22+
import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively;
2223

2324
import com.google.common.base.Joiner;
2425
import com.google.common.collect.Range;
@@ -32,6 +33,7 @@
3233
import java.nio.charset.StandardCharsets;
3334
import java.nio.file.Files;
3435
import java.nio.file.Path;
36+
import java.time.Duration;
3537
import java.util.List;
3638
import org.junit.jupiter.api.Test;
3739
import org.junit.jupiter.api.io.TempDir;
@@ -473,4 +475,20 @@ void producesFormattingChangesOnAlreadyFormattedFiles() throws FormatterExceptio
473475
formattedClass, List.of(Range.closedOpen(0, formattedClass.length()))))
474476
.isNotEmpty();
475477
}
478+
479+
@Test
480+
void formatsDeeplyNestedCallsQuickly() {
481+
// Nine nested Map.ofEntries(Map.entry(...)) are one more than fit in 120 columns with each argument kept on its
482+
// call's line. Every such attempt then failed, but only after laying out everything inside it, and each level
483+
// above repeated it for every layout it tried: formatting took time exponential in the depth.
484+
String call = "Map.ofEntries(Map.entry(\"key0\", \"value0\"), Map.entry(\"key1\", \"value1\"))";
485+
for (int depth = 0; depth < 9; depth++) {
486+
call = "Map.ofEntries(Map.entry(\"key" + depth + "\", " + call + "))";
487+
}
488+
String input = "class DeepNesting {\n Object foo() {\n return " + call + ";\n }\n}\n";
489+
Formatter formatter = Formatter.createFormatter(
490+
JavaFormatterOptions.builder().style(Style.OJF).build());
491+
492+
assertTimeoutPreemptively(Duration.ofSeconds(10), () -> formatter.formatSource(input));
493+
}
476494
}

0 commit comments

Comments
 (0)