Skip to content

Commit 1007fca

Browse files
bbakermanbbakerman
authored
Some benchmarking and improvements to schema creation times and object allocations (graphql-java#2453)
* Some benchmarking and improvements to schema creation times and object allocations * formatting * large-schema-rocketraman.graphqls added * SchemaTransformer can do no work if nothing changes * === usage
1 parent ae7131e commit 1007fca

8 files changed

Lines changed: 725 additions & 39 deletions

‎src/main/java/graphql/schema/GraphQLCodeRegistry.java‎

Lines changed: 56 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package graphql.schema;
22

33
import graphql.Assert;
4+
import graphql.Internal;
45
import graphql.PublicApi;
56
import graphql.schema.visibility.GraphqlFieldVisibility;
67

@@ -189,7 +190,7 @@ public static class Builder {
189190
private final Map<String, TypeResolver> typeResolverMap = new HashMap<>();
190191
private GraphqlFieldVisibility fieldVisibility = DEFAULT_FIELD_VISIBILITY;
191192
private DataFetcherFactory<?> defaultDataFetcherFactory = env -> PropertyDataFetcher.fetching(env.getFieldDefinition().getName());
192-
193+
private boolean changed = false;
193194

194195
private Builder() {
195196
}
@@ -202,6 +203,38 @@ private Builder(GraphQLCodeRegistry codeRegistry) {
202203
this.defaultDataFetcherFactory = codeRegistry.defaultDataFetcherFactory;
203204
}
204205

206+
/**
207+
* A helper method to track if the builder changes from the point
208+
* at which this method was called.
209+
*
210+
* @return this builder for fluent code
211+
*/
212+
@Internal
213+
public Builder trackChanges() {
214+
changed = false;
215+
return this;
216+
}
217+
218+
/**
219+
* @return true if the builder has changed since {@link #trackChanges()} was called
220+
*/
221+
@Internal
222+
public boolean hasChanged() {
223+
return changed;
224+
}
225+
226+
private Builder markChanged() {
227+
changed = true;
228+
return this;
229+
}
230+
231+
private Builder markChanged(boolean condition) {
232+
if (condition) {
233+
changed = true;
234+
}
235+
return this;
236+
}
237+
205238
/**
206239
* Returns a data fetcher associated with a field within a container type
207240
*
@@ -309,7 +342,7 @@ public Builder systemDataFetcher(FieldCoordinates coordinates, DataFetcher<?> da
309342
assertNotNull(coordinates);
310343
coordinates.assertValidNames();
311344
systemDataFetcherMap.put(coordinates.getFieldName(), DataFetcherFactories.useDataFetcher(dataFetcher));
312-
return this;
345+
return markChanged();
313346
}
314347

315348
/**
@@ -329,7 +362,7 @@ public Builder dataFetcher(FieldCoordinates coordinates, DataFetcherFactory<?> d
329362
} else {
330363
dataFetcherMap.put(coordinates, dataFetcherFactory);
331364
}
332-
return this;
365+
return markChanged();
333366
}
334367

335368
/**
@@ -347,6 +380,7 @@ public Builder dataFetcherIfAbsent(FieldCoordinates coordinates, DataFetcher<?>
347380
} else {
348381
dataFetcher(coordinates, dataFetcher);
349382
}
383+
return markChanged();
350384
}
351385
return this;
352386
}
@@ -362,7 +396,7 @@ public Builder dataFetcherIfAbsent(FieldCoordinates coordinates, DataFetcher<?>
362396
public Builder dataFetchers(String parentTypeName, Map<String, DataFetcher<?>> fieldDataFetchers) {
363397
assertNotNull(fieldDataFetchers);
364398
fieldDataFetchers.forEach((fieldName, dataFetcher) -> dataFetcher(coordinates(parentTypeName, fieldName), dataFetcher));
365-
return this;
399+
return markChanged(!fieldDataFetchers.isEmpty());
366400
}
367401

368402
/**
@@ -375,57 +409,63 @@ public Builder dataFetchers(String parentTypeName, Map<String, DataFetcher<?>> f
375409
*/
376410
public Builder defaultDataFetcher(DataFetcherFactory<?> defaultDataFetcherFactory) {
377411
this.defaultDataFetcherFactory = Assert.assertNotNull(defaultDataFetcherFactory);
378-
return this;
412+
return markChanged();
379413
}
380414

381415
public Builder dataFetchers(GraphQLCodeRegistry codeRegistry) {
382416
this.dataFetcherMap.putAll(codeRegistry.dataFetcherMap);
383-
return this;
417+
return markChanged(!codeRegistry.dataFetcherMap.isEmpty());
384418
}
385419

386420
public Builder typeResolver(GraphQLInterfaceType interfaceType, TypeResolver typeResolver) {
387421
typeResolverMap.put(interfaceType.getName(), typeResolver);
388-
return this;
422+
return markChanged();
389423
}
390424

391425
public Builder typeResolverIfAbsent(GraphQLInterfaceType interfaceType, TypeResolver typeResolver) {
392-
typeResolverMap.putIfAbsent(interfaceType.getName(), typeResolver);
426+
if (!typeResolverMap.containsKey(interfaceType.getName())) {
427+
typeResolverMap.put(interfaceType.getName(), typeResolver);
428+
return markChanged();
429+
}
393430
return this;
394431
}
395432

396433
public Builder typeResolver(GraphQLUnionType unionType, TypeResolver typeResolver) {
397434
typeResolverMap.put(unionType.getName(), typeResolver);
398-
return this;
435+
return markChanged();
399436
}
400437

401438
public Builder typeResolverIfAbsent(GraphQLUnionType unionType, TypeResolver typeResolver) {
402-
typeResolverMap.putIfAbsent(unionType.getName(), typeResolver);
403-
return this;
439+
if (!typeResolverMap.containsKey(unionType.getName())) {
440+
typeResolverMap.put(unionType.getName(), typeResolver);
441+
return markChanged();
442+
}
443+
return markChanged();
404444
}
405445

406446
public Builder typeResolver(String typeName, TypeResolver typeResolver) {
407447
typeResolverMap.put(assertValidName(typeName), typeResolver);
408-
return this;
448+
return markChanged();
409449
}
410450

411451
public Builder typeResolvers(GraphQLCodeRegistry codeRegistry) {
412452
this.typeResolverMap.putAll(codeRegistry.typeResolverMap);
413-
return this;
453+
return markChanged(!codeRegistry.typeResolverMap.isEmpty());
414454
}
415455

416456
public Builder fieldVisibility(GraphqlFieldVisibility fieldVisibility) {
417457
this.fieldVisibility = assertNotNull(fieldVisibility);
418-
return this;
458+
return markChanged();
419459
}
420460

421461
public Builder clearDataFetchers() {
422462
dataFetcherMap.clear();
423-
return this;
463+
return markChanged();
424464
}
425465

426466
public Builder clearTypeResolvers() {
427467
typeResolverMap.clear();
428-
return this;
468+
return markChanged();
429469
}
430470

431471
public GraphQLCodeRegistry build() {

‎src/main/java/graphql/schema/SchemaTransformer.java‎

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ private Object transformImpl(final GraphQLSchema schema, GraphQLSchemaElement sc
140140
final Map<String, GraphQLTypeReference> typeReferences = new LinkedHashMap<>();
141141

142142
// first pass - general transformation
143-
traverseAndTransform(dummyRoot, changedTypes, typeReferences, visitor, codeRegistry);
143+
boolean schemaChanged = traverseAndTransform(dummyRoot, changedTypes, typeReferences, visitor, codeRegistry);
144144

145145
// if we have changed any named elements AND we have type references referring to them then
146146
// we need to make a second pass to replace these type references to the new names
@@ -152,9 +152,13 @@ private Object transformImpl(final GraphQLSchema schema, GraphQLSchemaElement sc
152152
}
153153

154154
if (schema != null) {
155-
GraphQLSchema graphQLSchema = dummyRoot.rebuildSchema(codeRegistry);
156-
if (postTransformation != null) {
157-
graphQLSchema = graphQLSchema.transform(postTransformation);
155+
156+
GraphQLSchema graphQLSchema = schema;
157+
if (schemaChanged || codeRegistry.hasChanged()) {
158+
graphQLSchema = dummyRoot.rebuildSchema(codeRegistry);
159+
if (postTransformation != null) {
160+
graphQLSchema = graphQLSchema.transform(postTransformation);
161+
}
158162
}
159163
return graphQLSchema;
160164
} else {
@@ -177,7 +181,7 @@ public TraversalControl visitGraphQLTypeReference(GraphQLTypeReference typeRef,
177181
traverseAndTransform(dummyRoot, new HashMap<>(), new HashMap<>(), typeRefVisitor, codeRegistry);
178182
}
179183

180-
private void traverseAndTransform(DummyRoot dummyRoot, Map<String, GraphQLNamedType> changedTypes, Map<String, GraphQLTypeReference> typeReferences, GraphQLTypeVisitor visitor, GraphQLCodeRegistry.Builder codeRegistry) {
184+
private boolean traverseAndTransform(DummyRoot dummyRoot, Map<String, GraphQLNamedType> changedTypes, Map<String, GraphQLTypeReference> typeReferences, GraphQLTypeVisitor visitor, GraphQLCodeRegistry.Builder codeRegistry) {
181185
List<NodeZipper<GraphQLSchemaElement>> zippers = new LinkedList<>();
182186
Map<GraphQLSchemaElement, NodeZipper<GraphQLSchemaElement>> zipperByNodeAfterTraversing = new LinkedHashMap<>();
183187
Map<GraphQLSchemaElement, NodeZipper<GraphQLSchemaElement>> zipperByOriginalNode = new LinkedHashMap<>();
@@ -268,16 +272,16 @@ public TraversalControl backRef(TraverserContext<GraphQLSchemaElement> context)
268272

269273
List<List<GraphQLSchemaElement>> stronglyConnectedTopologicallySorted = getStronglyConnectedComponentsTopologicallySorted(reverseDependencies, typeRefReverseDependencies);
270274

271-
zipUpToDummyRoot(zippers, stronglyConnectedTopologicallySorted, breadcrumbsByZipper, zipperByNodeAfterTraversing);
275+
return zipUpToDummyRoot(zippers, stronglyConnectedTopologicallySorted, breadcrumbsByZipper, zipperByNodeAfterTraversing);
272276
}
273277

274278

275-
private void zipUpToDummyRoot(List<NodeZipper<GraphQLSchemaElement>> zippers,
276-
List<List<GraphQLSchemaElement>> stronglyConnectedTopologicallySorted,
277-
Map<NodeZipper<GraphQLSchemaElement>, List<List<Breadcrumb<GraphQLSchemaElement>>>> breadcrumbsByZipper,
278-
Map<GraphQLSchemaElement, NodeZipper<GraphQLSchemaElement>> nodeToZipper) {
279+
private boolean zipUpToDummyRoot(List<NodeZipper<GraphQLSchemaElement>> zippers,
280+
List<List<GraphQLSchemaElement>> stronglyConnectedTopologicallySorted,
281+
Map<NodeZipper<GraphQLSchemaElement>, List<List<Breadcrumb<GraphQLSchemaElement>>>> breadcrumbsByZipper,
282+
Map<GraphQLSchemaElement, NodeZipper<GraphQLSchemaElement>> nodeToZipper) {
279283
if (zippers.size() == 0) {
280-
return;
284+
return false;
281285
}
282286
Set<NodeZipper<GraphQLSchemaElement>> curZippers = new LinkedHashSet<>(zippers);
283287

@@ -331,8 +335,8 @@ private void zipUpToDummyRoot(List<NodeZipper<GraphQLSchemaElement>> zippers,
331335
breadcrumbsByZipper.put(newZipper, breadcrumbsForOriginalParent);
332336

333337
}
334-
335338
}
339+
return true;
336340
}
337341

338342
private Map<NodeZipper<GraphQLSchemaElement>, List<Breadcrumb<GraphQLSchemaElement>>> zipperWithSameParent(GraphQLSchemaElement parent,

‎src/main/java/graphql/schema/idl/SchemaDirectiveWiringSchemaGeneratorPostProcessing.java‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,23 +43,35 @@ public SchemaDirectiveWiringSchemaGeneratorPostProcessing(TypeDefinitionRegistry
4343

4444
@Override
4545
public GraphQLSchema process(GraphQLSchema originalSchema) {
46-
GraphQLSchema newSchema = SchemaTransformer.transformSchema(originalSchema, new Visitor());
47-
return newSchema.transform(builder -> {
48-
// they could have changed the code registry so rebuild it
49-
GraphQLCodeRegistry codeRegistry = this.codeRegistryBuilder.build();
50-
builder.codeRegistry(codeRegistry);
51-
});
46+
codeRegistryBuilder.trackChanges();
47+
Visitor visitor = new Visitor();
48+
GraphQLSchema newSchema = SchemaTransformer.transformSchema(originalSchema, visitor);
49+
if (visitor.schemaChanged() || codeRegistryBuilder.hasChanged()) {
50+
return newSchema.transform(builder -> {
51+
// they could have changed the code registry so rebuild it
52+
GraphQLCodeRegistry codeRegistry = this.codeRegistryBuilder.build();
53+
builder.codeRegistry(codeRegistry);
54+
});
55+
}
56+
return newSchema;
5257
}
5358

5459
public class Visitor extends GraphQLTypeVisitorStub {
5560

61+
private boolean schemaChanged = false;
62+
63+
public boolean schemaChanged() {
64+
return schemaChanged;
65+
}
66+
5667
private SchemaGeneratorDirectiveHelper.Parameters mkBehaviourParams() {
5768
return new SchemaGeneratorDirectiveHelper.Parameters(typeRegistry, runtimeWiring, directiveBehaviourContext, codeRegistryBuilder);
5869
}
5970

6071
private TraversalControl changOrContinue(GraphQLSchemaElement node, GraphQLSchemaElement newNode, TraverserContext<GraphQLSchemaElement> context) {
6172
if (node != newNode) {
6273
TreeTransformerUtil.changeNode(context, newNode);
74+
schemaChanged = true;
6375
}
6476
return CONTINUE;
6577
}

‎src/main/java/graphql/util/DefaultTraverserContext.java‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,10 +56,11 @@ public DefaultTraverserContext(T curNode,
5656
if (parent == null || parent.isRootContext()) {
5757
this.breadcrumbs = ImmutableKit.emptyList();
5858
} else {
59-
List<Breadcrumb<T>> breadcrumbs = new ArrayList<>(parent.getBreadcrumbs().size() + 1);
60-
breadcrumbs.add(new Breadcrumb<>(this.parent.thisNode(), this.location));
61-
breadcrumbs.addAll(parent.getBreadcrumbs());
62-
this.breadcrumbs = ImmutableList.copyOf(breadcrumbs);
59+
this.breadcrumbs = ImmutableList.<Breadcrumb<T>>builderWithExpectedSize(parent.getBreadcrumbs().size() + 1)
60+
.add(new Breadcrumb<>(this.parent.thisNode(), this.location))
61+
.addAll(parent.getBreadcrumbs())
62+
.build();
63+
6364
}
6465
}
6566

‎src/test/groovy/graphql/schema/GraphQLCodeRegistryTest.groovy‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -345,4 +345,50 @@ class GraphQLCodeRegistryTest extends Specification {
345345
codeRegistry.getDataFetcher(userCoords, userFieldDef) == dfUser
346346

347347
}
348+
349+
def "builder can track changes - internal methods"() {
350+
DataFetcher<?> dfSystem = { env -> "system" }
351+
DataFetcher<?> dfUser = { env -> "user" }
352+
def systemFieldDef = newFieldDefinition().name("__system").type(GraphQLInt).build()
353+
def userFieldDef = newFieldDefinition().name("field").type(GraphQLInt).build()
354+
def systemCoords = FieldCoordinates.systemCoordinates(systemFieldDef.name)
355+
def userCoords = FieldCoordinates.coordinates("User", userFieldDef.name)
356+
357+
when:
358+
def codeRegistry = GraphQLCodeRegistry.newCodeRegistry()
359+
then:
360+
!codeRegistry.hasChanged()
361+
362+
when:
363+
codeRegistry.dataFetcher(systemCoords, dfSystem)
364+
codeRegistry.dataFetcher(userCoords, dfUser)
365+
then:
366+
codeRegistry.hasChanged()
367+
368+
when:
369+
codeRegistry.trackChanges()
370+
then:
371+
!codeRegistry.hasChanged()
372+
373+
when:
374+
codeRegistry.clearDataFetchers()
375+
then:
376+
codeRegistry.hasChanged()
377+
378+
when:
379+
def newCodeRegistry = GraphQLCodeRegistry.newCodeRegistry(codeRegistry.build())
380+
then:
381+
!newCodeRegistry.hasChanged()
382+
383+
when:
384+
newCodeRegistry = GraphQLCodeRegistry.newCodeRegistry(codeRegistry.build())
385+
then:
386+
!newCodeRegistry.hasChanged()
387+
388+
when:
389+
newCodeRegistry.dataFetcher(systemCoords, dfSystem as DataFetcher)
390+
newCodeRegistry.dataFetcher(userCoords, dfUser as DataFetcher)
391+
then:
392+
newCodeRegistry.hasChanged()
393+
}
348394
}

‎src/test/groovy/graphql/schema/SchemaTransformerTest.groovy‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -744,4 +744,23 @@ type Query {
744744
}
745745
'''
746746
}
747+
748+
def "if nothing changes in the schema transformer, we return the same object"() {
749+
750+
def schema = TestUtil.schema("type Query { f : String }")
751+
752+
def fieldChanger = new GraphQLTypeVisitorStub() {
753+
754+
@Override
755+
TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node,
756+
TraverserContext<GraphQLSchemaElement> context) {
757+
return TraversalControl.CONTINUE
758+
}
759+
}
760+
761+
when:
762+
def newSchema = SchemaTransformer.transformSchema(schema, fieldChanger)
763+
then:
764+
newSchema === schema
765+
}
747766
}

‎src/test/groovy/graphql/schema/idl/SchemaGeneratorDirectiveHelperTest.groovy‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ class SchemaGeneratorDirectiveHelperTest extends Specification {
5252
})
5353
.build()
5454

55-
def assertCallHierarchy(elementHierarchy, astHierarchy, String name, List<String> l) {
55+
static def assertCallHierarchy(elementHierarchy, astHierarchy, String name, List<String> l) {
5656
assert elementHierarchy[name] == l, "unexpected elementHierarchy"
5757
assert astHierarchy[name] == l, "unexpected astHierarchy"
5858
true

0 commit comments

Comments
 (0)