FpKit now longer uses streams for performance reasons by bbakerman · Pull Request #3932 · graphql-java/graphql-java

bbakerman · 2025-04-27T00:51:48Z

We no longer uses .stream() since its slower than an imperative style.

The methods were lacking tests so I went all TDD on it and write tests fist with the old streams implementation and then changed it and watched the tests pass

This reverts commit 262ff2f.

bbakerman · 2025-04-27T00:53:18Z

src/main/java/graphql/util/FpKit.java

+                resultMap.put(key, obj);
+            }
+        }
+        return resultMap;


new common generic function - used in other places

github-actions · 2025-04-27T00:55:53Z

Test Results

312 files ±0 312 suites ±0 53s ⏱️ -1s
3 586 tests +5 3 581 ✅ +5 5 💤 ±0 0 ❌ ±0
3 675 runs +5 3 670 ✅ +5 5 💤 ±0 0 ❌ ±0

Results for commit c81cee8. ± Comparison against base commit 262ff2f.

This pull request removes 173 and adds 156 tests. Note that renamed tests count towards both.

	?
                __schema { types { fields { args { type { name fields { name }}}}}}
                __schema { types { fields { type { name fields { name }}}}}
                __schema { types { inputFields { type { inputFields { name }}}}}
                __schema { types { interfaces { fields { type { interfaces { name } } } } } }
                __schema { types { name} }
                __type(name : "t") { name }
                a1: __schema { types { name} }
                a1: __type(name : "t") { name }
                a2 :  __type(name : "t1") { name }
…

graphql.DataFetcherTest ‑ get Boolean property value [fetcher: <graphql.schema.PropertyDataFetcher@7fd69dd propertyName=booleanField function=null>, #0]
graphql.DataFetcherTest ‑ get Boolean property value [fetcher: <graphql.schema.SingletonPropertyDataFetcher@33364212>, #1]
graphql.DataFetcherTest ‑ get Boolean property value with get [fetcher: <graphql.schema.PropertyDataFetcher@23940f86 propertyName=booleanFieldWithGet function=null>, #0]
graphql.DataFetcherTest ‑ get Boolean property value with get [fetcher: <graphql.schema.SingletonPropertyDataFetcher@33364212>, #1]
graphql.DataFetcherTest ‑ get property value [fetcher: <graphql.schema.SingletonPropertyDataFetcher@33364212>, #1]
graphql.DataFetcherTest ‑ get public field value as property [fetcher: <graphql.schema.PropertyDataFetcher@7318daf8 propertyName=publicField function=null>, #0]
graphql.DataFetcherTest ‑ get public field value as property [fetcher: <graphql.schema.SingletonPropertyDataFetcher@33364212>, #1]
graphql.ScalarsBooleanTest ‑ parseValue throws exception for invalid input <java.lang.Object@47e0adb4>
graphql.ScalarsBooleanTest ‑ serialize throws exception for invalid input <java.lang.Object@20094313>
graphql.ScalarsIDTest ‑ parseValue allows any object via String.valueOf <java.lang.Object@52b288a2>
…

♻️ This comment has been updated with latest results.

bbakerman · 2025-04-27T00:56:54Z

src/main/java/graphql/util/FpKit.java

+        // The cleanest version of this code would have two maps, one of immutable list builders and one
+        // of the built immutable lists.  BUt we are trying to be performant and memory efficient so
+        // we treat it as a map of objects and cast like its Java 4x
+        //


This comment above explains the whacky code. One less map allocation

Alternative is

public static <T, NewKey> Map<NewKey, ImmutableList<T>> groupingBy(Collection<T> list, Function<T, NewKey> function) { Map<NewKey, ImmutableList.Builder<T>> tempMap = new LinkedHashMap<>(); for (T item : list) { NewKey key = function.apply(item); tempMap.computeIfAbsent(key, k -> ImmutableList.builder()).add(item); } Map<NewKey, ImmutableList<T>> resultMap = new LinkedHashMap<>(); for (Map.Entry<NewKey, ImmutableList.Builder<T>> entry : tempMap.entrySet()) { resultMap.put(entry.getKey(), entry.getValue().build()); } return resultMap; }

Note two allocations - neater code - worse performance

bbakerman · 2025-04-27T00:58:04Z

src/main/java/graphql/util/FpKit.java

-        );
+    public static <T, NewKey> Map<NewKey, T> toMapByUniqueKey(Collection<T> list, Function<T, NewKey> keyFunction) {
+        return toMap(list, keyFunction, throwingMerger());
    }


This was renamed from groupingByUniqueKey to toMapByUniqueKey because its NOT a group by at all. Its a toMap - it was badly named

bbakerman · 2025-04-27T00:58:31Z

src/main/java/graphql/util/FpKit.java


-    public static <K, V, U> List<U> mapEntries(Map<K, V> map, BiFunction<K, V, U> function) {
-        return map.entrySet().stream().map(entry -> function.apply(entry.getKey(), entry.getValue())).collect(Collectors.toList());
-    }


never used so I removed it - one less stream thing to port

bbakerman · 2025-04-27T00:59:03Z

src/main/java/graphql/util/FpKit.java

-                throwingMerger(),
-                LinkedHashMap::new)
-        );
-    }


never used - so I removed it - one less stream thing to port

bbakerman · 2025-04-27T00:59:47Z

src/test/groovy/graphql/util/FpKitTest.groovy

        l == ["Parrot"]
    }

+    class Person {


hey look - TESTS!

We never had very many tests for FpKit

So I I write them first - then changed the implementation

TDD!!!

andimarek · 2025-04-27T03:26:45Z

src/main/java/graphql/util/FpKit.java

+        Map<NewKey, T> resultMap = new LinkedHashMap<>();
+        for (T obj : collection) {
+            NewKey key = keyFunction.apply(obj);
+            if (resultMap.containsKey(key)) {


Make it one less statement by looking up the value and save it one call

Technically not the right semantics in a general sense - sure there could be non

If this map permits null values, then a return value of null does not necessarily indicate that the map contains no mapping for the key; it's also possible that the map explicitly maps the key to null. The containsKey operation may be used to distinguish these two cases.

…ed unused code

bbakerman · 2025-04-27T06:30:47Z

src/main/java/graphql/util/FpKit.java


-    public static <T> CompletableFuture<List<T>> flatList(CompletableFuture<List<List<T>>> cf) {
-        return cf.thenApply(FpKit::flatList);
-    }


never used - no need to port it

bbakerman · 2025-04-27T06:31:01Z

src/main/java/graphql/util/FpKit.java

-        return listLists.stream()
-                .flatMap(List::stream)
-                .collect(ImmutableList.toImmutableList());
-    }


never used - no need to port it

dondonz · 2025-05-14T07:30:16Z

src/main/java/graphql/util/FpKit.java

+            return Collections.emptyMap();
+        }
+        // Convert builders to ImmutableLists in place to avoid an extra allocation
+        // yes the code is yuck -  but its more performant yuck!


Do you mind if I borrow this line for the release notes?

dondonz · 2025-05-14T07:31:13Z

src/main/java/graphql/util/FpKit.java

+                        .add(item);
+            }
+        }
+        if (resutMap.isEmpty()) {


I think you meant to call it a resultMap, anyway not blocking this PR

dondonz · 2025-05-14T07:36:48Z

Leaving a breadcrumb: I have Andi's blessing to approve the PR, he originally had marked this as "changes requested"

Revert "Revert "FpKit now longer uses streams for performance reasons""

26bb001

This reverts commit 262ff2f.

bbakerman changed the title ~~Revert "Revert "FpKit now longer uses streams for performance reasons""~~ FpKit now longer uses streams for performance reasons Apr 27, 2025

bbakerman commented Apr 27, 2025

View reviewed changes

FpKit now longer uses streams for performance reasons - tweak

36fc91c

bbakerman commented Apr 27, 2025

View reviewed changes

andimarek requested changes Apr 27, 2025

View reviewed changes

bbakerman added the performance work that is primarily targeted as performance improvements label Apr 27, 2025

FpKit now longer uses streams for performance reasons - tweak - remov…

c81cee8

…ed unused code

bbakerman commented Apr 27, 2025

View reviewed changes

bbakerman requested a review from dondonz April 27, 2025 06:31

dondonz modified the milestones: 24.x non-breaking changes and bugfixes, 25.x breaking changes, 24.0 May 11, 2025

dondonz reviewed May 14, 2025

View reviewed changes

dondonz approved these changes May 14, 2025

View reviewed changes

dondonz merged commit 6fcb381 into master May 14, 2025
2 checks passed

dondonz mentioned this pull request May 15, 2025

Cherry pick 3932 to remove streams from FPKit #3971

Merged

Conversation

bbakerman commented Apr 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dondonz commented May 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bbakerman commented Apr 27, 2025 •

edited

Loading

github-actions bot commented Apr 27, 2025 •

edited

Loading