Iterate over implementations in isPossibleType#4033
Conversation
e73ac24 to
c95a0b4
Compare
There was a problem hiding this comment.
We get the TypeDefinition, just to keep the existing semantics of that method that verified the type actually exists in the schema.
There was a problem hiding this comment.
For performance reason we avoid streams. Normal iterations are faster in our experience,
There was a problem hiding this comment.
Great, I avoid them too but I had seen others so tried to match general style 😸 will remove.
There was a problem hiding this comment.
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
c95a0b4 to
4c27acc
Compare
| return new TypeInfo(type); | ||
| } | ||
|
|
||
| public static TypeName getTypeName(Type type) { |
There was a problem hiding this comment.
What TypeInfo gave us, but without allocation of a TypeInfo object and without the tracking of decorations.
| } | ||
| return (TypeName) type; | ||
| } | ||
|
|
There was a problem hiding this comment.
This needs a unit test and some JavaDoc
| .map(TypeInfo::getTypeName) | ||
| .map(tn -> types.get(tn.getName())) | ||
| .anyMatch(td -> td.getName().equals(iFace.getName())) | ||
| ); |
There was a problem hiding this comment.
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();
??
There was a problem hiding this comment.
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 thatgetTypes(Class<T> targetClass)is very different from the reference totypesused 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.
There was a problem hiding this comment.
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
TypeInfoobject - Filter and allocate a list of all implementing types
| return implementingTypeDefinitions.stream() | ||
| .anyMatch(od -> od.getName().equals(targetObjectTypeDef.getName())); | ||
|
|
||
| for (TypeDefinition<?> t : types.values()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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" | ||
| } |

See #4021 (comment) for more context.
Current
isPossibleTypeneeds to go through two expensive things for what seems like a simple check, due. to the fact it callsgetAllImplementationsOffirst.getTypesgets called bygetAllImplementationsOfto collect allImplementingTypeDefinitions. (src)TypeInfoobject is created to get name.This PR tries to avoid both these things, and try to keep to a single iteration of types/impls.