GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — FpKit now longer uses streams for performance reasons by bbakerman · Pull Request #3932 · graphql-java/graphql-java

Conversation

bbakerman commented Apr 27, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          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

Comment thread

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

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

bbakerman Apr 27, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new common generic function - used in other places

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

github-actions Bot commented Apr 27, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          FpKit now longer uses streams for performance reasons - tweak

36fc91c

bbakerman commented

Apr 27, 2025

View reviewed changes

Comment thread

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
		//

bbakerman Apr 27, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman Apr 27, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note two allocations - neater code - worse performance

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman commented

Apr 27, 2025

View reviewed changes

Comment thread

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());
		}

bbakerman Apr 27, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman commented

Apr 27, 2025

View reviewed changes

Comment thread

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());
		}

bbakerman Apr 27, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman commented

Apr 27, 2025

View reviewed changes

Comment thread

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

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

bbakerman Apr 27, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman commented

Apr 27, 2025

View reviewed changes

Comment thread

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

		l == ["Parrot"]
		}

		class Person {

bbakerman Apr 27, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dondonz May 14, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noice

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

andimarek requested changes

Apr 27, 2025

View reviewed changes

Comment thread

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)) {

andimarek Apr 27, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman Apr 27, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Comment thread

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


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

bbakerman Apr 27, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never used - no need to port it

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman commented

Apr 27, 2025

View reviewed changes

Comment thread

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

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

bbakerman Apr 27, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never used - no need to port it

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Comment thread

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!

dondonz May 14, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dondonz reviewed

May 14, 2025

View reviewed changes

Comment thread

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

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

dondonz May 14, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dondonz commented May 14, 2025

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dondonz approved these changes

May 14, 2025

View reviewed changes

dondonz merged commit 6fcb381 into master

May 14, 2025

dondonz mentioned this pull request

May 15, 2025

Cherry pick 3932 to remove streams from FPKit #3971

Merged

Reviewers

andimarek

andimarek requested changes

dondonz

dondonz approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

performance

work that is primarily targeted as performance improvements

Projects

None yet

Milestone

24.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

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!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bbakerman commented Apr 27, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot commented Apr 27, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.