GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Added a read only copy of a type registry for performance reasons by bbakerman · Pull Request #4021 · graphql-java/graphql-java

Conversation

bbakerman commented Jun 24, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

xuorig reacted with heart emoji

All reactions

❤️ 1 reaction


          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

Comment thread

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

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

bbakerman Jun 24, 2025

Copy link

Copy Markdown

Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman commented

Jun 24, 2025

View reviewed changes

Comment thread

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

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

bbakerman Jun 24, 2025

Copy link

Copy Markdown

Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman commented

Jun 24, 2025

View reviewed changes

Comment thread

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);

bbakerman Jun 24, 2025

Copy link

Copy Markdown

Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman commented

Jun 24, 2025

View reviewed changes

Comment thread

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

		static class BuildContext {
		private final TypeDefinitionRegistry typeRegistry;
		private final ImmutableTypeDefinitionRegistry typeRegistry;
		private final RuntimeWiring wiring;

bbakerman Jun 24, 2025

Copy link

Copy Markdown

Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman commented

Jun 24, 2025

View reviewed changes

Comment thread

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

		public class SchemaTypeChecker {

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

bbakerman Jun 24, 2025

Copy link

Copy Markdown

Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

github-actions Bot commented Jun 24, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


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

3409acf

xuorig approved these changes

Jun 24, 2025

View reviewed changes

xuorig left a comment

Copy link

Copy Markdown

Contributor

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 🙇

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

xuorig commented Jul 2, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bbakerman commented Jul 2, 2025

Copy link

Copy Markdown

Member Author

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 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

xuorig mentioned this pull request

Jul 2, 2025

Iterate over implementations in isPossibleType #4033

Closed

xuorig commented Jul 2, 2025

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bbakerman merged commit 52bdcf1 into master

Jul 4, 2025

rstata mentioned this pull request

Dec 20, 2025

GraphQLSchema.FastBuilder for High-Performance Schema Construction #4196

Closed

Reviewers

1 more reviewer

xuorig

xuorig approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

25.x breaking changes

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

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!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bbakerman commented Jun 24, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot commented Jun 24, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

xuorig commented Jul 2, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.