Skip to content
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

Minor performance fixes #3236

Merged
merged 4 commits into from
Jun 2, 2023
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package graphql.execution;

import com.google.common.collect.Maps;
import graphql.ExecutionResult;
import graphql.ExecutionResultImpl;
import graphql.PublicSpi;
Expand Down Expand Up @@ -28,10 +29,9 @@ protected BiConsumer<List<ExecutionResult>, Throwable> handleResults(ExecutionCo
handleNonNullException(executionContext, overallResult, exception);
return;
}
Map<String, Object> resolvedValuesByField = new LinkedHashMap<>(fieldNames.size());
Map<String, Object> resolvedValuesByField = Maps.newLinkedHashMapWithExpectedSize(fieldNames.size());
int ix = 0;
for (ExecutionResult executionResult : results) {

String fieldName = fieldNames.get(ix++);
resolvedValuesByField.put(fieldName, executionResult.getData());
}
Expand Down
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
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);
Copy link
Contributor Author

@dfa1 dfa1 May 30, 2023

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
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
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ public class ExecutionStrategyParameters {
private final NonNullableFieldValidator nonNullableFieldValidator;
private final ResultPath path;
private final MergedField currentField;
private final int listSize;
private final int currentListIndex;
private final ExecutionStrategyParameters parent;

private ExecutionStrategyParameters(ExecutionStepInfo executionStepInfo,
Expand All @@ -30,8 +28,6 @@ private ExecutionStrategyParameters(ExecutionStepInfo executionStepInfo,
NonNullableFieldValidator nonNullableFieldValidator,
ResultPath path,
MergedField currentField,
int listSize,
int currentListIndex,
ExecutionStrategyParameters parent) {

this.executionStepInfo = assertNotNull(executionStepInfo, () -> "executionStepInfo is null");
Expand All @@ -41,8 +37,6 @@ private ExecutionStrategyParameters(ExecutionStepInfo executionStepInfo,
this.nonNullableFieldValidator = nonNullableFieldValidator;
this.path = path;
this.currentField = currentField;
this.listSize = listSize;
this.currentListIndex = currentListIndex;
this.parent = parent;
}

Expand Down Expand Up @@ -70,14 +64,6 @@ public Object getLocalContext() {
return localContext;
}

public int getListSize() {
return listSize;
}

public int getCurrentListIndex() {
return currentListIndex;
}

Copy link
Member

Choose a reason for hiding this comment

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

There are not used anywhere and are informational

public ExecutionStrategyParameters getParent() {
return parent;
}
Expand Down Expand Up @@ -119,8 +105,6 @@ public static class Builder {
NonNullableFieldValidator nonNullableFieldValidator;
ResultPath path = ResultPath.rootPath();
MergedField currentField;
int listSize;
int currentListIndex;
ExecutionStrategyParameters parent;

/**
Expand All @@ -141,8 +125,6 @@ private Builder(ExecutionStrategyParameters oldParameters) {
this.currentField = oldParameters.currentField;
this.path = oldParameters.path;
this.parent = oldParameters.parent;
this.listSize = oldParameters.listSize;
this.currentListIndex = oldParameters.currentListIndex;
}

public Builder executionStepInfo(ExecutionStepInfo executionStepInfo) {
Expand Down Expand Up @@ -185,24 +167,14 @@ public Builder path(ResultPath path) {
return this;
}

public Builder listSize(int listSize) {
this.listSize = listSize;
return this;
}

public Builder currentListIndex(int currentListIndex) {
this.currentListIndex = currentListIndex;
return this;
}

public Builder parent(ExecutionStrategyParameters parent) {
this.parent = parent;
return this;
}


public ExecutionStrategyParameters build() {
return new ExecutionStrategyParameters(executionStepInfo, source, localContext, fields, nonNullableFieldValidator, path, currentField, listSize, currentListIndex, parent);
return new ExecutionStrategyParameters(executionStepInfo, source, localContext, fields, nonNullableFieldValidator, path, currentField, parent);
}
}
}