Added a read only copy of a type registry for performance reasons #4021

bbakerman · 2025-06-24T03:57:40Z

The graphql.schema.idl.TypeDefinitionRegistry is very mutable and we cant break its mutability at this stage of its life.

So we have added a .readOnly() mechanism that returns a graphql.schema.idl.ImmutableTypeDefinitionRegistry that is a TypeDefinitionRegistry

So code that is read only like the SchemaGenerator and TypeCheckers can be faster because they dont allocate copies all the time

bbakerman · 2025-06-24T03:59:48Z

src/main/java/graphql/schema/idl/ImmutableTypeDefinitionRegistry.java

+    @Override
+    public void remove(String key, SDLDefinition definition) {
+        throw unsupportedOperationException();
+    }


All the mutative methods are unsupported

bbakerman · 2025-06-24T04:00:11Z

src/main/java/graphql/schema/idl/ImmutableTypeDefinitionRegistry.java

+    public Map<String, DirectiveDefinition> getDirectiveDefinitions() {
+        return directiveDefinitions;
+    }
+}


No copies made for these read only methods

bbakerman · 2025-06-24T04:00:44Z

src/main/java/graphql/schema/idl/SchemaGenerator.java

-        List<GraphQLError> errors = typeChecker.checkTypeRegistry(typeRegistryCopy, wiring);
+        // by making it read only all the traversal and checks run faster
+        ImmutableTypeDefinitionRegistry fasterImmutableRegistry = typeRegistryCopy.readOnly();
+        List<GraphQLError> errors = typeChecker.checkTypeRegistry(fasterImmutableRegistry, wiring);


This is where the performance gains will be made

bbakerman · 2025-06-24T04:01:19Z

src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java

@@ -111,7 +111,7 @@ public class SchemaGeneratorHelper {
     * it gives us helper functions
     */
    static class BuildContext {
-        private final TypeDefinitionRegistry typeRegistry;
+        private final ImmutableTypeDefinitionRegistry typeRegistry;
        private final RuntimeWiring wiring;


I made this use the type just to be more explicit but I didnt change it every where it gets used

bbakerman · 2025-06-24T04:01:33Z

src/main/java/graphql/schema/idl/SchemaTypeChecker.java

@@ -52,7 +52,7 @@
 @Internal
 public class SchemaTypeChecker {

-    public List<GraphQLError> checkTypeRegistry(TypeDefinitionRegistry typeRegistry, RuntimeWiring wiring) throws SchemaProblem {
+    public List<GraphQLError> checkTypeRegistry(ImmutableTypeDefinitionRegistry typeRegistry, RuntimeWiring wiring) throws SchemaProblem {


I made this use the type just to be more explicit but I didnt change it every where it gets used

github-actions · 2025-06-24T04:02:43Z

Test Results

319 files - 631 319 suites - 631 2m 50s ⏱️ - 5m 35s
4 899 tests - 297 4 889 ✅ - 298 10 💤 + 1 0 ❌ ±0
4 988 runs - 9 925 4 978 ✅ - 9 906 10 💤 - 19 0 ❌ ±0

Results for commit 3409acf. ± Comparison against base commit aef8032.

This pull request removes 504 and adds 185 tests. Note that renamed tests count towards both.

	?
                __schema { types { fields { args { type { name fields { name }}}}}}
                __schema { types { fields { type { name fields { name }}}}}
                __schema { types { inputFields { type { inputFields { name }}}}}
                __schema { types { interfaces { fields { type { interfaces { name } } } } } }
                __schema { types { name} }
                __type(name : "t") { name }
                a1: __schema { types { name} }
                a1: __type(name : "t") { name }
                a2 :  __type(name : "t1") { name }
…

graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure23@7068f7ca delegate=graphql.AssertTest@5eeedb60 owner=graphql.AssertTest@5eeedb60 thisObject=graphql.AssertTest@5eeedb60 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure24@38548b19 delegate=graphql.AssertTest@5eeedb60 owner=graphql.AssertTest@5eeedb60 thisObject=graphql.AssertTest@5eeedb60 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure25@41aaedaa delegate=graphql.AssertTest@5eeedb60 owner=graphql.AssertTest@5eeedb60 thisObject=graphql.AssertTest@5eeedb60 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure20@c446b14 delegate=graphql.AssertTest@5eeedb60 owner=graphql.AssertTest@5eeedb60 thisObject=graphql.AssertTest@5eeedb60 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure21@3af356f delegate=graphql.AssertTest@5eeedb60 owner=graphql.AssertTest@5eeedb60 thisObject=graphql.AssertTest@5eeedb60 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure22@4443ef6f delegate=graphql.AssertTest@5eeedb60 owner=graphql.AssertTest@5eeedb60 thisObject=graphql.AssertTest@5eeedb60 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure3@3f36b447 delegate=graphql.AssertTest@5eeedb60 owner=graphql.AssertTest@5eeedb60 thisObject=graphql.AssertTest@5eeedb60 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure4@67a056f1 delegate=graphql.AssertTest@5eeedb60 owner=graphql.AssertTest@5eeedb60 thisObject=graphql.AssertTest@5eeedb60 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure5@72ed9aad delegate=graphql.AssertTest@5eeedb60 owner=graphql.AssertTest@5eeedb60 thisObject=graphql.AssertTest@5eeedb60 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertNotNull with different number of error args with non null does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_6prov0_closure6@52a36910 delegate=graphql.AssertTest@5eeedb60 owner=graphql.AssertTest@5eeedb60 thisObject=graphql.AssertTest@5eeedb60 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0]
…

This pull request skips 1 test.

graphql.schema.fetching.LambdaFetchingSupportTest ‑ different class loaders induce certain behaviours

♻️ This comment has been updated with latest results.

xuorig

Thank you! This will lead to big performance gains for larger schemas 🙇

xuorig · 2025-07-02T01:39:52Z

Hey @bbakerman 👋 There's a great opportunity to precompute getAllImplementationsOf for all interfaces in the read-only registry.

It would tackle another perf issue in checkInterfaceIsImplemented. Downside is a one-time cost when converting to the read-only registry. Thoughts?

bbakerman · 2025-07-02T04:16:33Z

Hey @bbakerman 👋 There's a great opportunity to precompute getAllImplementationsOf for all interfaces in the read-only registry.

It would tackle another perf issue in checkInterfaceIsImplemented. Downside is a one-time cost when converting to the read-only registry. Thoughts?

I am typing here without the code in front of me. Is this code always called when we generate a schema - in which case a pre-compute makes sense. Otherwise I would do it as in lazy way on first call.

Also I would get this PR in place and do it in another PR

xuorig · 2025-07-02T13:44:57Z

Is this code always called when we generate a schema - in which case a pre-compute makes sense. Otherwise I would do it as in lazy way on first call.

Ok I thought it did at first, but it is actually called only for each interface returned by fields of interfaces, in the isSubTypeOf check. So depending on the schema, this could vary.

Looking at it a bit more, I think we should probably avoid calling getAllImplementationsOf in there in general, what do you think of something like this instead: #4033

Added a read only copy of a type registry

e2f7467

bbakerman added this to the 25.x breaking changes milestone Jun 24, 2025

bbakerman mentioned this pull request Jun 24, 2025

Performance issue in makeSchema with schemas containing a lot of extensions #4020

Closed

bbakerman changed the title ~~Added a read only copy of a type registry~~ Added a read only copy of a type registry for performance reasons Jun 24, 2025

bbakerman commented Jun 24, 2025

View reviewed changes

Added a read only copy of a type registry - tweak tests

3409acf

xuorig approved these changes Jun 24, 2025

View reviewed changes

xuorig mentioned this pull request Jul 2, 2025

Iterate over implementations in isPossibleType #4033

Closed

bbakerman merged commit 52bdcf1 into master Jul 4, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Added a read only copy of a type registry for performance reasons #4021

Added a read only copy of a type registry for performance reasons #4021

Uh oh!

bbakerman commented Jun 24, 2025 •

edited

Loading

Uh oh!

bbakerman Jun 24, 2025

Uh oh!

bbakerman Jun 24, 2025

Uh oh!

bbakerman Jun 24, 2025

Uh oh!

bbakerman Jun 24, 2025

Uh oh!

bbakerman Jun 24, 2025

Uh oh!

github-actions bot commented Jun 24, 2025 •

edited

Loading

Uh oh!

xuorig left a comment

Uh oh!

xuorig commented Jul 2, 2025 •

edited

Loading

Uh oh!

bbakerman commented Jul 2, 2025

Uh oh!

xuorig commented Jul 2, 2025

Uh oh!

Uh oh!

Uh oh!

Added a read only copy of a type registry for performance reasons #4021

Added a read only copy of a type registry for performance reasons #4021

Uh oh!

Conversation

bbakerman commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbakerman Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

bbakerman Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

bbakerman Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

bbakerman Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

bbakerman Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

xuorig left a comment

Choose a reason for hiding this comment

Uh oh!

xuorig commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbakerman commented Jul 2, 2025

Uh oh!

xuorig commented Jul 2, 2025

Uh oh!

Uh oh!

Uh oh!

bbakerman commented Jun 24, 2025 •

edited

Loading

github-actions bot commented Jun 24, 2025 •

edited

Loading

xuorig commented Jul 2, 2025 •

edited

Loading