Introduce `StreamMapFilter` Refaster rule #1467

zphilipp · 2024-12-18T08:20:51Z

Prefer

stream.map(map::get).filter(Objects::nonNull)

over

stream.filter(map::containsKey).map(map::get)

Suggested commit message

Introduce `StreamMapFilter` Refaster rule (#1467)

github-actions · 2024-12-18T08:23:14Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2024-12-18T08:34:50Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java

werli · 2024-12-18T09:37:12Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java

(Unrelated to this code)
The same pattern may also be applicable for Flux where we should call Flux#mapNotNull. Maybe good follow up? 👀

Sure, I can create another PR. As @mohamedsamehsalah tells me, this will get me closer to my sticker. 😁

You will definitely receive one for this contribution already!

github-actions · 2024-12-18T10:06:46Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Nice one! Some small points (didn't apply them yet 😬)

rickie · 2024-12-20T13:29:59Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java

+  static final class StreamMapFilter<K, V> {
+    @BeforeTemplate
+    Stream<V> before(Stream<K> stream, Map<K, V> map) {
+      return stream.filter(map::containsKey).map(map::get);


I'm curious about the performance. As usually we try to moving filtering to the "earliest" spot, right? I guess preventing the double lookup makes up for that 🤔 ?

Again a reason why having a "benchmarking-framework" in place in EPS would be useful.

Indeed we should find time to progress on such a framework. For now I wrote a custom benchmark:

import static com.google.common.collect.ImmutableMap.toImmutableMap; import static java.util.function.Function.identity; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import java.util.Objects; import java.util.concurrent.TimeUnit; import java.util.regex.Pattern; import org.openjdk.jmh.annotations.Benchmark; import org.openjdk.jmh.annotations.BenchmarkMode; import org.openjdk.jmh.annotations.Fork; import org.openjdk.jmh.annotations.Measurement; import org.openjdk.jmh.annotations.Mode; import org.openjdk.jmh.annotations.OutputTimeUnit; import org.openjdk.jmh.annotations.Scope; import org.openjdk.jmh.annotations.Setup; import org.openjdk.jmh.annotations.State; import org.openjdk.jmh.annotations.Warmup; import org.openjdk.jmh.profile.GCProfiler; import org.openjdk.jmh.runner.Runner; import org.openjdk.jmh.runner.RunnerException; import org.openjdk.jmh.runner.options.OptionsBuilder; @State(Scope.Benchmark) @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.MICROSECONDS) @Fork( jvmArgs = {"-Xms256M", "-Xmx256M"}, value = 1) @Warmup(iterations = 5) @Measurement(iterations = 10) public class Benchmark { private ImmutableList<String> elements; private ImmutableMap<String, String> map; public static void main(String[] args) throws RunnerException { String testRegex = Pattern.quote(Benchmark.class.getCanonicalName()); new Runner(new OptionsBuilder().include(testRegex).addProfiler(GCProfiler.class).build()).run(); } @Setup public void setUp() { elements = ImmutableList.of("a", "b", "c", "d", "e", "f", "g", "h", "i", "j"); map = elements.stream() .filter(e -> e.charAt(0) % 2 == 0) .collect(toImmutableMap(identity(), e -> e + e)); } @Benchmark public long filterFirst() { return elements.stream().filter(map::containsKey).map(map::get).count(); } @Benchmark public long mapFirst() { return elements.stream().map(map::get).filter(Objects::nonNull).count(); } }

This yielded the following results:

Benchmark Mode Cnt Score Error Units Benchmark.filterFirst avgt 10 0.116 ± 0.001 us/op Benchmark.filterFirst:gc.alloc.rate avgt 10 2559.065 ± 21.986 MB/sec Benchmark.filterFirst:gc.alloc.rate.norm avgt 10 312.000 ± 0.001 B/op Benchmark.filterFirst:gc.count avgt 10 1687.000 counts Benchmark.filterFirst:gc.time avgt 10 1060.000 ms Benchmark.mapFirst avgt 10 0.095 ± 0.001 us/op Benchmark.mapFirst:gc.alloc.rate avgt 10 2978.998 ± 14.750 MB/sec Benchmark.mapFirst:gc.alloc.rate.norm avgt 10 296.000 ± 0.001 B/op Benchmark.mapFirst:gc.count avgt 10 1964.000 counts Benchmark.mapFirst:gc.time avgt 10 1416.000 ms

So this looks like a good change, performance-wise 👍

Thanks you two for challenging this and showing that it actually results in an improvement. 💪

rickie · 2024-12-20T13:30:03Z

...ne-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StreamRulesTestInput.java

@@ -141,6 +141,11 @@ boolean testStreamFindAnyIsPresent() {
    return Stream.of(1).findFirst().isPresent();
  }

+  Stream<String> testStreamMapFilter() {
+    ImmutableMap<String, String> map = ImmutableMap.of("foo", "bar");


We can inline these values :)

rickie · 2024-12-20T13:31:19Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java

@@ -297,6 +297,23 @@ boolean after(Stream<T> stream) {
    }
  }

+  /**
+   * When using {@link Map#get(Object)} in a {@link Stream#map(Function)} operation, prefer


I would rephrase this to Prefer {@link ..} over {@link ..} to avoid double look-up or something :).

Stephan202

Rebased and added a commit. Congrats on your first contribution @zphilipp! 🎉

Stephan202 · 2024-12-18T13:23:46Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java

+  static final class StreamMapFilter<K, V> {
+    @BeforeTemplate
+    Stream<V> before(Stream<K> stream, Map<K, V> map) {
+      return stream.filter(map::containsKey).map(map::get);
+    }
+
+    @AfterTemplate
+    Stream<V> after(Stream<K> stream, Map<K, V> map) {
+      return stream.map(map::get).filter(Objects::nonNull);
+    }
+  }


Map#get and Map#containsKey accept an Object, so we can generalize the rule by not requiring that the Stream has elements of type K.

Stephan202 · 2024-12-20T22:29:50Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java

+  static final class StreamMapFilter<K, V> {
+    @BeforeTemplate
+    Stream<V> before(Stream<K> stream, Map<K, V> map) {
+      return stream.filter(map::containsKey).map(map::get);


Indeed we should find time to progress on such a framework. For now I wrote a custom benchmark:

import static com.google.common.collect.ImmutableMap.toImmutableMap; import static java.util.function.Function.identity; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import java.util.Objects; import java.util.concurrent.TimeUnit; import java.util.regex.Pattern; import org.openjdk.jmh.annotations.Benchmark; import org.openjdk.jmh.annotations.BenchmarkMode; import org.openjdk.jmh.annotations.Fork; import org.openjdk.jmh.annotations.Measurement; import org.openjdk.jmh.annotations.Mode; import org.openjdk.jmh.annotations.OutputTimeUnit; import org.openjdk.jmh.annotations.Scope; import org.openjdk.jmh.annotations.Setup; import org.openjdk.jmh.annotations.State; import org.openjdk.jmh.annotations.Warmup; import org.openjdk.jmh.profile.GCProfiler; import org.openjdk.jmh.runner.Runner; import org.openjdk.jmh.runner.RunnerException; import org.openjdk.jmh.runner.options.OptionsBuilder; @State(Scope.Benchmark) @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.MICROSECONDS) @Fork( jvmArgs = {"-Xms256M", "-Xmx256M"}, value = 1) @Warmup(iterations = 5) @Measurement(iterations = 10) public class Benchmark { private ImmutableList<String> elements; private ImmutableMap<String, String> map; public static void main(String[] args) throws RunnerException { String testRegex = Pattern.quote(Benchmark.class.getCanonicalName()); new Runner(new OptionsBuilder().include(testRegex).addProfiler(GCProfiler.class).build()).run(); } @Setup public void setUp() { elements = ImmutableList.of("a", "b", "c", "d", "e", "f", "g", "h", "i", "j"); map = elements.stream() .filter(e -> e.charAt(0) % 2 == 0) .collect(toImmutableMap(identity(), e -> e + e)); } @Benchmark public long filterFirst() { return elements.stream().filter(map::containsKey).map(map::get).count(); } @Benchmark public long mapFirst() { return elements.stream().map(map::get).filter(Objects::nonNull).count(); } }

This yielded the following results:

Benchmark Mode Cnt Score Error Units Benchmark.filterFirst avgt 10 0.116 ± 0.001 us/op Benchmark.filterFirst:gc.alloc.rate avgt 10 2559.065 ± 21.986 MB/sec Benchmark.filterFirst:gc.alloc.rate.norm avgt 10 312.000 ± 0.001 B/op Benchmark.filterFirst:gc.count avgt 10 1687.000 counts Benchmark.filterFirst:gc.time avgt 10 1060.000 ms Benchmark.mapFirst avgt 10 0.095 ± 0.001 us/op Benchmark.mapFirst:gc.alloc.rate avgt 10 2978.998 ± 14.750 MB/sec Benchmark.mapFirst:gc.alloc.rate.norm avgt 10 296.000 ± 0.001 B/op Benchmark.mapFirst:gc.count avgt 10 1964.000 counts Benchmark.mapFirst:gc.time avgt 10 1416.000 ms

So this looks like a good change, performance-wise 👍

github-actions · 2024-12-20T22:56:46Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Congrats on your first PR @zphilipp !

rickie

Congrats on your first PR @zphilipp !

rickie · 2024-12-22T12:46:55Z

😓 idk why it's doing it twice now.

Anyway, anything from your side @werli :)?

werli · 2024-12-23T08:13:44Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java

+  static final class StreamMapFilter<K, V> {
+    @BeforeTemplate
+    Stream<V> before(Stream<K> stream, Map<K, V> map) {
+      return stream.filter(map::containsKey).map(map::get);


Thanks you two for challenging this and showing that it actually results in an improvement. 💪

github-actions · 2024-12-23T08:18:19Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2024-12-23T08:28:47Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

sonarqubecloud · 2024-12-23T08:28:49Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

zphilipp force-pushed the pzeipert/stream-map-to-map-get branch from 3b002d9 to cb2e17d Compare December 18, 2024 08:32

werli reviewed Dec 18, 2024

View reviewed changes

zphilipp force-pushed the pzeipert/stream-map-to-map-get branch from cb2e17d to 436aded Compare December 18, 2024 10:04

zphilipp requested a review from werli December 18, 2024 10:07

Stephan202 added this to the 0.20.0 milestone Dec 18, 2024

Stephan202 added the new feature label Dec 18, 2024

zphilipp changed the title ~~Introduce StreamMapToValuesFromMap Refaster rule~~ Introduce StreamMapFilter Refaster rule Dec 20, 2024

rickie reviewed Dec 20, 2024

View reviewed changes

Stephan202 force-pushed the pzeipert/stream-map-to-map-get branch from 436aded to 0821705 Compare December 20, 2024 22:54

Stephan202 approved these changes Dec 20, 2024

View reviewed changes

rickie approved these changes Dec 22, 2024

View reviewed changes

werli approved these changes Dec 23, 2024

View reviewed changes

Stephan202 force-pushed the pzeipert/stream-map-to-map-get branch from 0821705 to dadd141 Compare December 23, 2024 08:16

zphilipp and others added 3 commits December 23, 2024 09:26

Introduce StreamMapToValuesFromMap Refaster rule

8f5bd0f

Rename rule and add documentation

96a1bdd

Suggestions

b30af54

Stephan202 force-pushed the pzeipert/stream-map-to-map-get branch from dadd141 to b30af54 Compare December 23, 2024 08:26

rickie merged commit 0fa59a5 into master Dec 23, 2024
16 checks passed

rickie deleted the pzeipert/stream-map-to-map-get branch December 23, 2024 08:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `StreamMapFilter` Refaster rule #1467

Introduce `StreamMapFilter` Refaster rule #1467

zphilipp commented Dec 18, 2024 •

edited by Stephan202

Loading

github-actions bot commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

werli Dec 18, 2024

zphilipp Dec 18, 2024

rickie Dec 20, 2024

github-actions bot commented Dec 18, 2024

rickie left a comment

rickie Dec 20, 2024

Stephan202 Dec 20, 2024

werli Dec 23, 2024

rickie Dec 20, 2024

rickie Dec 20, 2024

Stephan202 left a comment

Stephan202 Dec 18, 2024

Stephan202 Dec 20, 2024

github-actions bot commented Dec 20, 2024

rickie left a comment

rickie left a comment

rickie commented Dec 22, 2024

werli Dec 23, 2024

github-actions bot commented Dec 23, 2024

github-actions bot commented Dec 23, 2024

sonarqubecloud bot commented Dec 23, 2024

Introduce StreamMapFilter Refaster rule #1467

Introduce StreamMapFilter Refaster rule #1467

Conversation

zphilipp commented Dec 18, 2024 • edited by Stephan202 Loading

Suggested commit message

github-actions bot commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 18, 2024

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 20, 2024

rickie left a comment

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

rickie commented Dec 22, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 23, 2024

github-actions bot commented Dec 23, 2024

sonarqubecloud bot commented Dec 23, 2024

Quality Gate passed

Introduce `StreamMapFilter` Refaster rule #1467

Introduce `StreamMapFilter` Refaster rule #1467

zphilipp commented Dec 18, 2024 •

edited by Stephan202

Loading