Skip to content

Commit 7a4e1fb

Browse files
andimarekclaude
andimarek
and
claude
committed
Optimize validation hot paths to reduce collection overhead
Replace HashMap/LinkedHashMap allocations with linear scans for small argument lists, single-pass partitioning in groupByCommonParents, pre-sized maps, computeIfAbsent, HashSet for fragment tracking, and ArrayList over LinkedList. Short-circuit isRuleEnabled when all rules are enabled. ~12% improvement on ValidatorBenchmark (largeSchema4: 7.86→6.94ms, manyFragments: 5.83→5.16ms). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 23dd3db commit 7a4e1fb

1 file changed

Lines changed: 69 additions & 39 deletions

File tree

‎src/main/java/graphql/validation/OperationValidator.java‎

Lines changed: 69 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@
6969
import java.util.HashSet;
7070
import java.util.LinkedHashMap;
7171
import java.util.LinkedHashSet;
72-
import java.util.LinkedList;
72+
7373
import java.util.List;
7474
import java.util.Map;
7575
import java.util.Objects;
@@ -90,8 +90,7 @@
9090
import static graphql.schema.GraphQLTypeUtil.simplePrint;
9191
import static graphql.schema.GraphQLTypeUtil.unwrapAll;
9292
import static graphql.schema.GraphQLTypeUtil.unwrapOne;
93-
import static graphql.util.FpKit.filterSet;
94-
import static graphql.util.FpKit.groupingBy;
93+
9594
import static graphql.validation.ValidationError.newValidationError;
9695
import static graphql.validation.ValidationErrorType.BadValueForDefaultArg;
9796
import static graphql.validation.ValidationErrorType.DuplicateArgumentNames;
@@ -328,16 +327,28 @@ public class OperationValidator implements DocumentVisitor {
328327
// fragmentRetraversalDepth == 0 means we're NOT inside a manually-traversed fragment => run non-fragment-spread checks
329328
// operationScope means we're inside an operation => can trigger fragment traversal
330329

330+
private final boolean allRulesEnabled;
331+
331332
public OperationValidator(ValidationContext validationContext, ValidationErrorCollector errorCollector, Predicate<OperationValidationRule> rulePredicate) {
332333
this.validationContext = validationContext;
333334
this.errorCollector = errorCollector;
334335
this.validationUtil = new ValidationUtil();
335336
this.rulePredicate = rulePredicate;
337+
this.allRulesEnabled = detectAllRulesEnabled(rulePredicate);
336338
prepareFragmentSpreadsMap();
337339
}
338340

341+
private static boolean detectAllRulesEnabled(Predicate<OperationValidationRule> predicate) {
342+
for (OperationValidationRule rule : OperationValidationRule.values()) {
343+
if (!predicate.test(rule)) {
344+
return false;
345+
}
346+
}
347+
return true;
348+
}
349+
339350
private boolean isRuleEnabled(OperationValidationRule rule) {
340-
return rulePredicate.test(rule);
351+
return allRulesEnabled || rulePredicate.test(rule);
341352
}
342353

343354
/**
@@ -909,8 +920,8 @@ public void leave(Node node, List<Node> path) {
909920
}
910921

911922
private void validateNoFragmentCycles(FragmentDefinition fragmentDefinition) {
912-
LinkedList<String> path = new LinkedList<>();
913-
path.add(0, fragmentDefinition.getName());
923+
ArrayList<String> path = new ArrayList<>();
924+
path.add(fragmentDefinition.getName());
914925
Map<String, Set<String>> transitiveSpreads = buildTransitiveSpreads(path, new HashMap<>());
915926

916927
for (Map.Entry<String, Set<String>> entry : transitiveSpreads.entrySet()) {
@@ -921,8 +932,8 @@ private void validateNoFragmentCycles(FragmentDefinition fragmentDefinition) {
921932
}
922933
}
923934

924-
private Map<String, Set<String>> buildTransitiveSpreads(LinkedList<String> path, Map<String, Set<String>> transitiveSpreads) {
925-
String name = path.peekFirst();
935+
private Map<String, Set<String>> buildTransitiveSpreads(ArrayList<String> path, Map<String, Set<String>> transitiveSpreads) {
936+
String name = path.get(path.size() - 1);
926937
if (transitiveSpreads.containsKey(name)) {
927938
return transitiveSpreads;
928939
}
@@ -942,8 +953,8 @@ private Map<String, Set<String>> buildTransitiveSpreads(LinkedList<String> path,
942953
if (path.contains(child) || transitiveSpreads.containsKey(child)) {
943954
continue;
944955
}
945-
LinkedList<String> childPath = new LinkedList<>(path);
946-
childPath.add(0, child);
956+
ArrayList<String> childPath = new ArrayList<>(path);
957+
childPath.add(child);
947958
buildTransitiveSpreads(childPath, transitiveSpreads);
948959
}
949960
return transitiveSpreads;
@@ -959,7 +970,7 @@ private void validateNoUndefinedVariables(VariableReference variableReference) {
959970

960971
// --- NoUnusedFragments ---
961972
private void validateNoUnusedFragments() {
962-
List<String> allUsedFragments = new ArrayList<>();
973+
Set<String> allUsedFragments = new HashSet<>();
963974
for (List<String> fragmentsInOneOperation : fragmentsUsedDirectlyInOperation) {
964975
for (String fragment : fragmentsInOneOperation) {
965976
collectUsedFragmentsInDefinition(allUsedFragments, fragment);
@@ -973,9 +984,8 @@ private void validateNoUnusedFragments() {
973984
}
974985
}
975986

976-
private void collectUsedFragmentsInDefinition(List<String> result, String fragmentName) {
977-
if (result.contains(fragmentName)) return;
978-
result.add(fragmentName);
987+
private void collectUsedFragmentsInDefinition(Set<String> result, String fragmentName) {
988+
if (!result.add(fragmentName)) return;
979989
List<String> spreadList = spreadsInDefinition.get(fragmentName);
980990
if (spreadList == null) {
981991
return;
@@ -991,7 +1001,7 @@ private void validateOverlappingFieldsCanBeMerged(OperationDefinition operationD
9911001
}
9921002

9931003
private void overlappingFieldsImpl(SelectionSet selectionSet, @Nullable GraphQLOutputType graphQLOutputType) {
994-
Map<String, Set<FieldAndType>> fieldMap = new LinkedHashMap<>();
1004+
Map<String, Set<FieldAndType>> fieldMap = new LinkedHashMap<>(selectionSet.getSelections().size());
9951005
Set<String> visitedFragments = new LinkedHashSet<>();
9961006
overlappingFields_collectFields(fieldMap, selectionSet, graphQLOutputType, visitedFragments);
9971007
List<Conflict> conflicts = findConflicts(fieldMap);
@@ -1037,9 +1047,6 @@ private void overlappingFields_collectFieldsForInlineFragment(Map<String, Set<Fi
10371047

10381048
private void overlappingFields_collectFieldsForField(Map<String, Set<FieldAndType>> fieldMap, @Nullable GraphQLType parentType, Field field) {
10391049
String responseName = field.getResultKey();
1040-
if (!fieldMap.containsKey(responseName)) {
1041-
fieldMap.put(responseName, new LinkedHashSet<>());
1042-
}
10431050
GraphQLOutputType fieldType = null;
10441051
GraphQLUnmodifiedType unwrappedParent = parentType != null ? unwrapAll(parentType) : null;
10451052
if (unwrappedParent instanceof GraphQLFieldsContainer) {
@@ -1050,7 +1057,7 @@ private void overlappingFields_collectFieldsForField(Map<String, Set<FieldAndTyp
10501057
fieldType = fieldDefinition != null ? fieldDefinition.getType() : null;
10511058
}
10521059
}
1053-
fieldMap.get(responseName).add(new FieldAndType(field, fieldType, unwrappedParent));
1060+
fieldMap.computeIfAbsent(responseName, k -> new LinkedHashSet<>()).add(new FieldAndType(field, fieldType, unwrappedParent));
10541061
}
10551062

10561063
private List<Conflict> findConflicts(Map<String, Set<FieldAndType>> fieldMap) {
@@ -1109,17 +1116,38 @@ private void sameForCommonParentsByName(Map<String, Set<FieldAndType>> fieldMap,
11091116
}
11101117

11111118
private List<Set<FieldAndType>> groupByCommonParents(Set<FieldAndType> fields) {
1112-
Set<FieldAndType> abstractTypes = filterSet(fields, fieldAndType -> isInterfaceOrUnion(fieldAndType.parentType));
1113-
Set<FieldAndType> concreteTypes = filterSet(fields, fieldAndType -> fieldAndType.parentType instanceof GraphQLObjectType);
1114-
if (concreteTypes.isEmpty()) {
1115-
return Collections.singletonList(abstractTypes);
1116-
}
1117-
Map<GraphQLType, ImmutableList<FieldAndType>> groupsByConcreteParent = groupingBy(concreteTypes, fieldAndType -> fieldAndType.parentType);
1118-
List<Set<FieldAndType>> result = new ArrayList<>();
1119-
for (ImmutableList<FieldAndType> concreteGroup : groupsByConcreteParent.values()) {
1120-
Set<FieldAndType> oneResultGroup = new LinkedHashSet<>(concreteGroup);
1121-
oneResultGroup.addAll(abstractTypes);
1122-
result.add(oneResultGroup);
1119+
// Single-pass: partition into abstract types and concrete groups simultaneously
1120+
List<FieldAndType> abstractTypes = null;
1121+
Map<GraphQLType, Set<FieldAndType>> concreteGroups = null;
1122+
1123+
for (FieldAndType fieldAndType : fields) {
1124+
if (isInterfaceOrUnion(fieldAndType.parentType)) {
1125+
if (abstractTypes == null) {
1126+
abstractTypes = new ArrayList<>();
1127+
}
1128+
abstractTypes.add(fieldAndType);
1129+
} else if (fieldAndType.parentType instanceof GraphQLObjectType) {
1130+
if (concreteGroups == null) {
1131+
concreteGroups = new LinkedHashMap<>();
1132+
}
1133+
concreteGroups.computeIfAbsent(fieldAndType.parentType, k -> new LinkedHashSet<>()).add(fieldAndType);
1134+
}
1135+
}
1136+
1137+
if (concreteGroups == null || concreteGroups.isEmpty()) {
1138+
// No concrete types — return all abstract types as a single group
1139+
if (abstractTypes == null) {
1140+
return Collections.singletonList(fields);
1141+
}
1142+
return Collections.singletonList(new LinkedHashSet<>(abstractTypes));
1143+
}
1144+
1145+
List<Set<FieldAndType>> result = new ArrayList<>(concreteGroups.size());
1146+
for (Set<FieldAndType> concreteGroup : concreteGroups.values()) {
1147+
if (abstractTypes != null) {
1148+
concreteGroup.addAll(abstractTypes);
1149+
}
1150+
result.add(concreteGroup);
11231151
}
11241152
return result;
11251153
}
@@ -1349,10 +1377,10 @@ private boolean isValidTargetCompositeType(GraphQLType type) {
13491377
private void validateProvidedNonNullArguments_field(Field field) {
13501378
GraphQLFieldDefinition fieldDef = validationContext.getFieldDef();
13511379
if (fieldDef == null) return;
1352-
Map<String, Argument> argumentMap = argumentMap(field.getArguments());
1380+
List<Argument> providedArguments = field.getArguments();
13531381

13541382
for (GraphQLArgument graphQLArgument : fieldDef.getArguments()) {
1355-
Argument argument = argumentMap.get(graphQLArgument.getName());
1383+
Argument argument = findArgumentByName(providedArguments, graphQLArgument.getName());
13561384
boolean nonNullType = isNonNull(graphQLArgument.getType());
13571385
boolean noDefaultValue = graphQLArgument.getArgumentDefaultValue().isNotSet();
13581386
if (argument == null && nonNullType && noDefaultValue) {
@@ -1372,10 +1400,10 @@ private void validateProvidedNonNullArguments_field(Field field) {
13721400
private void validateProvidedNonNullArguments_directive(Directive directive) {
13731401
GraphQLDirective graphQLDirective = validationContext.getDirective();
13741402
if (graphQLDirective == null) return;
1375-
Map<String, Argument> argumentMap = argumentMap(directive.getArguments());
1403+
List<Argument> providedArguments = directive.getArguments();
13761404

13771405
for (GraphQLArgument graphQLArgument : graphQLDirective.getArguments()) {
1378-
Argument argument = argumentMap.get(graphQLArgument.getName());
1406+
Argument argument = findArgumentByName(providedArguments, graphQLArgument.getName());
13791407
boolean nonNullType = isNonNull(graphQLArgument.getType());
13801408
boolean noDefaultValue = graphQLArgument.getArgumentDefaultValue().isNotSet();
13811409
if (argument == null && nonNullType && noDefaultValue) {
@@ -1385,12 +1413,14 @@ private void validateProvidedNonNullArguments_directive(Directive directive) {
13851413
}
13861414
}
13871415

1388-
private Map<String, Argument> argumentMap(List<Argument> arguments) {
1389-
Map<String, Argument> result = new LinkedHashMap<>();
1390-
for (Argument argument : arguments) {
1391-
result.put(argument.getName(), argument);
1416+
private static @Nullable Argument findArgumentByName(List<Argument> arguments, String name) {
1417+
for (int i = 0; i < arguments.size(); i++) {
1418+
Argument argument = arguments.get(i);
1419+
if (argument.getName().equals(name)) {
1420+
return argument;
1421+
}
13921422
}
1393-
return result;
1423+
return null;
13941424
}
13951425

13961426
// --- ScalarLeaves ---

0 commit comments

Comments
 (0)