Skip to content

Iterate over implementations in isPossibleType #4033

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

Closed

Conversation

xuorig
Copy link

@xuorig xuorig commented Jul 2, 2025

See #4021 (comment) for more context.

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

  1. getTypes gets called by getAllImplementationsOf to collect all ImplementingTypeDefinitions. (src)
  2. 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()))
Copy link
Author

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.

Copy link
Member

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,

Copy link
Author

@xuorig xuorig Jul 3, 2025

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.

Copy link
Member

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

@xuorig xuorig force-pushed the is-possible-type-perf-improvement branch from c95a0b4 to 4c27acc Compare July 2, 2025 14:17
@@ -26,6 +26,18 @@ public static TypeInfo typeInfo(Type type) {
return new TypeInfo(type);
}

public static TypeName getTypeName(Type type) {
Copy link
Author

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

Copy link
Member

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()))
);
Copy link
Member

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

??

Copy link
Author

@xuorig xuorig Jul 3, 2025

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:

  1. 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)
  2. 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.

Copy link
Author

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

@xuorig 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()) {
Copy link
Author

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.

Copy link
Member

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

Copy link
Member

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"
}
Copy link
Member

Choose a reason for hiding this comment

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

👍

@bbakerman
Copy link
Member

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)

@bbakerman bbakerman closed this Jul 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants