Skip to content

Avoid allocating a type resolve env if the type is already an object type #2980

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

Merged
bbakerman merged 3 commits into from
Oct 6, 2022
Merged

Avoid allocating a type resolve env if the type is already an object type #2980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7100262
Avoid allocating a type resolve env if the type is already an object …
bbakerman Oct 5, 2022
6004840
Avoid allocating a type resolve env if the type is already an object …
bbakerman Oct 5, 2022
668c098
Avoid allocating a type resolve env if the type is already an object …
bbakerman Oct 5, 2022
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
25 changes: 14 additions & 11 deletions src/main/java/graphql/execution/ExecutionStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,21 +349,21 @@ protected <T> CompletableFuture<T> handleFetchingException(ExecutionContext exec
.build();

try {
return asyncHandleException(dataFetcherExceptionHandler, handlerParameters, executionContext);
return asyncHandleException(dataFetcherExceptionHandler, handlerParameters);
} catch (Exception handlerException) {
Copy link
Member Author

@bbakerman bbakerman Oct 5, 2022

Choose a reason for hiding this comment

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

Boy scout change - the executionContext was never used

handlerParameters = DataFetcherExceptionHandlerParameters.newExceptionParameters()
.dataFetchingEnvironment(environment)
.exception(handlerException)
.build();
return asyncHandleException(new SimpleDataFetcherExceptionHandler(), handlerParameters, executionContext);
return asyncHandleException(new SimpleDataFetcherExceptionHandler(), handlerParameters);
Copy link
Member Author

@bbakerman bbakerman Oct 5, 2022

Choose a reason for hiding this comment

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

Boy scout change - the executionContext was never used

}
}

private <T> CompletableFuture<T> asyncHandleException(DataFetcherExceptionHandler handler, DataFetcherExceptionHandlerParameters handlerParameters, ExecutionContext executionContext) {
private <T> CompletableFuture<T> asyncHandleException(DataFetcherExceptionHandler handler, DataFetcherExceptionHandlerParameters handlerParameters) {
Copy link
Member Author

@bbakerman bbakerman Oct 5, 2022

Choose a reason for hiding this comment

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

Boy scout change - the executionContext was never used

//noinspection unchecked
return handler.handleException(handlerParameters)
.thenApply(handlerResult -> (T) DataFetcherResult.<FetchedValue>newResult().errors(handlerResult.getErrors()).build()
);
return handler.handleException(handlerParameters).thenApply(
handlerResult -> (T) DataFetcherResult.<FetchedValue>newResult().errors(handlerResult.getErrors()).build()
);
}

/**
Expand Down Expand Up @@ -620,7 +620,7 @@ protected CompletableFuture<ExecutionResult> completeValueForScalar(ExecutionCon
protected CompletableFuture<ExecutionResult> completeValueForEnum(ExecutionContext executionContext, ExecutionStrategyParameters parameters, GraphQLEnumType enumType, Object result) {
Object serialized;
try {
serialized = enumType.serialize(result);
serialized = enumType.serialize(result, executionContext.getGraphQLContext(), executionContext.getLocale());
Copy link
Member Author

@bbakerman bbakerman Oct 5, 2022

Choose a reason for hiding this comment

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

Boy scout change - the deprecated method should not have been using this mechanism

} catch (CoercingSerializeException e) {
serialized = handleCoercionProblem(executionContext, parameters, e);
}
Expand All @@ -633,7 +633,7 @@ protected CompletableFuture<ExecutionResult> completeValueForEnum(ExecutionConte
}

/**
* Called to turn an java object value into an graphql object value
* Called to turn a java object value into an graphql object value
*
* @param executionContext contains the top level execution parameters
* @param parameters contains the parameters holding the fields to be executed and source object
Expand All @@ -649,7 +649,7 @@ protected CompletableFuture<ExecutionResult> completeValueForObject(ExecutionCon
.schema(executionContext.getGraphQLSchema())
.objectType(resolvedObjectType)
.fragments(executionContext.getFragmentsByName())
.variables(executionContext.getVariables())
.variables(executionContext.getCoercedVariables().toMap())
Copy link
Member Author

@bbakerman bbakerman Oct 5, 2022

Choose a reason for hiding this comment

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

Boy scout change - the deprecated method was used

.build();

MergedSelectionSet subFields = fieldCollector.collectFields(collectorParameters, parameters.getField());
Expand Down Expand Up @@ -679,7 +679,6 @@ private Object handleCoercionProblem(ExecutionContext context, ExecutionStrategy
return null;
}


/**
* Converts an object that is known to should be an Iterable into one
*
Expand All @@ -694,6 +693,10 @@ protected Iterable<Object> toIterable(Object result) {
}

protected GraphQLObjectType resolveType(ExecutionContext executionContext, ExecutionStrategyParameters parameters, GraphQLType fieldType) {
// we can avoid a method call and type resolver environment allocation if we know it's an object type
if (fieldType instanceof GraphQLObjectType) {
return (GraphQLObjectType) fieldType;
}
Copy link
Member Author

@bbakerman bbakerman Oct 5, 2022

Choose a reason for hiding this comment

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

we can avoid calling the extra method and inside that the resolver type env allocation if we know its an object type upfront

I originally did this inside esolvedType.resolveType() but why even call it with object types. This is faster

return resolvedType.resolveType(executionContext, parameters.getField(), parameters.getSource(), parameters.getExecutionStepInfo(), fieldType, parameters.getLocalContext());
}

Expand Down Expand Up @@ -743,7 +746,7 @@ protected GraphQLFieldDefinition getFieldDef(GraphQLSchema schema, GraphQLObject
}

/**
* See (http://facebook.github.io/graphql/#sec-Errors-and-Non-Nullability),
* See (<a href="http://facebook.github.io/graphql/#sec-Errors-and-Non-Nullability">...</a>),
Copy link
Member Author

@bbakerman bbakerman Oct 5, 2022

Choose a reason for hiding this comment

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

Boy scout change - warning from IDEA

* <p>
* If a non nullable child field type actually resolves to a null value and the parent type is nullable
* then the parent must in fact become null
Expand Down
14 changes: 5 additions & 9 deletions src/main/java/graphql/execution/ResolveType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package graphql.execution;

import graphql.Assert;
import graphql.Internal;
import graphql.TypeResolutionEnvironment;
import graphql.normalized.ExecutableNormalizedField;
Expand All @@ -19,9 +20,9 @@
@Internal
public class ResolveType {


public GraphQLObjectType resolveType(ExecutionContext executionContext, MergedField field, Object source, ExecutionStepInfo executionStepInfo, GraphQLType fieldType, Object localContext) {
GraphQLObjectType resolvedType;
Assert.assertTrue(fieldType instanceof GraphQLInterfaceType || fieldType instanceof GraphQLUnionType,
() -> "The passed in fieldType MUST be an interface or union type : " + fieldType.getClass().getName());
Copy link
Member Author

@bbakerman bbakerman Oct 5, 2022

Choose a reason for hiding this comment

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

Now only interfaces and unions into this method. The outer code does the object check.

I know its more self contained if perhaps did all three but performance is performance. Not only do we avoid a env object allocation - we avoid a method call as well.

DataFetchingFieldSelectionSet fieldSelectionSet = buildSelectionSet(executionContext, field, (GraphQLOutputType) fieldType, executionStepInfo);
TypeResolutionEnvironment env = TypeResolutionParameters.newParameters()
.field(field)
Expand All @@ -35,13 +36,10 @@ public GraphQLObjectType resolveType(ExecutionContext executionContext, MergedFi
.schema(executionContext.getGraphQLSchema())
.build();
if (fieldType instanceof GraphQLInterfaceType) {
resolvedType = resolveTypeForInterface(env, (GraphQLInterfaceType) fieldType);
} else if (fieldType instanceof GraphQLUnionType) {
resolvedType = resolveTypeForUnion(env, (GraphQLUnionType) fieldType);
return resolveTypeForInterface(env, (GraphQLInterfaceType) fieldType);
} else {
resolvedType = (GraphQLObjectType) fieldType;
return resolveTypeForUnion(env, (GraphQLUnionType) fieldType);
}
return resolvedType;
}

private DataFetchingFieldSelectionSet buildSelectionSet(ExecutionContext executionContext, MergedField field, GraphQLOutputType fieldType, ExecutionStepInfo executionStepInfo) {
Expand All @@ -65,11 +63,9 @@ private GraphQLObjectType resolveAbstractType(TypeResolutionEnvironment env, Typ
if (result == null) {
throw new UnresolvedTypeException(abstractType);
}

if (!env.getSchema().isPossibleType(abstractType, result)) {
throw new UnresolvedTypeException(abstractType, result);
}

return result;
}

Expand Down