FpKit now longer uses streams for performance reasons - #3932
Conversation
Sorry, something went wrong.
This reverts commit 262ff2f.
| resultMap.put(key, obj); | ||
| } | ||
| } | ||
| return resultMap; |
There was a problem hiding this comment.
new common generic function - used in other places
Sorry, something went wrong.
Test Results 312 files ±0 312 suites ±0 53s ⏱️ -1s 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. |
Sorry, something went wrong.
| // 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 | ||
| // |
There was a problem hiding this comment.
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;
}
Sorry, something went wrong.
There was a problem hiding this comment.
Note two allocations - neater code - worse performance
Sorry, something went wrong.
| ); | ||
| public static <T, NewKey> Map<NewKey, T> toMapByUniqueKey(Collection<T> list, Function<T, NewKey> keyFunction) { | ||
| return toMap(list, keyFunction, throwingMerger()); | ||
| } |
There was a problem hiding this comment.
This was renamed from groupingByUniqueKey to toMapByUniqueKey because its NOT a group by at all. Its a toMap - it was badly named
Sorry, something went wrong.
|
|
||
| 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()); | ||
| } |
There was a problem hiding this comment.
never used so I removed it - one less stream thing to port
Sorry, something went wrong.
| throwingMerger(), | ||
| LinkedHashMap::new) | ||
| ); | ||
| } |
There was a problem hiding this comment.
never used - so I removed it - one less stream thing to port
Sorry, something went wrong.
| l == ["Parrot"] | ||
| } | ||
|
|
||
| class Person { |
There was a problem hiding this comment.
hey look - TESTS!
We never had very many tests for FpKit
So I I write them first - then changed the implementation
TDD!!!
Sorry, something went wrong.
There was a problem hiding this comment.
Noice
Sorry, something went wrong.
| Map<NewKey, T> resultMap = new LinkedHashMap<>(); | ||
| for (T obj : collection) { | ||
| NewKey key = keyFunction.apply(obj); | ||
| if (resultMap.containsKey(key)) { |
There was a problem hiding this comment.
Make it one less statement by looking up the value and save it one call
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
|
||
| public static <T> CompletableFuture<List<T>> flatList(CompletableFuture<List<List<T>>> cf) { | ||
| return cf.thenApply(FpKit::flatList); | ||
| } |
There was a problem hiding this comment.
never used - no need to port it
Sorry, something went wrong.
| return listLists.stream() | ||
| .flatMap(List::stream) | ||
| .collect(ImmutableList.toImmutableList()); | ||
| } |
There was a problem hiding this comment.
never used - no need to port it
Sorry, something went wrong.
| return Collections.emptyMap(); | ||
| } | ||
| // Convert builders to ImmutableLists in place to avoid an extra allocation | ||
| // yes the code is yuck - but its more performant yuck! |
There was a problem hiding this comment.
Do you mind if I borrow this line for the release notes?
Sorry, something went wrong.
| .add(item); | ||
| } | ||
| } | ||
| if (resutMap.isEmpty()) { |
There was a problem hiding this comment.
I think you meant to call it a resultMap, anyway not blocking this PR
Sorry, something went wrong.
|
Leaving a breadcrumb: I have Andi's blessing to approve the PR, he originally had marked this as "changes requested" |
Sorry, something went wrong.
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