Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
May 15, 2025
Merged

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

merged 4 commits into from
May 15, 2025

Conversation

bbakerman
Copy link
Member

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

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

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

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

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

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