From ee5778fd4097121ea273bfe7b2ec685ecc4368bd Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Fri, 8 Nov 2024 15:48:35 -0500 Subject: [PATCH 1/4] Add StreamFlatMapOptional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If one has a `Stream> stream` of size `N` and does `stream.flatMap(Optional::stream)`, you’ll end up allocating `N` extra streams — one for each `Optional` input element. When `N` is large or on latency, throughput, and allocation sensitive code paths those allocations can cause extra GC cycles or pauses if allocation rate is high enough. `Stream.filter(Optional::isPresent).map(Optional::get)` is more efficient than `Stream.flatMap(Optional::stream)` as it does not allocate a new `Stream` for every element in the stream. --- .../errorprone/StreamFlatMapOptional.java | 79 +++++++++++++++++++ .../errorprone/StreamFlatmapOptionalTest.java | 51 ++++++++++++ .../BaselineErrorProneExtension.java | 1 + 3 files changed, 131 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamFlatMapOptional.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamFlatmapOptionalTest.java diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamFlatMapOptional.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamFlatMapOptional.java new file mode 100644 index 000000000..0585a1be5 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamFlatMapOptional.java @@ -0,0 +1,79 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * 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.palantir.baseline.errorprone; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import java.util.List; +import java.util.Optional; +import java.util.function.Function; +import java.util.stream.Stream; + +@AutoService(BugChecker.class) +@BugPattern( + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + severity = BugPattern.SeverityLevel.WARNING, + summary = "`Stream.filter(Optional::isPresent).map(Optional::get)` is more efficient than " + + "`Stream.flatMap(Optional::stream)`") +public final class StreamFlatMapOptional extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + + private static final Matcher STREAM_FLAT_MAP = MethodMatchers.instanceMethod() + .onDescendantOf(Stream.class.getName()) + .namedAnyOf("flatMap") + .withParameters(Function.class.getName()); + + private static final Matcher OPTIONAL_STREAM = MethodMatchers.instanceMethod() + .onDescendantOf(Optional.class.getName()) + .named("stream") + .withNoParameters(); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (STREAM_FLAT_MAP.matches(tree, state)) { + ExpressionTree stream = ASTHelpers.getReceiver(tree); + if (stream != null) { + ExpressionTree streamReceiver = ASTHelpers.getReceiver(stream); + if (streamReceiver != null) { + List arguments = tree.getArguments(); + if (arguments != null && arguments.size() == 1) { + if (OPTIONAL_STREAM.matches(arguments.get(0), state)) { + String replacement = state.getSourceForNode(ASTHelpers.getReceiver(tree.getMethodSelect())) + + ".filter(Optional::isPresent).map(Optional::get)"; + SuggestedFix fix = SuggestedFix.builder() + .addImport("java.util.Optional") + .replace(tree, replacement) + .build(); + return buildDescription(tree).addFix(fix).build(); + } + } + } + } + } + return Description.NO_MATCH; + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamFlatmapOptionalTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamFlatmapOptionalTest.java new file mode 100644 index 000000000..3f86466ed --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamFlatmapOptionalTest.java @@ -0,0 +1,51 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * 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.palantir.baseline.errorprone; + +import org.junit.jupiter.api.Test; + +class StreamFlatmapOptionalTest { + @Test + public void test() { + fix().addInputLines( + "Test.java", + "import java.util.Collection;", + "import java.util.List;", + "import java.util.Optional;", + "public class Test {", + " List f(List>> in) {", + " return in.stream().flatMap(Collection::stream).flatMap(Optional::stream).toList();", + " }", + "}") + .addOutputLines( + "Test.java", + "import java.util.Collection;", + "import java.util.List;", + "import java.util.Optional;", + "public class Test {", + " List f(List>> in) {", + " return in.stream().flatMap(Collection::stream)" + + ".filter(Optional::isPresent).map(Optional::get).toList();", + " }", + "}") + .doTest(); + } + + private RefactoringValidator fix() { + return RefactoringValidator.of(StreamFlatMapOptional.class, getClass()); + } +} diff --git a/gradle-baseline-java/src/main/java/com/palantir/baseline/extensions/BaselineErrorProneExtension.java b/gradle-baseline-java/src/main/java/com/palantir/baseline/extensions/BaselineErrorProneExtension.java index f911233b9..bf3ff82bb 100644 --- a/gradle-baseline-java/src/main/java/com/palantir/baseline/extensions/BaselineErrorProneExtension.java +++ b/gradle-baseline-java/src/main/java/com/palantir/baseline/extensions/BaselineErrorProneExtension.java @@ -72,6 +72,7 @@ public class BaselineErrorProneExtension { "Slf4jLogsafeArgs", "Slf4jThrowable", "StreamOfEmpty", + "StreamFlatMapOptional", "StrictUnusedVariable", "StringBuilderConstantParameters", "ThrowError", From 4c1c248a50a032a70f095c768f6e92573aff7223 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Fri, 8 Nov 2024 20:54:21 +0000 Subject: [PATCH 2/4] Add generated changelog entries --- changelog/@unreleased/pr-2946.v2.yml | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changelog/@unreleased/pr-2946.v2.yml diff --git a/changelog/@unreleased/pr-2946.v2.yml b/changelog/@unreleased/pr-2946.v2.yml new file mode 100644 index 000000000..78ae4166d --- /dev/null +++ b/changelog/@unreleased/pr-2946.v2.yml @@ -0,0 +1,8 @@ +type: improvement +improvement: + description: |- + If one has a `Stream> stream` of size `N` and does `stream.flatMap(Optional::stream)`, you’ll end up allocating `N` extra streams — one for each `Optional` input element. When `N` is large, those allocations can cause extra GC cycles and pauses if allocation rate is high enough leading to issues with latency, throughput, and allocation sensitive code paths. + + `Stream.filter(Optional::isPresent).map(Optional::get)` is more efficient than `Stream.flatMap(Optional::stream)` as it does not allocate a new `Stream` for every element in the stream. + links: + - https://github.com/palantir/gradle-baseline/pull/2946 From 296f87de6a17032ae06ed208aec1440254400e32 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Fri, 8 Nov 2024 16:58:49 -0500 Subject: [PATCH 3/4] Fix non-method-reference --- .../errorprone/StreamFlatMapOptional.java | 23 ++++++++----------- .../errorprone/StreamFlatmapOptionalTest.java | 19 +++++++++++---- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamFlatMapOptional.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamFlatMapOptional.java index 0585a1be5..8b2820af3 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamFlatMapOptional.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamFlatMapOptional.java @@ -57,19 +57,16 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState if (STREAM_FLAT_MAP.matches(tree, state)) { ExpressionTree stream = ASTHelpers.getReceiver(tree); if (stream != null) { - ExpressionTree streamReceiver = ASTHelpers.getReceiver(stream); - if (streamReceiver != null) { - List arguments = tree.getArguments(); - if (arguments != null && arguments.size() == 1) { - if (OPTIONAL_STREAM.matches(arguments.get(0), state)) { - String replacement = state.getSourceForNode(ASTHelpers.getReceiver(tree.getMethodSelect())) - + ".filter(Optional::isPresent).map(Optional::get)"; - SuggestedFix fix = SuggestedFix.builder() - .addImport("java.util.Optional") - .replace(tree, replacement) - .build(); - return buildDescription(tree).addFix(fix).build(); - } + List arguments = tree.getArguments(); + if (arguments != null && arguments.size() == 1) { + if (OPTIONAL_STREAM.matches(arguments.get(0), state)) { + String replacement = state.getSourceForNode(ASTHelpers.getReceiver(tree.getMethodSelect())) + + ".filter(Optional::isPresent).map(Optional::get)"; + SuggestedFix fix = SuggestedFix.builder() + .addImport("java.util.Optional") + .replace(tree, replacement) + .build(); + return buildDescription(tree).addFix(fix).build(); } } } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamFlatmapOptionalTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamFlatmapOptionalTest.java index 3f86466ed..a66626bc3 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamFlatmapOptionalTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamFlatmapOptionalTest.java @@ -26,9 +26,15 @@ public void test() { "import java.util.Collection;", "import java.util.List;", "import java.util.Optional;", + "import java.util.stream.Collectors;", "public class Test {", - " List f(List>> in) {", - " return in.stream().flatMap(Collection::stream).flatMap(Optional::stream).toList();", + " List f1(List>> in) {", + " return in.stream().flatMap(Collection::stream)" + + ".flatMap(Optional::stream).collect(Collectors.toList());", + " }", + " List f2(List>> in) {", + " return in.stream().flatMap(list -> list.stream().flatMap(Optional::stream))" + + ".collect(Collectors.toList());", " }", "}") .addOutputLines( @@ -36,10 +42,15 @@ public void test() { "import java.util.Collection;", "import java.util.List;", "import java.util.Optional;", + "import java.util.stream.Collectors;", "public class Test {", - " List f(List>> in) {", + " List f1(List>> in) {", " return in.stream().flatMap(Collection::stream)" - + ".filter(Optional::isPresent).map(Optional::get).toList();", + + ".filter(Optional::isPresent).map(Optional::get).collect(Collectors.toList());", + " }", + " List f2(List>> in) {", + " return in.stream().flatMap(list -> list.stream()" + + ".filter(Optional::isPresent).map(Optional::get)).collect(Collectors.toList());", " }", "}") .doTest(); From 68b779a0bc9e2caa2f5a293bad747119f21ce271 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Fri, 8 Nov 2024 20:45:21 -0500 Subject: [PATCH 4/4] Address PR comments to handle single pass multiple replacements --- .../errorprone/StreamFlatMapOptional.java | 35 +++++++++------- .../errorprone/StreamFlatmapOptionalTest.java | 42 ++++++++++++------- 2 files changed, 45 insertions(+), 32 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamFlatMapOptional.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamFlatMapOptional.java index 8b2820af3..7aa6231f4 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamFlatMapOptional.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamFlatMapOptional.java @@ -21,13 +21,15 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.ChildMultiMatcher.MatchType; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; import com.google.errorprone.matchers.method.MethodMatchers; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; -import java.util.List; import java.util.Optional; import java.util.function.Function; import java.util.stream.Stream; @@ -52,23 +54,24 @@ public final class StreamFlatMapOptional extends BugChecker implements BugChecke .named("stream") .withNoParameters(); + private static final Matcher STREAM_FLATMAP_OPTIONAL_STREAM = Matchers.methodInvocation( + STREAM_FLAT_MAP, + // Any of the three MatchTypes are reasonable in this case, given a single arg + MatchType.LAST, + OPTIONAL_STREAM); + @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (STREAM_FLAT_MAP.matches(tree, state)) { - ExpressionTree stream = ASTHelpers.getReceiver(tree); - if (stream != null) { - List arguments = tree.getArguments(); - if (arguments != null && arguments.size() == 1) { - if (OPTIONAL_STREAM.matches(arguments.get(0), state)) { - String replacement = state.getSourceForNode(ASTHelpers.getReceiver(tree.getMethodSelect())) - + ".filter(Optional::isPresent).map(Optional::get)"; - SuggestedFix fix = SuggestedFix.builder() - .addImport("java.util.Optional") - .replace(tree, replacement) - .build(); - return buildDescription(tree).addFix(fix).build(); - } - } + if (STREAM_FLATMAP_OPTIONAL_STREAM.matches(tree, state)) { + ExpressionTree receiver = ASTHelpers.getReceiver(tree.getMethodSelect()); + if (receiver != null) { + SuggestedFix.Builder fix = SuggestedFix.builder(); + String optionalType = SuggestedFixes.qualifyType(state, fix, Optional.class.getCanonicalName()); + String replacement = ".filter(" + optionalType + "::isPresent).map(" + optionalType + "::get)"; + return buildDescription(tree) + .addFix(fix.replace(state.getEndPosition(receiver), state.getEndPosition(tree), replacement) + .build()) + .build(); } } return Description.NO_MATCH; diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamFlatmapOptionalTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamFlatmapOptionalTest.java index a66626bc3..24fd9e05b 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamFlatmapOptionalTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StreamFlatmapOptionalTest.java @@ -24,33 +24,43 @@ public void test() { fix().addInputLines( "Test.java", "import java.util.Collection;", - "import java.util.List;", "import java.util.Optional;", - "import java.util.stream.Collectors;", + "import java.util.stream.Stream;", "public class Test {", - " List f1(List>> in) {", - " return in.stream().flatMap(Collection::stream)" - + ".flatMap(Optional::stream).collect(Collectors.toList());", + " Stream f1(Stream>> in) {", + " return in.flatMap(Collection::stream).flatMap(Optional::stream);", " }", - " List f2(List>> in) {", - " return in.stream().flatMap(list -> list.stream().flatMap(Optional::stream))" - + ".collect(Collectors.toList());", + " Stream f2(Stream>> in) {", + " return in.flatMap(list -> list.stream().flatMap(Optional::stream));", + " }", + " Stream f3(Stream>> in) {", + " return in.flatMap(list -> list.stream()).flatMap(Optional::stream);", + " }", + " Stream f4(Stream>> in) {", + " return in.flatMap(Optional::stream).flatMap(Optional::stream);", " }", "}") .addOutputLines( "Test.java", "import java.util.Collection;", - "import java.util.List;", "import java.util.Optional;", - "import java.util.stream.Collectors;", + "import java.util.stream.Stream;", "public class Test {", - " List f1(List>> in) {", - " return in.stream().flatMap(Collection::stream)" - + ".filter(Optional::isPresent).map(Optional::get).collect(Collectors.toList());", + " Stream f1(Stream>> in) {", + " return in.flatMap(Collection::stream)" + + ".filter(Optional::isPresent).map(Optional::get);", + " }", + " Stream f2(Stream>> in) {", + " return in.flatMap(list -> list.stream()" + + ".filter(Optional::isPresent).map(Optional::get));", + " }", + " Stream f3(Stream>> in) {", + " return in.flatMap(list -> list.stream())" + + ".filter(Optional::isPresent).map(Optional::get);", " }", - " List f2(List>> in) {", - " return in.stream().flatMap(list -> list.stream()" - + ".filter(Optional::isPresent).map(Optional::get)).collect(Collectors.toList());", + " Stream f4(Stream>> in) {", + " return in.filter(Optional::isPresent).map(Optional::get)" + + ".filter(Optional::isPresent).map(Optional::get);", " }", "}") .doTest();