Iterate over implementations in isPossibleType by xuorig · Pull Request #4033

Iterate over implementations in isPossibleType by xuorig · Pull Request #4033 · graphql-java/graphql-java

Current isPossibleType needs to go through two expensive things for what seems like a simple check, due. to the fact it calls getAllImplementationsOf first.

getTypes gets called by getAllImplementationsOf to collect all ImplementingTypeDefinitions. (src)
For every implementation, a TypeInfo object is created to get name.

This PR tries to avoid both these things, and try to keep to a single iteration of types/impls.

		.anyMatch(itd -> itd.getImplements()
		.stream()
		.map(TypeInfo::getTypeName)
		.map(tn -> types.get(tn.getName()))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We get the TypeDefinition, just to keep the existing semantics of that method that verified the type actually exists in the schema.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For performance reason we avoid streams. Normal iterations are faster in our experience,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I avoid them too but I had seen others so tried to match general style 😸 will remove.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have evolved the code base BTW - older code still uses.stream() but mostly we try to unwind them now to get those extra 0.1% boosts

		return new TypeInfo(type);
		}

		public static TypeName getTypeName(Type type) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What TypeInfo gave us, but without allocation of a TypeInfo object and without the tracking of decorations.

		}
		return (TypeName) type;
		}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a unit test and some JavaDoc

		.map(TypeInfo::getTypeName)
		.map(tn -> types.get(tn.getName()))
		.anyMatch(td -> td.getName().equals(iFace.getName()))
		);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused here

The code above is about the same as getAllImplementationsOf()

Its the saving here because it avoids

Optional<InterfaceTypeDefinition> interfaceTypeDef = getType(iFace, InterfaceTypeDefinition.class);

and the iinner

        String typeName = TypeInfo.typeInfo(type).getName();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, it's basically the two things I have in the PR description:

No call to getTypes, which allocates a huge list of implementing types (Edit: note that getTypes(Class<T> targetClass) is very different from the reference to types used in this PR)
No allocation of a TypeInfo object (TypeInfo.typeInfo)

On top of this we don't need to allocate the return list of getAllImplementationsOf, simply check if any matches.

Doesn't look like much but for our schema this is about 25-30% of the allocations for a makeExecutableSchema call.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look like much but for our schema this is about 25-30% of the allocations for a makeExecutableSchema call.

To add to this:

Imagine you have 1000s of types in a schema, including many abstract types.
For every field of all interfaces types
- Get and allocate a list for all the object and interface types in the schema
- For all the implementations of the above, allocate a TypeInfo object
- Filter and allocate a list of all implementing types

Marc-Andre Giroux added 2 commits

July 3, 2025 08:44

xuorig marked this pull request as ready for review

July 3, 2025 12:45

		return implementingTypeDefinitions.stream()
		.anyMatch(od -> od.getName().equals(targetObjectTypeDef.getName()));

		for (TypeDefinition<?> t : types.values()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth investing in a cached reverse lookup at some point instead of looking through all the types everytime. Maybe on that new read only type registry.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you see this reverse lookup working - simply a map of name -> type say or somethings else?

ps the read only type registry PR is now merged

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think i know now - I suspect you want something like


    private final Map<InterfaceTypeDefinition, List<ImplementingTypeDefinition>> allImplementationsOf;
    private final Map<InterfaceTypeDefinition, List<ObjectTypeDefinition>> implementationsOf;


    @Override
    public List<ImplementingTypeDefinition> getAllImplementationsOf(InterfaceTypeDefinition targetInterface) {
        return allImplementationsOf.getOrDefault(targetInterface, ImmutableList.of());
    }

    @Override
    public List<ObjectTypeDefinition> getImplementationsOf(InterfaceTypeDefinition targetInterface) {
        return implementationsOf.getOrDefault(targetInterface, ImmutableList.of());
    }

		"[named!]" \| "named"
		"[named!]!" \| "named"
		"[[named!]!]" \| "named"
		}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

I am going to close this PR in favour of #4040 - that new PR has your changes and more. plus with the read only reverse cache, its even cheaper than this PR since it does not traverse all types each call (rather once at build time)