From 808d7bc24a1ebbbea04b40c48b87b551cd73e132 Mon Sep 17 00:00:00 2001 From: Danny Thomas Date: Fri, 1 Mar 2024 16:52:58 +1100 Subject: [PATCH] Avoid repeated calls to getFieldDef and unnecessary immutable builders/collections (#3478) * Avoid repeated calls to getFieldDef and unnecessary immutable builders/collections * Remove unnecessary allocations in builders --- .../graphql/execution/ExecutionStrategy.java | 18 +++++++++++++++--- .../ExecutionStrategyParameters.java | 5 ++++- .../graphql/execution/FieldCollector.java | 5 +---- .../graphql/execution/FieldValueInfo.java | 3 ++- .../java/graphql/execution/MergedField.java | 11 +++++++++-- .../graphql/execution/MergedSelectionSet.java | 12 +++++++----- .../graphql/schema/GraphQLInterfaceType.java | 19 +++++++------------ .../graphql/schema/GraphQLObjectType.java | 8 ++++---- src/test/java/benchmark/TwitterBenchmark.java | 16 +++++++++++++--- 9 files changed, 62 insertions(+), 35 deletions(-) diff --git a/src/main/java/graphql/execution/ExecutionStrategy.java b/src/main/java/graphql/execution/ExecutionStrategy.java index 0d79d326b8..8d09d1ebcd 100644 --- a/src/main/java/graphql/execution/ExecutionStrategy.java +++ b/src/main/java/graphql/execution/ExecutionStrategy.java @@ -348,9 +348,9 @@ protected CompletableFuture resolveFieldWithInfo(ExecutionContex new InstrumentationFieldParameters(executionContext, executionStepInfo), executionContext.getInstrumentationState() )); - CompletableFuture fetchFieldFuture = fetchField(executionContext, parameters); + CompletableFuture fetchFieldFuture = fetchField(fieldDef, executionContext, parameters); CompletableFuture result = fetchFieldFuture.thenApply((fetchedValue) -> - completeField(executionContext, parameters, fetchedValue)); + completeField(fieldDef, executionContext, parameters, fetchedValue)); CompletableFuture fieldValueFuture = result.thenCompose(FieldValueInfo::getFieldValueFuture); @@ -377,7 +377,12 @@ protected CompletableFuture fetchField(ExecutionContext executionC MergedField field = parameters.getField(); GraphQLObjectType parentType = (GraphQLObjectType) parameters.getExecutionStepInfo().getUnwrappedNonNullType(); GraphQLFieldDefinition fieldDef = getFieldDef(executionContext.getGraphQLSchema(), parentType, field.getSingleField()); - GraphQLCodeRegistry codeRegistry = executionContext.getGraphQLSchema().getCodeRegistry(); + return fetchField(fieldDef, executionContext, parameters); + } + + private CompletableFuture fetchField(GraphQLFieldDefinition fieldDef, ExecutionContext executionContext, ExecutionStrategyParameters parameters) { + MergedField field = parameters.getField(); + GraphQLObjectType parentType = (GraphQLObjectType) parameters.getExecutionStepInfo().getUnwrappedNonNullType(); // if the DF (like PropertyDataFetcher) does not use the arguments or execution step info then dont build any @@ -412,6 +417,8 @@ protected CompletableFuture fetchField(ExecutionContext executionC .queryDirectives(queryDirectives) .build(); }); + + GraphQLCodeRegistry codeRegistry = executionContext.getGraphQLSchema().getCodeRegistry(); DataFetcher dataFetcher = codeRegistry.getDataFetcher(parentType, fieldDef); Instrumentation instrumentation = executionContext.getInstrumentation(); @@ -555,6 +562,11 @@ protected FieldValueInfo completeField(ExecutionContext executionContext, Execut Field field = parameters.getField().getSingleField(); GraphQLObjectType parentType = (GraphQLObjectType) parameters.getExecutionStepInfo().getUnwrappedNonNullType(); GraphQLFieldDefinition fieldDef = getFieldDef(executionContext.getGraphQLSchema(), parentType, field); + return completeField(fieldDef, executionContext, parameters, fetchedValue); + } + + private FieldValueInfo completeField(GraphQLFieldDefinition fieldDef, ExecutionContext executionContext, ExecutionStrategyParameters parameters, FetchedValue fetchedValue) { + GraphQLObjectType parentType = (GraphQLObjectType) parameters.getExecutionStepInfo().getUnwrappedNonNullType(); ExecutionStepInfo executionStepInfo = createExecutionStepInfo(executionContext, parameters, fieldDef, parentType); Instrumentation instrumentation = executionContext.getInstrumentation(); diff --git a/src/main/java/graphql/execution/ExecutionStrategyParameters.java b/src/main/java/graphql/execution/ExecutionStrategyParameters.java index ee0cdce42e..b40fa781c5 100644 --- a/src/main/java/graphql/execution/ExecutionStrategyParameters.java +++ b/src/main/java/graphql/execution/ExecutionStrategyParameters.java @@ -114,7 +114,7 @@ public static class Builder { ResultPath path = ResultPath.rootPath(); MergedField currentField; ExecutionStrategyParameters parent; - DeferredCallContext deferredCallContext = new DeferredCallContext(); + DeferredCallContext deferredCallContext; /** * @see ExecutionStrategyParameters#newParameters() @@ -188,6 +188,9 @@ public Builder deferredCallContext(DeferredCallContext deferredCallContext) { } public ExecutionStrategyParameters build() { + if (deferredCallContext == null) { + deferredCallContext = new DeferredCallContext(); + } return new ExecutionStrategyParameters(executionStepInfo, source, localContext, fields, nonNullableFieldValidator, path, currentField, parent, deferredCallContext); } } diff --git a/src/main/java/graphql/execution/FieldCollector.java b/src/main/java/graphql/execution/FieldCollector.java index 674819407d..93d58f97ab 100644 --- a/src/main/java/graphql/execution/FieldCollector.java +++ b/src/main/java/graphql/execution/FieldCollector.java @@ -148,10 +148,7 @@ private void collectField(FieldCollectorParameters parameters, Map fieldValueFuture; - private List listInfos = new ArrayList<>(); + private List listInfos = ImmutableList.of(); public Builder(CompleteValueType completeValueType) { this.completeValueType = completeValueType; diff --git a/src/main/java/graphql/execution/MergedField.java b/src/main/java/graphql/execution/MergedField.java index c308b0cc26..e66afb63f8 100644 --- a/src/main/java/graphql/execution/MergedField.java +++ b/src/main/java/graphql/execution/MergedField.java @@ -73,6 +73,10 @@ private MergedField(ImmutableList fields, ImmutableList getFields() { * @return all defer executions. */ @ExperimentalApi - public ImmutableList getDeferredExecutions() { + public List getDeferredExecutions() { return deferredExecutions; } - public static Builder newMergedField() { return new Builder(); } @@ -148,6 +151,10 @@ public static Builder newMergedField(List fields) { return new Builder().fields(fields); } + static MergedField newSingletonMergedField(Field field, DeferredExecution deferredExecution) { + return new MergedField(field, deferredExecution); + } + public MergedField transform(Consumer builderConsumer) { Builder builder = new Builder(this); builderConsumer.accept(builder); diff --git a/src/main/java/graphql/execution/MergedSelectionSet.java b/src/main/java/graphql/execution/MergedSelectionSet.java index a806af8555..8f279f05a6 100644 --- a/src/main/java/graphql/execution/MergedSelectionSet.java +++ b/src/main/java/graphql/execution/MergedSelectionSet.java @@ -13,10 +13,12 @@ @PublicApi public class MergedSelectionSet { - private final ImmutableMap subFields; + private final Map subFields; + private final List keys; protected MergedSelectionSet(Map subFields) { - this.subFields = ImmutableMap.copyOf(Assert.assertNotNull(subFields)); + this.subFields = subFields == null ? ImmutableMap.of() : subFields; + this.keys = ImmutableList.copyOf(this.subFields.keySet()); } public Map getSubFields() { @@ -40,7 +42,7 @@ public MergedField getSubField(String key) { } public List getKeys() { - return ImmutableList.copyOf(keySet()); + return keys; } public boolean isEmpty() { @@ -52,10 +54,10 @@ public static Builder newMergedSelectionSet() { } public static class Builder { - private Map subFields = ImmutableMap.of(); - private Builder() { + private Map subFields; + private Builder() { } public Builder subFields(Map subFields) { diff --git a/src/main/java/graphql/schema/GraphQLInterfaceType.java b/src/main/java/graphql/schema/GraphQLInterfaceType.java index 238b5abdd3..e0159a5ee0 100644 --- a/src/main/java/graphql/schema/GraphQLInterfaceType.java +++ b/src/main/java/graphql/schema/GraphQLInterfaceType.java @@ -2,12 +2,12 @@ import com.google.common.collect.ImmutableList; import graphql.Assert; -import graphql.AssertException; import graphql.DirectivesUtil; import graphql.Internal; import graphql.PublicApi; import graphql.language.InterfaceTypeDefinition; import graphql.language.InterfaceTypeExtensionDefinition; +import graphql.util.FpKit; import graphql.util.TraversalControl; import graphql.util.TraverserContext; @@ -20,12 +20,12 @@ import java.util.function.UnaryOperator; import static graphql.Assert.assertNotNull; +import static graphql.Assert.assertShouldNeverHappen; import static graphql.Assert.assertValidName; import static graphql.collect.ImmutableKit.emptyList; import static graphql.schema.GraphqlTypeComparators.sortTypes; import static graphql.util.FpKit.getByName; import static graphql.util.FpKit.valuesToList; -import static java.lang.String.format; /** * In graphql, an interface is an abstract type that defines the set of fields that a type must include to @@ -41,7 +41,7 @@ public class GraphQLInterfaceType implements GraphQLNamedType, GraphQLCompositeT private final String name; private final String description; - private final Map fieldDefinitionsByName = new LinkedHashMap<>(); + private final Map fieldDefinitionsByName; private final TypeResolver typeResolver; private final InterfaceTypeDefinition definition; private final ImmutableList extensionDefinitions; @@ -78,17 +78,12 @@ private GraphQLInterfaceType(String name, this.originalInterfaces = ImmutableList.copyOf(sortTypes(interfaceComparator, interfaces)); this.extensionDefinitions = ImmutableList.copyOf(extensionDefinitions); this.directivesHolder = new DirectivesUtil.DirectivesHolder(directives, appliedDirectives); - buildDefinitionMap(fieldDefinitions); + this.fieldDefinitionsByName = buildDefinitionMap(fieldDefinitions); } - private void buildDefinitionMap(List fieldDefinitions) { - for (GraphQLFieldDefinition fieldDefinition : fieldDefinitions) { - String name = fieldDefinition.getName(); - if (fieldDefinitionsByName.containsKey(name)) { - throw new AssertException(format("Duplicated definition for field '%s' in interface '%s'", name, this.name)); - } - fieldDefinitionsByName.put(name, fieldDefinition); - } + private Map buildDefinitionMap(List fieldDefinitions) { + return FpKit.getByName(fieldDefinitions, GraphQLFieldDefinition::getName, + (fld1, fld2) -> assertShouldNeverHappen("Duplicated definition for field '%s' in interface '%s'", fld1.getName(), this.name)); } @Override diff --git a/src/main/java/graphql/schema/GraphQLObjectType.java b/src/main/java/graphql/schema/GraphQLObjectType.java index 732c0ff753..b687215a68 100644 --- a/src/main/java/graphql/schema/GraphQLObjectType.java +++ b/src/main/java/graphql/schema/GraphQLObjectType.java @@ -44,7 +44,7 @@ public class GraphQLObjectType implements GraphQLNamedOutputType, GraphQLComposi private final String name; private final String description; private final Comparator interfaceComparator; - private final ImmutableMap fieldDefinitionsByName; + private final Map fieldDefinitionsByName; private final ImmutableList originalInterfaces; private final DirectivesUtil.DirectivesHolder directivesHolder; private final ObjectTypeDefinition definition; @@ -82,9 +82,9 @@ void replaceInterfaces(List interfaces) { this.replacedInterfaces = ImmutableList.copyOf(sortTypes(interfaceComparator, interfaces)); } - private ImmutableMap buildDefinitionMap(List fieldDefinitions) { - return ImmutableMap.copyOf(FpKit.getByName(fieldDefinitions, GraphQLFieldDefinition::getName, - (fld1, fld2) -> assertShouldNeverHappen("Duplicated definition for field '%s' in type '%s'", fld1.getName(), this.name))); + private Map buildDefinitionMap(List fieldDefinitions) { + return FpKit.getByName(fieldDefinitions, GraphQLFieldDefinition::getName, + (fld1, fld2) -> assertShouldNeverHappen("Duplicated definition for field '%s' in type '%s'", fld1.getName(), this.name)); } @Override diff --git a/src/test/java/benchmark/TwitterBenchmark.java b/src/test/java/benchmark/TwitterBenchmark.java index c32753b7b5..b3d9e34dba 100644 --- a/src/test/java/benchmark/TwitterBenchmark.java +++ b/src/test/java/benchmark/TwitterBenchmark.java @@ -21,6 +21,10 @@ import org.openjdk.jmh.annotations.OutputTimeUnit; import org.openjdk.jmh.annotations.Warmup; import org.openjdk.jmh.infra.Blackhole; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.Options; +import org.openjdk.jmh.runner.options.OptionsBuilder; import java.util.ArrayList; import java.util.List; @@ -128,8 +132,14 @@ protected Optional getPersistedQueryId(ExecutionInput executionInput) { } } - public static void main(String[] args) { - ExecutionResult result = execute(); - System.out.println(result); + public static void main(String[] args) throws RunnerException { + Options opt = new OptionsBuilder() + .include("benchmark.TwitterBenchmark") + .forks(1) + .warmupIterations(5) + .measurementIterations(10) + .build(); + + new Runner(opt).run(); } }