Avoid allocating a type resolve env if the type is already an object type #2980
Conversation
| try { | ||
| return asyncHandleException(dataFetcherExceptionHandler, handlerParameters, executionContext); | ||
| return asyncHandleException(dataFetcherExceptionHandler, handlerParameters); | ||
| } catch (Exception handlerException) { |
There was a problem hiding this comment.
Boy scout change - the executionContext was never used
| .exception(handlerException) | ||
| .build(); | ||
| return asyncHandleException(new SimpleDataFetcherExceptionHandler(), handlerParameters, executionContext); | ||
| return asyncHandleException(new SimpleDataFetcherExceptionHandler(), handlerParameters); |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Boy scout change - the executionContext was never used
| Object serialized; | ||
| try { | ||
| serialized = enumType.serialize(result); | ||
| serialized = enumType.serialize(result, executionContext.getGraphQLContext(), executionContext.getLocale()); |
There was a problem hiding this comment.
Boy scout change - the deprecated method should not have been using this mechanism
| .objectType(resolvedObjectType) | ||
| .fragments(executionContext.getFragmentsByName()) | ||
| .variables(executionContext.getVariables()) | ||
| .variables(executionContext.getCoercedVariables().toMap()) |
There was a problem hiding this comment.
Boy scout change - the deprecated method was used
…type - restored protected method
| // 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; | ||
| } |
There was a problem hiding this comment.
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
| | ||
| /** | ||
| * 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>), |
There was a problem hiding this comment.
Boy scout change - warning from IDEA
| GraphQLObjectType resolvedType; | ||
| Assert.assertTrue(fieldType instanceof GraphQLInterfaceType || fieldType instanceof GraphQLUnionType, | ||
| () -> "The passed in fieldType MUST be an interface or union type : " + fieldType.getClass().getName()); | ||
| TypeResolutionEnvironment env = buildEnv(executionContext, field, source, executionStepInfo, fieldType, localContext); |
There was a problem hiding this comment.
We now ONLY take unions and interfaces. Since the object type check is outside now. This class is NOT general purpose and ONLY used above
…type - restored direct env allocation code
| 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()); |
There was a problem hiding this comment.
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.
This will improve the runtime for every object type encountered since the type env allocation (and indeed method call) is not needed