Skip to content

This is some performance optimisations for NormalisedOperations #2732

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 4 commits into from
Feb 28, 2022
Merged

This is some performance optimisations for NormalisedOperations #2732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8485d91
Tweaks that make NormalisedOperations run faster in benchmarks
bbakerman Feb 22, 2022
f5bda81
Filter set moved back into main Kit class as immutable
bbakerman Feb 22, 2022
861127a
Improvement to benchmark but no improvements found
bbakerman Feb 22, 2022
d231660
Merge remote-tracking branch 'origin/master' into improved-normalised…
bbakerman Feb 28, 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
4 changes: 4 additions & 0 deletions src/main/java/graphql/execution/ConditionalNodes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ public class ConditionalNodes {
ValuesResolver valuesResolver = new ValuesResolver();

public boolean shouldInclude(Map<String, Object> variables, List<Directive> directives) {
// shortcut on no directives
if (directives.isEmpty()) {
return true;
}
boolean skip = getDirectiveResult(variables, directives, SkipDirective.getName(), false);
if (skip) {
return false;
Expand Down
13 changes: 6 additions & 7 deletions src/main/java/graphql/execution/MergedField.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ public class MergedField {
private final ImmutableList<Field> fields;
private final Field singleField;

private MergedField(List<Field> fields) {
private MergedField(ImmutableList<Field> fields) {
assertNotEmpty(fields);
this.fields = ImmutableList.copyOf(fields);
this.fields = fields;
this.singleField = fields.get(0);
}

Expand All @@ -74,7 +74,7 @@ private MergedField(List<Field> fields) {
*
* WARNING: This is not always the key in the execution result, because of possible aliases. See {@link #getResultKey()}
*
* @return the name of of the merged fields.
* @return the name of the merged fields.
*/
public String getName() {
return singleField.getName();
Expand Down Expand Up @@ -141,14 +141,13 @@ public MergedField transform(Consumer<Builder> builderConsumer) {

public static class Builder {

private final List<Field> fields;
private final ImmutableList.Builder<Field> fields = new ImmutableList.Builder<>();

private Builder() {
this.fields = new ArrayList<>();
}

private Builder(MergedField existing) {
this.fields = new ArrayList<>(existing.getFields());
fields.addAll(existing.getFields());
}

public Builder fields(List<Field> fields) {
Expand All @@ -162,7 +161,7 @@ public Builder addField(Field field) {
}

public MergedField build() {
return new MergedField(fields);
return new MergedField(fields.build());
}

}
Expand Down
14 changes: 6 additions & 8 deletions src/main/java/graphql/execution/ValuesResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,7 @@ public Map<String, NormalizedInputValue> getNormalizedVariableValues(GraphQLSche
public Map<String, Object> getArgumentValues(List<GraphQLArgument> argumentTypes,
List<Argument> arguments,
Map<String, Object> coercedVariables) {
GraphQLCodeRegistry codeRegistry = GraphQLCodeRegistry.newCodeRegistry().fieldVisibility(DEFAULT_FIELD_VISIBILITY).build();
return getArgumentValuesImpl(codeRegistry, argumentTypes, arguments, coercedVariables);
return getArgumentValuesImpl(DEFAULT_FIELD_VISIBILITY, argumentTypes, arguments, coercedVariables);
}
Copy link
Member Author

@bbakerman bbakerman Feb 22, 2022

Choose a reason for hiding this comment

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

We only took the GraphQLCodeRegistry so we could get the field visibility

So we collapsed it so we no longer instantiate an object to get to static


/**
Expand All @@ -162,7 +161,6 @@ public Map<String, Object> getArgumentValues(List<GraphQLArgument> argumentTypes
public Map<String, NormalizedInputValue> getNormalizedArgumentValues(List<GraphQLArgument> argumentTypes,
List<Argument> arguments,
Map<String, NormalizedInputValue> normalizedVariables) {
GraphQLCodeRegistry codeRegistry = GraphQLCodeRegistry.newCodeRegistry().fieldVisibility(DEFAULT_FIELD_VISIBILITY).build();
if (argumentTypes.isEmpty()) {
return Collections.emptyMap();
}
Expand All @@ -182,7 +180,7 @@ public Map<String, NormalizedInputValue> getNormalizedArgumentValues(List<GraphQ
}

GraphQLInputType argumentType = argumentDefinition.getType();
Object value = literalToNormalizedValue(codeRegistry.getFieldVisibility(), argumentType, argument.getValue(), normalizedVariables);
Object value = literalToNormalizedValue(DEFAULT_FIELD_VISIBILITY, argumentType, argument.getValue(), normalizedVariables);
result.put(argumentName, new NormalizedInputValue(simplePrint(argumentType), value));
}
return result;
Expand All @@ -192,7 +190,7 @@ public Map<String, Object> getArgumentValues(GraphQLCodeRegistry codeRegistry,
List<GraphQLArgument> argumentTypes,
List<Argument> arguments,
Map<String, Object> coercedVariables) {
return getArgumentValuesImpl(codeRegistry, argumentTypes, arguments, coercedVariables);
return getArgumentValuesImpl(codeRegistry.getFieldVisibility(), argumentTypes, arguments, coercedVariables);
}

public static Value<?> valueToLiteral(InputValueWithState inputValueWithState, GraphQLType type) {
Expand Down Expand Up @@ -427,7 +425,7 @@ private Map<String, Object> externalValueToInternalValueForVariables(GraphQLSche
}


private Map<String, Object> getArgumentValuesImpl(GraphQLCodeRegistry codeRegistry,
private Map<String, Object> getArgumentValuesImpl(GraphqlFieldVisibility fieldVisibility,
List<GraphQLArgument> argumentTypes,
List<Argument> arguments,
Map<String, Object> coercedVariables
Expand Down Expand Up @@ -455,7 +453,7 @@ private Map<String, Object> getArgumentValuesImpl(GraphQLCodeRegistry codeRegist
}
if (!hasValue && argumentDefinition.hasSetDefaultValue()) {
Object coercedDefaultValue = defaultValueToInternalValue(
codeRegistry.getFieldVisibility(),
fieldVisibility,
defaultValue,
argumentType);
coercedValues.put(argumentName, coercedDefaultValue);
Expand All @@ -467,7 +465,7 @@ private Map<String, Object> getArgumentValuesImpl(GraphQLCodeRegistry codeRegist
} else if (argumentValue instanceof VariableReference) {
coercedValues.put(argumentName, value);
} else {
value = literalToInternalValue(codeRegistry.getFieldVisibility(), argumentType, argument.getValue(), coercedVariables);
value = literalToInternalValue(fieldVisibility, argumentType, argument.getValue(), coercedVariables);
coercedValues.put(argumentName, value);
}
}
Expand Down
57 changes: 30 additions & 27 deletions src/main/java/graphql/normalized/ExecutableNormalizedOperationFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@
import java.util.Collection;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.Predicate;

import static graphql.Assert.assertNotNull;
import static graphql.Assert.assertShouldNeverHappen;
Expand Down Expand Up @@ -235,41 +235,44 @@ public CollectNFResult collectFromMergedField(FieldCollectorNormalizedQueryParam
possibleObjects
);
}
Map<String, List<CollectedField>> fieldsByName = fieldsByResultKey(collectedFields);
ImmutableList.Builder<ExecutableNormalizedField> resultNFs = ImmutableList.builder();
ImmutableListMultimap.Builder<ExecutableNormalizedField, FieldAndAstParent> normalizedFieldToAstFields = ImmutableListMultimap.builder();

createNFs(resultNFs, parameters, fieldsByName, normalizedFieldToAstFields, level, executableNormalizedField);

return new CollectNFResult(resultNFs.build(), normalizedFieldToAstFields.build());
}

private Map<String, List<CollectedField>> fieldsByResultKey(List<CollectedField> collectedFields) {
Map<String, List<CollectedField>> fieldsByName = new LinkedHashMap<>();
for (CollectedField collectedField : collectedFields) {
fieldsByName.computeIfAbsent(collectedField.field.getResultKey(), ignored -> new ArrayList<>()).add(collectedField);
}
List<ExecutableNormalizedField> resultNFs = new ArrayList<>();
ImmutableListMultimap.Builder<ExecutableNormalizedField, FieldAndAstParent> normalizedFieldToAstFields = ImmutableListMultimap.builder();

createNFs(parameters, fieldsByName, resultNFs, normalizedFieldToAstFields, level, executableNormalizedField);

return new CollectNFResult(resultNFs, normalizedFieldToAstFields.build());
return fieldsByName;
Copy link
Member Author

@bbakerman bbakerman Feb 22, 2022

Choose a reason for hiding this comment

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

common code extracted - not performance work per se

}

public CollectNFResult collectFromOperation(FieldCollectorNormalizedQueryParams parameters,
OperationDefinition operationDefinition,
GraphQLObjectType rootType) {
Set<GraphQLObjectType> possibleObjects = new LinkedHashSet<>();
possibleObjects.add(rootType);


Set<GraphQLObjectType> possibleObjects = ImmutableSet.of(rootType);
List<CollectedField> collectedFields = new ArrayList<>();
collectFromSelectionSet(parameters, operationDefinition.getSelectionSet(), collectedFields, rootType, possibleObjects);
// group by result key
Map<String, List<CollectedField>> fieldsByName = new LinkedHashMap<>();
for (CollectedField collectedField : collectedFields) {
fieldsByName.computeIfAbsent(collectedField.field.getResultKey(), ignored -> new ArrayList<>()).add(collectedField);
}
List<ExecutableNormalizedField> resultNFs = new ArrayList<>();
Map<String, List<CollectedField>> fieldsByName = fieldsByResultKey(collectedFields);
ImmutableList.Builder<ExecutableNormalizedField> resultNFs = ImmutableList.builder();
ImmutableListMultimap.Builder<ExecutableNormalizedField, FieldAndAstParent> normalizedFieldToAstFields = ImmutableListMultimap.builder();

createNFs(parameters, fieldsByName, resultNFs, normalizedFieldToAstFields, 1, null);
createNFs(resultNFs, parameters, fieldsByName, normalizedFieldToAstFields, 1, null);

return new CollectNFResult(resultNFs, normalizedFieldToAstFields.build());
return new CollectNFResult(resultNFs.build(), normalizedFieldToAstFields.build());
}

private void createNFs(FieldCollectorNormalizedQueryParams parameters,
private void createNFs(ImmutableList.Builder<ExecutableNormalizedField> nfListBuilder,
FieldCollectorNormalizedQueryParams parameters,
Map<String, List<CollectedField>> fieldsByName,
List<ExecutableNormalizedField> resultNFs,
ImmutableListMultimap.Builder<ExecutableNormalizedField, FieldAndAstParent> normalizedFieldToAstFields,
int level,
ExecutableNormalizedField parent) {
Expand All @@ -284,7 +287,7 @@ private void createNFs(FieldCollectorNormalizedQueryParams parameters,
for (CollectedField collectedField : fieldGroup.fields) {
normalizedFieldToAstFields.put(nf, new FieldAndAstParent(collectedField.field, collectedField.astTypeCondition));
}
resultNFs.add(nf);
nfListBuilder.add(nf);
}
if (commonParentsGroups.size() > 1) {
parameters.addPossibleMergers(parent, resultKey);
Expand All @@ -308,7 +311,8 @@ private ExecutableNormalizedField createNF(FieldCollectorNormalizedQueryParams p
normalizedArgumentValues = valuesResolver.getNormalizedArgumentValues(fieldDefinition.getArguments(), field.getArguments(), parameters.getNormalizedVariableValues());
}
ImmutableList<String> objectTypeNames = map(objectTypes, GraphQLObjectType::getName);
ExecutableNormalizedField executableNormalizedField = ExecutableNormalizedField.newNormalizedField()

return ExecutableNormalizedField.newNormalizedField()
.alias(field.getAlias())
.resolvedArguments(argumentValues)
.normalizedArguments(normalizedArgumentValues)
Expand All @@ -318,8 +322,6 @@ private ExecutableNormalizedField createNF(FieldCollectorNormalizedQueryParams p
.level(level)
.parent(parent)
.build();

return executableNormalizedField;
Copy link
Member Author

@bbakerman bbakerman Feb 22, 2022

Choose a reason for hiding this comment

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

inlined the returned field

}

private static class CollectedFieldGroup {
Expand All @@ -333,20 +335,21 @@ public CollectedFieldGroup(Set<CollectedField> fields, Set<GraphQLObjectType> ob
}

private List<CollectedFieldGroup> groupByCommonParents(Collection<CollectedField> fields) {
Set<GraphQLObjectType> allRelevantObjects = new LinkedHashSet<>();
ImmutableSet.Builder<GraphQLObjectType> objectTypes = ImmutableSet.builder();
for (CollectedField collectedField : fields) {
allRelevantObjects.addAll(collectedField.objectTypes);
objectTypes.addAll(collectedField.objectTypes);
}
Set<GraphQLObjectType> allRelevantObjects = objectTypes.build();
Map<GraphQLType, ImmutableList<CollectedField>> groupByAstParent = groupingBy(fields, fieldAndType -> fieldAndType.astTypeCondition);
if (groupByAstParent.size() == 1) {
return singletonList(new CollectedFieldGroup(new LinkedHashSet<>(fields), allRelevantObjects));
return singletonList(new CollectedFieldGroup(ImmutableSet.copyOf(fields), allRelevantObjects));
}
List<CollectedFieldGroup> result = new ArrayList<>();
ImmutableList.Builder<CollectedFieldGroup> result = ImmutableList.builder();
for (GraphQLObjectType objectType : allRelevantObjects) {
Set<CollectedField> relevantFields = filterSet(fields, field -> field.objectTypes.contains(objectType));
result.add(new CollectedFieldGroup(relevantFields, singleton(objectType)));
}
return result;
return result.build();
}


Expand Down
5 changes: 3 additions & 2 deletions src/main/java/graphql/util/FpKit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@


import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import graphql.Internal;

import java.lang.reflect.Array;
Expand Down Expand Up @@ -279,13 +280,13 @@ public static <T> List<T> filterList(Collection<T> list, Predicate<T> filter) {
}

public static <T> Set<T> filterSet(Collection<T> input, Predicate<T> filter) {
LinkedHashSet<T> result = new LinkedHashSet<>();
ImmutableSet.Builder<T> result = ImmutableSet.builder();
for (T t : input) {
if (filter.test(t)) {
result.add(t);
}
}
return result;
return result.build();
}
Copy link
Member Author

@bbakerman bbakerman Feb 22, 2022

Choose a reason for hiding this comment

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

This is used in 2 places (overlapping fields code as well) and both work as expected


/**
Expand Down
28 changes: 21 additions & 7 deletions src/test/java/benchmark/NQBenchmark.java → src/test/java/benchmark/NQBenchmark1.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Threads;
import org.openjdk.jmh.annotations.Warmup;
import org.openjdk.jmh.infra.Blackhole;

import java.io.IOException;
import java.net.URL;
import java.util.Collections;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;

import static com.google.common.io.Resources.getResource;
Expand All @@ -32,9 +32,9 @@
@BenchmarkMode(Mode.Throughput)
@Warmup(iterations = 2)
@Measurement(iterations = 2, timeUnit = TimeUnit.NANOSECONDS)
public class NQBenchmark {
public class NQBenchmark1 {

@org.openjdk.jmh.annotations.State(Scope.Benchmark)
@State(Scope.Benchmark)
public static class MyState {

GraphQLSchema schema;
Expand Down Expand Up @@ -62,15 +62,29 @@ private String readFromClasspath(String file) throws IOException {

@Benchmark
@Warmup(iterations = 2)
@Measurement(iterations = 100, time = 10)
@Measurement(iterations = 3, time = 10)
@Threads(1)
@Fork(3)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.MILLISECONDS)
public ExecutableNormalizedOperation benchMarkAvgTime(MyState myState) throws ExecutionException, InterruptedException {
public void benchMarkAvgTime(MyState myState, Blackhole blackhole ) {
runImpl(myState, blackhole);
}

@Benchmark
@Warmup(iterations = 2)
@Measurement(iterations = 3, time = 10)
@Threads(1)
@Fork(3)
@BenchmarkMode(Mode.Throughput)
@OutputTimeUnit(TimeUnit.SECONDS)
public void benchMarkThroughput(MyState myState, Blackhole blackhole ) {
runImpl(myState, blackhole);
}

private void runImpl(MyState myState, Blackhole blackhole) {
ExecutableNormalizedOperation executableNormalizedOperation = ExecutableNormalizedOperationFactory.createExecutableNormalizedOperation(myState.schema, myState.document, null, Collections.emptyMap());
// System.out.println("fields size:" + normalizedQuery.getFieldToNormalizedField().size());
return executableNormalizedOperation;
blackhole.consume(executableNormalizedOperation);
}
Copy link
Member Author

@bbakerman bbakerman Feb 22, 2022

Choose a reason for hiding this comment

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

Added throughput



Expand Down
12 changes: 10 additions & 2 deletions src/test/java/benchmark/OverlappingFieldValidationBenchmark.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Threads;
import org.openjdk.jmh.annotations.Warmup;
import org.openjdk.jmh.infra.Blackhole;

import java.io.IOException;
import java.net.URL;
Expand Down Expand Up @@ -75,8 +76,15 @@ private String readFromClasspath(String file) throws IOException {
}

@Benchmark
public List<ValidationError> overlappingFieldValidation(MyState myState) {
return validateQuery(myState.schema, myState.document);
public void overlappingFieldValidationAbgTime(MyState myState, Blackhole blackhole) {
blackhole.consume(validateQuery(myState.schema, myState.document));
}

@Benchmark
@BenchmarkMode(Mode.Throughput)
@OutputTimeUnit(TimeUnit.SECONDS)
public void overlappingFieldValidationThroughput(MyState myState, Blackhole blackhole) {
blackhole.consume(validateQuery(myState.schema, myState.document));
Copy link
Member Author

@bbakerman bbakerman Feb 22, 2022

Choose a reason for hiding this comment

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

Added throughput

I tried to find some perf improvements in Overlapping fields code but none to be easily found

}


Expand Down