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#4021bbakerman merged 2 commits into
Conversation
Sorry, something went wrong.
| @Override | ||
| public void remove(String key, SDLDefinition definition) { | ||
| throw unsupportedOperationException(); | ||
| } |
There was a problem hiding this comment.
All the mutative methods are unsupported
Sorry, something went wrong.
| public Map<String, DirectiveDefinition> getDirectiveDefinitions() { | ||
| return directiveDefinitions; | ||
| } | ||
| } |
There was a problem hiding this comment.
No copies made for these read only methods
Sorry, something went wrong.
| 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); |
There was a problem hiding this comment.
This is where the performance gains will be made
Sorry, something went wrong.
| static class BuildContext { | ||
| private final TypeDefinitionRegistry typeRegistry; | ||
| private final ImmutableTypeDefinitionRegistry typeRegistry; | ||
| private final RuntimeWiring wiring; |
There was a problem hiding this comment.
I made this use the type just to be more explicit but I didnt change it every where it gets used
Sorry, something went wrong.
| public class SchemaTypeChecker { | ||
|
|
||
| public List<GraphQLError> checkTypeRegistry(TypeDefinitionRegistry typeRegistry, RuntimeWiring wiring) throws SchemaProblem { | ||
| public List<GraphQLError> checkTypeRegistry(ImmutableTypeDefinitionRegistry typeRegistry, RuntimeWiring wiring) throws SchemaProblem { |
There was a problem hiding this comment.
I made this use the type just to be more explicit but I didnt change it every where it gets used
Sorry, something went wrong.
Test Results 319 files - 631 319 suites - 631 2m 50s ⏱️ - 5m 35s 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] … graphql.schema.fetching.LambdaFetchingSupportTest ‑ different class loaders induce certain behaviours ♻️ This comment has been updated with latest results. |
Sorry, something went wrong.
xuorig
left a comment
There was a problem hiding this comment.
Thank you! This will lead to big performance gains for larger schemas 🙇
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
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