Skip to content

Removing some of the Optional.map() and .stream() for performance reasons#3930

Merged
dondonz merged 4 commits intomasterfrom
remove-optional-streams-style-code
May 15, 2025
Merged

Removing some of the Optional.map() and .stream() for performance reasons#3930
dondonz merged 4 commits intomasterfrom
remove-optional-streams-style-code

Conversation

@bbakerman
Copy link
Member

@bbakerman bbakerman commented Apr 24, 2025

Some of these are on a critical path so will help a smidge to be more performant.

Others are not and are mostly done via IDEA "rewriting" the .stream() to a direct call

@bbakerman bbakerman added the performance work that is primarily targeted as performance improvements label Apr 24, 2025
bbakerman
bbakerman commented Apr 24, 2025
View reviewed changes
src/main/java/graphql/execution/Execution.java
Optional.ofNullable(executionContext.getGraphQLContext())
.map(graphqlContext -> graphqlContext.getBoolean(ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT))
.orElse(false)
executionContext.hasIncrementalSupport()
Copy link
Member Author

@bbakerman bbakerman Apr 24, 2025

Choose a reason for hiding this comment

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

This is critical path stuff and is used in a number of places

bbakerman
bbakerman commented Apr 24, 2025
View reviewed changes
src/main/java/graphql/execution/ExecutionStrategy.java
Optional.ofNullable(executionContext.getGraphQLContext())
.map(graphqlContext -> graphqlContext.getBoolean(ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT))
.orElse(false)
executionContext.hasIncrementalSupport()
Copy link
Member Author

@bbakerman bbakerman Apr 24, 2025

Choose a reason for hiding this comment

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

This is more critical path - avoids two Optional allocations

bbakerman
bbakerman commented Apr 24, 2025
View reviewed changes
...on/instrumentation/dataloader/PerLevelDataLoaderDispatchStrategyWithDeferAlwaysDispatch.java
count++;
}
}
int nonDeferredFieldCount = count;
Copy link
Member Author

@bbakerman bbakerman Apr 24, 2025

Choose a reason for hiding this comment

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

This is more critical path when defer is on

bbakerman
bbakerman commented Apr 24, 2025
View reviewed changes
src/main/java/graphql/incremental/DelayedIncrementalPartialResultImpl.java
for (IncrementalPayload incrementalItem : incrementalItems) {
list.add(incrementalItem.toSpecification());
}
result.put("incremental", list);
Copy link
Member Author

@bbakerman bbakerman Apr 24, 2025

Choose a reason for hiding this comment

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

critical path when defer is on

@github-actions
Copy link
Contributor

github-actions bot commented Apr 24, 2025 •
edited
Loading

Test Results

  314 files  ±0    314 suites  ±0   53s ⏱️ -1s
3 611 tests +1  3 606 ✅ +1  5 💤 ±0  0 ❌ ±0 
3 700 runs  +1  3 695 ✅ +1  5 💤 ±0  0 ❌ ±0 

Results for commit 0d8ab56. ± Comparison against base commit 397c050.

This pull request removes 174 and adds 153 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@a50d709 propertyName=booleanField function=null>, #0]
graphql.DataFetcherTest ‑ get Boolean property value [fetcher: <graphql.schema.SingletonPropertyDataFetcher@2c88a3e8>, #1]
graphql.DataFetcherTest ‑ get Boolean property value with get [fetcher: <graphql.schema.PropertyDataFetcher@65f651eb propertyName=booleanFieldWithGet function=null>, #0]
graphql.DataFetcherTest ‑ get Boolean property value with get [fetcher: <graphql.schema.SingletonPropertyDataFetcher@2c88a3e8>, #1]
graphql.DataFetcherTest ‑ get property value [fetcher: <graphql.schema.PropertyDataFetcher@529c2a9a propertyName=property function=null>, #0]
graphql.DataFetcherTest ‑ get property value [fetcher: <graphql.schema.SingletonPropertyDataFetcher@2c88a3e8>, #1]
graphql.DataFetcherTest ‑ get public field value as property [fetcher: <graphql.schema.PropertyDataFetcher@73d60e76 propertyName=publicField function=null>, #0]
graphql.DataFetcherTest ‑ get public field value as property [fetcher: <graphql.schema.SingletonPropertyDataFetcher@2c88a3e8>, #1]
graphql.ScalarsBooleanTest ‑ parseValue throws exception for invalid input <java.lang.Object@6c122f96>
graphql.ScalarsBooleanTest ‑ serialize throws exception for invalid input <java.lang.Object@cb390dc>
…

♻️ This comment has been updated with latest results.

@bbakerman bbakerman requested review from andimarek and dondonz April 27, 2025 06:31
@andimarek
Copy link
Member

andimarek commented May 9, 2025

One thing: is Collectors.ToList really the same as ArrayList and should we initialize ArrayList with the right size if we know it?

@dondonz dondonz modified the milestones: 25.x breaking changes, 24.0 May 13, 2025
@bbakerman
Merge remote-tracking branch 'origin/master' into remove-optional-str…
0d8ab56 
…eams-style-code

# Conflicts:
#	src/main/java/graphql/util/FpKit.java
@dondonz dondonz merged commit 0ad655c into master May 15, 2025
2 checks passed
@dondonz dondonz deleted the remove-optional-streams-style-code branch May 15, 2025 06:09
@dondonz dondonz mentioned this pull request May 15, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andimarek andimarek andimarek approved these changes

@dondonz dondonz dondonz approved these changes

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.

3 participants

@bbakerman @andimarek @dondonz