FpKit now longer uses streams for performance reasons #3932
Conversation
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
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.♻️ This comment has been updated with latest results. |
| // 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;
}
There was a problem hiding this comment.
Note two allocations - neater code - worse performance
| LinkedHashMap::new) | ||
| ); | ||
| 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
| @@ -240,11 +259,6 @@ public static <T> List<T> valuesToList(Map<?, T> map) { | |||
| return new ArrayList<>(map.values()); | |||
| } | |||
|
|
|||
| 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
| throwingMerger(), | ||
| LinkedHashMap::new) | ||
| ); | ||
| } |
There was a problem hiding this comment.
never used - so I removed it - one less stream thing to port
| @@ -98,6 +98,100 @@ class FpKitTest extends Specification { | |||
| 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!!!
| 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
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
containsKeyoperation may be used to distinguish these two cases.
| @@ -261,21 +278,13 @@ public static <T> List<List<T>> transposeMatrix(List<? extends List<T>> matrix) | |||
| return result; | |||
| } | |||
|
|
|||
| 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
| return listLists.stream() | ||
| .flatMap(List::stream) | ||
| .collect(ImmutableList.toImmutableList()); | ||
| } |
There was a problem hiding this comment.
never used - no need to port it
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