Skip to content

Commit 32f3b92

Browse files
crogozcrogoz
authored
[Performance Enhancements] Lambda expressions format change & Multi-level exploration logic changes (#1535)
Improve perf by inspecting both breaking & not-breaking for the `LAST_LEVELS_TO_EXPLORE` levels
1 parent 30cf358 commit 32f3b92

12 files changed

Lines changed: 421 additions & 34 deletions

File tree

‎palantir-java-format-benchmarks/build.gradle‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,13 @@ tasks.named('jmh', JMHTask.class) {
3131
environment.put('JARS_CLASSPATH', configurations.named('formatterNativeJars').map { conf -> conf.collect { it.absolutePath }.join(":") })
3232
}
3333

34+
spotless {
35+
java {
36+
// symlinks to a test input file that shouldn't be formatted
37+
targetExclude 'src/test/resources/*'
38+
}
39+
}
40+
3441
jmhCompileGeneratedClasses {
3542
options.errorprone.enabled = false
3643
}

‎palantir-java-format-benchmarks/src/jmh/java/com/palantir/javaformat/BenchmarkMultiFiles.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,23 @@ private static List<String> getFilesToFormat() {
6262
}
6363
}
6464

65+
@State(Scope.Benchmark)
66+
public static class MultiLevelStates {
67+
68+
final String fileToFormat = getFileToFormat();
69+
70+
private static String getFileToFormat() {
71+
try {
72+
return Paths.get(".")
73+
.resolve("src/test/resources/PalantirDeeplyNestedCalls.java")
74+
.toAbsolutePath()
75+
.toString();
76+
} catch (Exception e) {
77+
throw new RuntimeException("Error reading resource ", e);
78+
}
79+
}
80+
}
81+
6582
@Benchmark
6683
@BenchmarkMode(Mode.All)
6784
@OutputTimeUnit(TimeUnit.SECONDS)
@@ -76,6 +93,17 @@ public final void runNativeImage(BenchmarkState state) throws InterruptedExcepti
7693
assertThat(process.waitFor()).isEqualTo(0);
7794
}
7895

96+
@Benchmark
97+
@BenchmarkMode(Mode.AverageTime)
98+
@OutputTimeUnit(TimeUnit.SECONDS)
99+
public final void runNestedLevel(MultiLevelStates state) throws InterruptedException, IOException {
100+
ProcessBuilder p = new ProcessBuilder();
101+
p.command(List.of(
102+
Path.of(System.getenv("NATIVE_IMAGE_CLASSPATH")).toString(), "-i", "--palantir", state.fileToFormat));
103+
Process process = p.inheritIO().start();
104+
assertThat(process.waitFor()).isEqualTo(0);
105+
}
106+
79107
@Benchmark
80108
@BenchmarkMode(Mode.All)
81109
@OutputTimeUnit(TimeUnit.SECONDS)

‎palantir-java-format-benchmarks/src/test/resources/PalantirDeeplyNestedCalls.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
../../../../palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/palantir-deeply-nested-calls.input

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

Lines changed: 66 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,9 @@
5252
/** A {@code Level} inside a {@link Doc}. */
5353
public final class Level extends Doc {
5454
/**
55-
* How many branches we are allowed to take (i.e how many times we can consider breaking vs not breaking the current
56-
* level) before we stop branching and always break, which is the google-java-format default behaviour.
55+
* The depth of nested levels in the current level tree from which we explore both breaking vs not breaking the current level.
5756
*/
58-
private static final int MAX_BRANCHING_COEFFICIENT = 20;
57+
private static final int LAST_LEVELS_TO_EXPLORE = 8;
5958

6059
private static final Collector<Level, ?, Optional<Level>> GET_LAST_COLLECTOR = Collectors.reducing((u, v) -> v);
6160

@@ -64,6 +63,10 @@ public final class Level extends Doc {
6463
@SuppressWarnings("Immutable") // Effectively immutable
6564
private final ImmutableSupplier<SplitsBreaks> memoizedSplitsBreaks =
6665
Suppliers.memoize(() -> splitByBreaks(docs))::get;
66+
67+
@SuppressWarnings("Immutable") // Effectively immutable
68+
private final ImmutableSupplier<Integer> memoizedMaxDepth = Suppliers.memoize(() -> computeMaxDepth(docs))::get;
69+
6770
/** The immutable characteristics of this level determined before the level contents are available. */
6871
private final OpenOp openOp;
6972

@@ -130,7 +133,7 @@ public State computeBreaks(CommentsHelper commentsHelper, int maxWidth, State st
130133
* into account the level's {@link #getColumnLimitBeforeLastBreak()}.
131134
*
132135
* @return the width after fitting it onto one line, if it was possible. This is guaranteed to be less than
133-
* {@code maxWidth}
136+
* {@code maxWidth}
134137
*/
135138
private Optional<Integer> tryToFitOnOneLine(int maxWidth, State state, Iterable<Doc> docs) {
136139
int column = state.column();
@@ -189,16 +192,15 @@ public State breakThisLevel() {
189192

190193
@Override
191194
public State preferBreakingLastInnerLevel(boolean _keepIndentWhenInlined) {
192-
// Try both breaking and not breaking. Choose the better one based on LOC, preferring
193-
// breaks if the outcome is the same.
194-
State state = this.state.withNewBranch();
195-
196-
Obs.Exploration broken = breakNormally(state);
197-
198-
if (state.branchingCoefficient() < MAX_BRANCHING_COEFFICIENT) {
195+
int maxDepth = getMaxDepth();
196+
// Explore both breaking and not breaking if we're in the last LAST_LEVELS_TO_EXPLORE levels. Choose the
197+
// better one based on LOC, preferring breaks if the outcome is the same.
198+
if (maxDepth <= LAST_LEVELS_TO_EXPLORE) {
199+
State state = this.state.withNewBranch();
200+
Obs.Exploration broken = breakNormally(state);
199201
State state1 = state.withNoIndent();
200202
Optional<Obs.Exploration> lastLevelBroken = levelNode.maybeExplore(
201-
"tryBreakLastLevel",
203+
"tryBreakLastLevel" + maxDepth,
202204
state1,
203205
explorationNode -> tryBreakLastLevel(commentsHelper, maxWidth, state1, explorationNode, true));
204206

@@ -208,8 +210,20 @@ public State preferBreakingLastInnerLevel(boolean _keepIndentWhenInlined) {
208210
return lastLevelBroken.get().markAccepted();
209211
}
210212
}
213+
return broken.markAccepted();
214+
} else {
215+
State state1 = state.withNoIndent();
216+
Optional<Obs.Exploration> lastLevelBroken = levelNode.maybeExplore(
217+
"tryBreakLastLevel only" + maxDepth,
218+
state1,
219+
explorationNode -> tryBreakLastLevel(commentsHelper, maxWidth, state1, explorationNode, true));
220+
221+
if (lastLevelBroken.isPresent()) {
222+
return lastLevelBroken.get().markAccepted();
223+
}
224+
// falling back to breaking normally
211225
}
212-
return broken.markAccepted();
226+
return breakNormally(state).markAccepted();
213227
}
214228

215229
@Override
@@ -242,7 +256,7 @@ public State inlineSuffix() {
242256
private Exploration breakNormally(State state, LevelNode levelNode, CommentsHelper commentsHelper, int maxWidth) {
243257
State stateForBroken = state.withIndentIncrementedBy(getPlusIndent());
244258
return levelNode.explore(
245-
"breaking normally",
259+
"breaking normally " + getMaxDepth(),
246260
stateForBroken,
247261
explorationNode -> computeBroken(commentsHelper, maxWidth, stateForBroken, explorationNode));
248262
}
@@ -258,7 +272,7 @@ private Exploration breakNormally(State state, LevelNode levelNode, CommentsHelp
258272
* </ul>
259273
*
260274
* @param brokenState is expected to be the state resulting from {@link Level#breakNormally}, but is passed in
261-
* for efficiency reasons
275+
* for efficiency reasons
262276
*/
263277
private Optional<State> handle_breakOnlyIfInnerLevelsThenFitOnOneLine(
264278
CommentsHelper commentsHelper,
@@ -540,17 +554,9 @@ private static Optional<State> tryBreakInnerLevel_checkInner(
540554
keepIndentWhenInlined,
541555
explorationNode);
542556
case ACCEPT_INLINE_CHAIN_IF_SIMPLE_OTHERWISE_CHECK_INNER:
543-
// We will allow inlining this kind of level but only if the level itself is
544-
// not simple.
545-
if (lastLevel2.openOp.complexity() == Complexity.SIMPLE) {
546-
return Optional.empty();
547-
}
548-
return innerLevel.tryInlinePrefixOntoCurrentLine(
549-
commentsHelper,
550-
maxWidth,
551-
state1,
552-
keepIndentWhenInlined,
553-
explorationNode);
557+
// specific to lambda body expressions - falls back to `breakNormally` in
558+
// `preferBreakingLastInnerLevel`
559+
return Optional.empty();
554560
default:
555561
throw new RuntimeException("Unknown breakabilityIfLastLevel: " + lastLevel2);
556562
}
@@ -736,6 +742,39 @@ public String representation(State state) {
736742
return new LevelDelimitedFlatValueDocVisitor(state).visit(this);
737743
}
738744

745+
/**
746+
* Get the approximation of the maximum depth of nested levels in this level tree (memoized).
747+
*/
748+
private int getMaxDepth() {
749+
return memoizedMaxDepth.get();
750+
}
751+
752+
/**
753+
* Calculates the maximum depth of nested levels in this level tree.
754+
* This computation is expensive, so it's memoized via {@link #getMaxDepth()}.
755+
*/
756+
private int computeMaxDepth(Iterable<Doc> docs) {
757+
int maxChildDepth = 0;
758+
for (Doc doc : docs) {
759+
if (doc instanceof Level) {
760+
Level childLevel = (Level) doc;
761+
maxChildDepth = Math.max(maxChildDepth, childLevel.getMaxDepth());
762+
}
763+
}
764+
765+
// We use the presence of breaks as a proxy for meaningful structural nesting.
766+
boolean hasBreaks = false;
767+
for (Doc doc : docs) {
768+
if (doc instanceof Break) {
769+
hasBreaks = true;
770+
break;
771+
}
772+
}
773+
774+
// Only count levels that have breaks (which can actually be formatted across multiple lines)
775+
return hasBreaks ? 1 + maxChildDepth : maxChildDepth;
776+
}
777+
739778
/**
740779
* Get the width of a sequence of {@link Doc}s.
741780
*
@@ -774,6 +813,7 @@ public String toString() {
774813
interface SplitsBreaks {
775814
/** Groups of {@link Doc}s that are children of the current {@link Level}, separated by {@link Break}s. */
776815
ImmutableList<ImmutableList<Doc>> splits();
816+
777817
/** {@link Break}s between {@link Doc}s in the current {@link Level}. */
778818
ImmutableList<Break> breaks();
779819
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,14 @@
3939
public abstract class State {
4040
/** Last indent that was actually taken. */
4141
public abstract int lastIndent();
42+
4243
/** Next indent, if the level is about to be broken. */
4344
public abstract int indent();
4445

4546
public abstract int column();
4647

4748
public abstract boolean mustBreak();
49+
4850
/** Counts how many lines a particular formatting took. */
4951
public abstract int numLines();
5052

‎palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B20128760.output‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,11 @@ class B20128760 {
7070
Xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx {}
7171

7272
{
73-
Stream<ItemKey> itemIdsStream = stream(members).flatMap(m -> m.getFieldValues().entrySet().stream()
74-
.filter(fv -> itemLinkFieldIds.contains(fv.getKey()))
75-
.flatMap(fv -> FieldDTO.deserializeStringToListOfStrings(fv.getValue()).stream()
76-
.map(id -> new ItemKey(fieldsById.get(fv.getKey()).getItemTypeId(), id))));
73+
Stream<ItemKey> itemIdsStream = stream(members)
74+
.flatMap(m -> m.getFieldValues().entrySet().stream()
75+
.filter(fv -> itemLinkFieldIds.contains(fv.getKey()))
76+
.flatMap(fv -> FieldDTO.deserializeStringToListOfStrings(fv.getValue()).stream()
77+
.map(id ->
78+
new ItemKey(fieldsById.get(fv.getKey()).getItemTypeId(), id))));
7779
}
7880
}

‎palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/palantir-chains-lambdas.input‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,5 +5,11 @@ public class Foo {
55
.baz(argargargargargargargargargargargargargargargargargargargargarg, param -> doStuff()
66
.method1()
77
.method2(arljhfaldjfhalfjhalfjahflahflahfaljfhadlfjhljsahljhfsaljfhlfajhfljh));
8+
9+
thisIsMyObject
10+
.callingMethod1()
11+
.callingMethod2(argargargargargargargargargargargargargargargargargargargargarg, param -> doStuff()
12+
.method1()
13+
.method2(arljhfaldjfhalfjhalfjahflahflahfaljfhadlfjhljsahljhfsaljfhlfajhfljh));
814
}
915
}

‎palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/palantir-chains-lambdas.output‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,18 @@
11
public class Foo {
22
public static void main(String[] args) {
3-
foo.bar().baz(argargargargargargargargargargargargargargargargargargargargarg, param -> doStuff()
4-
.method1()
5-
.method2(arljhfaldjfhalfjhalfjahflahflahfaljfhadlfjhljsahljhfsaljfhlfajhfljh));
3+
foo.bar()
4+
.baz(
5+
argargargargargargargargargargargargargargargargargargargargarg,
6+
param -> doStuff()
7+
.method1()
8+
.method2(arljhfaldjfhalfjhalfjahflahflahfaljfhadlfjhljsahljhfsaljfhlfajhfljh));
9+
10+
thisIsMyObject
11+
.callingMethod1()
12+
.callingMethod2(
13+
argargargargargargargargargargargargargargargargargargargargarg,
14+
param -> doStuff()
15+
.method1()
16+
.method2(arljhfaldjfhalfjhalfjahflahflahfaljfhadlfjhljsahljhfsaljfhlfajhfljh));
617
}
718
}

‎palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/palantir-deeply-nested-calls.input‎

Lines changed: 79 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,4 +12,82 @@ class PalantirDeeplyNestedCalls {
1212
.build()))
1313
.build()))
1414
.build();
15-
}
15+
16+
void testMethod() {
17+
assertThat(ConfigServiceV1.getAllItems(item1, item2, item3, item4))
18+
.containsExactlyInAnyOrderElementsOf(Set.of(
19+
new ItemWithCustomLocator(
20+
ItemConfigV1.DEFAULT_CONFIG.toCustomString(),
21+
ItemConfigV1.createCustomConfig(ItemConfigV1Locator.of(TEST_ITEM_A))),
22+
new ItemWithCustomLocator(
23+
ItemConfigV1.builder()
24+
.id(getId(TEST_ITEM_A))
25+
.name(ItemName.of(TEST_API_NAME.get()))
26+
.apiName(TEST_API_NAME)
27+
.spec(ItemConfigV1Spec.native_(NativeItemConfigV1Spec.model(TestModel.of())))
28+
.category(TestCatalog.TEST_CATEGORY_ALPHA)
29+
.creator(TestCatalog.TEST_CREATOR_BETA)
30+
.status(ItemLifecycleStatus.EXPERIMENTAL)
31+
.statusV2(ItemLifecycleStatusV2.experimental(LifecycleStatusExperimental.of()))
32+
.inputSpec(ItemInputSpec.unknownInputType(UnknownInputSpec.of()))
33+
.creationDate(ItemMetadataInfo.builder().build())
34+
.performanceAttributes(
35+
ItemAttributes.builder().build())
36+
.categories(TestCatalog.TEST_CATEGORY_ALPHA)
37+
.build(),
38+
CustomItemLocator.hosted(HostedItemLocator.of(TEST_ITEM_B))),
39+
new ItemWithCustomLocator(
40+
ItemConfigV1.builder()
41+
.id(getId(TestModelCatalog.MODEL_XYZ_123.apiName()))
42+
.name(ItemName.of(TestModelCatalog.MODEL_XYZ_123
43+
.apiName()
44+
.get()))
45+
.apiName(TestModelCatalog.MODEL_XYZ_123.apiName())
46+
.spec(ItemConfigV1Spec.native_(
47+
NativeItemConfigV1Spec.hubModel(HubModelSpec.of())))
48+
.category(TestCatalog.TEST_CATEGORY_ALPHA)
49+
.creator(TestCatalog.TEST_CREATOR_BETA)
50+
.status(ItemLifecycleStatus.EXPERIMENTAL)
51+
.statusV2(ItemLifecycleStatusV2.experimental(LifecycleStatusExperimental.of()))
52+
.inputSpec(ItemInputSpec.unknownInputType(UnknownInputSpec.of()))
53+
.creationDate(ItemMetadataInfo.builder().build())
54+
.performanceAttributes(
55+
ItemAttributes.builder().build())
56+
.categories(TestCatalog.TEST_CATEGORY_ALPHA)
57+
.build(),
58+
CustomItemLocator.hosted(HostedItemLocator.of(TEST_ITEM_C))),
59+
// Discovered items
60+
new ItemWithCustomLocator(
61+
TestModelCatalog.MODEL_ABC_456.toItemApi(),
62+
CustomItemLocator.discovered(DiscoveredItemLocator.of())),
63+
new ItemWithCustomLocator(
64+
ItemConfigV1.builder()
65+
.id(getId(TEST_API_NAME))
66+
.name(ItemName.of(TEST_API_NAME.get()))
67+
.apiName(TEST_API_NAME)
68+
.spec(ItemConfigV1Spec.native_(
69+
NativeItemConfigV1Spec.hubModel(HubModelSpec.of())))
70+
.category(TestCatalog.TEST_CATEGORY_ALPHA)
71+
.creator(TestCatalog.TEST_CREATOR_BETA)
72+
.status(ItemLifecycleStatus.EXPERIMENTAL)
73+
.statusV2(ItemLifecycleStatusV2.experimental(LifecycleStatusExperimental.of()))
74+
.inputSpec(ItemInputSpec.unknownInputType(UnknownInputSpec.of()))
75+
.creationDate(ItemMetadataInfo.builder().build())
76+
.performanceAttributes(
77+
ItemAttributes.builder().build())
78+
.categories(TestCatalog.TEST_CATEGORY_ALPHA)
79+
.build(),
80+
CustomItemLocator.discovered(DiscoveredItemLocator.of()))));
81+
}
82+
83+
public Map<FooBarRid, BazQuxMetrics> otherMethod(
84+
FooAuthHeader authHeader, Set<FooBarRid> thingRids, WidgetTimeRange timeRange) {
85+
return myCustomService
86+
.get()
87+
.fetchData(authHeader, FooMyCustomClass.of(thingRids, timeRange))
88+
.entrySet()
89+
.stream()
90+
.collect(Collectors.toMap(
91+
Map.Entry::getKey, entry -> entry.getValue().getMetrics()));
92+
}
93+
}

0 commit comments

Comments
 (0)