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
Conversation
| @Override | ||
| public void remove(String key, SDLDefinition definition) { | ||
| throw unsupportedOperationException(); | ||
| } |
There was a problem hiding this comment.
All the mutative methods are unsupported
| public Map<String, DirectiveDefinition> getDirectiveDefinitions() { | ||
| return directiveDefinitions; | ||
| } | ||
| } |
There was a problem hiding this comment.
No copies made for these read only methods
| 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
| 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
| 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
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.This pull request skips 1 test.♻️ This comment has been updated with latest results. |
xuorig
left a comment
There was a problem hiding this comment.
Thank you! This will lead to big performance gains for larger schemas 🙇
|
Hey @bbakerman 👋 There's a great opportunity to precompute It would tackle another perf issue in |
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 |
Ok I thought it did at first, but it is actually called only for each interface returned by fields of interfaces, in the Looking at it a bit more, I think we should probably avoid calling |
The
graphql.schema.idl.TypeDefinitionRegistryis very mutable and we cant break its mutability at this stage of its life.So we have added a
.readOnly()mechanism that returns agraphql.schema.idl.ImmutableTypeDefinitionRegistrythat is aTypeDefinitionRegistrySo code that is read only like the SchemaGenerator and TypeCheckers can be faster because they dont allocate copies all the time