-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Changes from all commits
66d527c
a777920
35eb00e
0d8ab56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -283,9 +283,7 @@ private BiConsumer<List<Object>, Throwable> buildFieldValueMap(List<String> fiel | |
| DeferredExecutionSupport createDeferredExecutionSupport(ExecutionContext executionContext, ExecutionStrategyParameters parameters) { | ||
| MergedSelectionSet fields = parameters.getFields(); | ||
|
|
||
| return Optional.ofNullable(executionContext.getGraphQLContext()) | ||
| .map(graphqlContext -> graphqlContext.getBoolean(ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT)) | ||
| .orElse(false) ? | ||
| return executionContext.hasIncrementalSupport() ? | ||
| new DeferredExecutionSupport.DeferredExecutionSupportImpl( | ||
| fields, | ||
| parameters, | ||
|
|
@@ -908,9 +906,7 @@ protected Object completeValueForObject(ExecutionContext executionContext, Execu | |
| MergedSelectionSet subFields = fieldCollector.collectFields( | ||
| collectorParameters, | ||
| parameters.getField(), | ||
| Optional.ofNullable(executionContext.getGraphQLContext()) | ||
| .map(graphqlContext -> graphqlContext.getBoolean(ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT)) | ||
| .orElse(false) | ||
| executionContext.hasIncrementalSupport() | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is more critical path - avoids two Optional allocations |
||
| ); | ||
|
|
||
| ExecutionStepInfo newExecutionStepInfo = executionStepInfo.changeTypeWithPreservedNonNull(resolvedObjectType); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| import graphql.execution.ExecutionContext; | ||
| import graphql.execution.ExecutionStrategyParameters; | ||
| import graphql.execution.FieldValueInfo; | ||
| import graphql.execution.MergedField; | ||
| import graphql.schema.DataFetcher; | ||
| import graphql.util.LockKit; | ||
| import org.dataloader.DataLoaderRegistry; | ||
|
|
@@ -195,10 +196,13 @@ public void fieldFetched(ExecutionContext executionContext, | |
| } | ||
|
|
||
| private void increaseCallCounts(int curLevel, ExecutionStrategyParameters parameters) { | ||
| int nonDeferredFieldCount = (int) parameters.getFields().getSubFieldsList().stream() | ||
| .filter(field -> !field.isDeferred()) | ||
| .count(); | ||
|
|
||
| int count = 0; | ||
| for (MergedField field : parameters.getFields().getSubFieldsList()) { | ||
| if (!field.isDeferred()) { | ||
| count++; | ||
| } | ||
| } | ||
| int nonDeferredFieldCount = count; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is more critical path when defer is on |
||
| callStack.lock.runLocked(() -> { | ||
| callStack.increaseExpectedFetchCount(curLevel, nonDeferredFieldCount); | ||
| callStack.increaseHappenedStrategyCalls(curLevel); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,12 @@ | ||
| package graphql.incremental; | ||
|
|
||
| import graphql.ExperimentalApi; | ||
| import graphql.util.FpKit; | ||
|
|
||
| import java.util.Collections; | ||
| import java.util.LinkedHashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| @ExperimentalApi | ||
| public class DelayedIncrementalPartialResultImpl implements DelayedIncrementalPartialResult { | ||
|
|
@@ -44,10 +44,12 @@ public Map<String, Object> toSpecification() { | |
| result.put("extensions", extensions); | ||
| } | ||
|
|
||
| if(incrementalItems != null) { | ||
| result.put("incremental", incrementalItems.stream() | ||
| .map(IncrementalPayload::toSpecification) | ||
| .collect(Collectors.toList())); | ||
| if (incrementalItems != null) { | ||
| List<Map<String, Object>> list = FpKit.arrayListSizedTo(incrementalItems); | ||
| for (IncrementalPayload incrementalItem : incrementalItems) { | ||
| list.add(incrementalItem.toSpecification()); | ||
| } | ||
| result.put("incremental", list); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. critical path when defer is on |
||
| } | ||
|
|
||
| return result; | ||
|
|
||
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 is critical path stuff and is used in a number of places