-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Added a read only copy of a type registry for performance reasons #4021
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@Override | ||
public void remove(String key, SDLDefinition definition) { | ||
throw unsupportedOperationException(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the mutative methods are unsupported
public Map<String, DirectiveDefinition> getDirectiveDefinitions() { | ||
return directiveDefinitions; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where the performance gains will be made
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this use the type just to be more explicit but I didnt change it every where it gets used
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.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 agraphql.schema.idl.ImmutableTypeDefinitionRegistry
that is aTypeDefinitionRegistry
So code that is read only like the SchemaGenerator and TypeCheckers can be faster because they dont allocate copies all the time