Minor performance fixes by dfa1 · Pull Request #3236 · graphql-java/graphql-java · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ public CompletableFuture<ExecutionResult> execute(ExecutionContext executionCont
handleResultsConsumer.accept(null, throwable.getCause());
return;
}

Async.CombinedBuilder<ExecutionResult> executionResultFutures = Async.ofExpectedSize(completeValueInfos.size());
for (FieldValueInfo completeValueInfo : completeValueInfos) {
executionResultFutures.add(completeValueInfo.getFieldValue());
Expand Down
39 changes: 1 addition & 38 deletions src/main/java/graphql/execution/ExecutionStepInfoFactory.java
Original file line number Diff line number Diff line change
@@ -1,52 +1,15 @@
package graphql.execution;

import graphql.Internal;
import graphql.introspection.Introspection;
import graphql.language.Argument;
import graphql.schema.GraphQLCodeRegistry;
import graphql.schema.GraphQLFieldDefinition;
import graphql.schema.GraphQLList;
import graphql.schema.GraphQLObjectType;
import graphql.schema.GraphQLOutputType;
import graphql.util.FpKit;

import java.util.List;
import java.util.Map;
import java.util.function.Supplier;

@Internal
public class ExecutionStepInfoFactory {

public ExecutionStepInfo newExecutionStepInfoForSubField(ExecutionContext executionContext, MergedField mergedField, ExecutionStepInfo parentInfo) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just checked - code was never used

GraphQLObjectType parentType = (GraphQLObjectType) parentInfo.getUnwrappedNonNullType();
GraphQLFieldDefinition fieldDefinition = Introspection.getFieldDef(executionContext.getGraphQLSchema(), parentType, mergedField.getName());
GraphQLOutputType fieldType = fieldDefinition.getType();
List<Argument> fieldArgs = mergedField.getArguments();
GraphQLCodeRegistry codeRegistry = executionContext.getGraphQLSchema().getCodeRegistry();
Supplier<Map<String, Object>> argumentValuesSupplier = () -> ValuesResolver.getArgumentValues(codeRegistry,
fieldDefinition.getArguments(),
fieldArgs,
executionContext.getCoercedVariables(),
executionContext.getGraphQLContext(),
executionContext.getLocale());
Supplier<Map<String, Object>> argumentValues = FpKit.intraThreadMemoize(argumentValuesSupplier);

ResultPath newPath = parentInfo.getPath().segment(mergedField.getResultKey());

return parentInfo.transform(builder -> builder
.parentInfo(parentInfo)
.type(fieldType)
.fieldDefinition(fieldDefinition)
.fieldContainer(parentType)
.field(mergedField)
.path(newPath)
.arguments(argumentValues));
}

public ExecutionStepInfo newExecutionStepInfoForListElement(ExecutionStepInfo executionInfo, int index) {
public ExecutionStepInfo newExecutionStepInfoForListElement(ExecutionStepInfo executionInfo, ResultPath indexedPath) {
GraphQLList fieldType = (GraphQLList) executionInfo.getUnwrappedNonNullType();
GraphQLOutputType typeInList = (GraphQLOutputType) fieldType.getWrappedType();
ResultPath indexedPath = executionInfo.getPath().segment(index);

@dfa1 dfa1 May 30, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

according to my memory profiler, this is dropping from 3-4% of total heap to just 1-2% (as expected)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just to explain to others - the indexedPath was allocated out side this method and then reallocated again here. This double allocation has been removed

return executionInfo.transform(builder -> builder
.parentInfo(executionInfo)
.type(typeInList)
Expand Down
5 changes: 1 addition & 4 deletions src/main/java/graphql/execution/ExecutionStrategy.java
Original file line number Diff line number Diff line change
Expand Up @@ -569,19 +569,16 @@ protected FieldValueInfo completeValueForList(ExecutionContext executionContext,
for (Object item : iterableValues) {
ResultPath indexedPath = parameters.getPath().segment(index);

ExecutionStepInfo stepInfoForListElement = executionStepInfoFactory.newExecutionStepInfoForListElement(executionStepInfo, index);
ExecutionStepInfo stepInfoForListElement = executionStepInfoFactory.newExecutionStepInfoForListElement(executionStepInfo, indexedPath);

NonNullableFieldValidator nonNullableFieldValidator = new NonNullableFieldValidator(executionContext, stepInfoForListElement);

int finalIndex = index;
FetchedValue value = unboxPossibleDataFetcherResult(executionContext, parameters, item);

ExecutionStrategyParameters newParameters = parameters.transform(builder ->
builder.executionStepInfo(stepInfoForListElement)
.nonNullFieldValidator(nonNullableFieldValidator)
.listSize(size.orElse(-1)) // -1 signals that we don't know the size
.localContext(value.getLocalContext())
.currentListIndex(finalIndex)
.path(indexedPath)
.source(value.getFetchedValue())
);
Expand Down
30 changes: 1 addition & 29 deletions src/main/java/graphql/execution/ExecutionStrategyParameters.java