-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Iterate over implementations in isPossibleType #4033
Conversation
e73ac24
to
c95a0b4
Compare
.anyMatch(itd -> itd.getImplements() | ||
.stream() | ||
.map(TypeInfo::getTypeName) | ||
.map(tn -> types.get(tn.getName())) |
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.
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.
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,
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.
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.
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
c95a0b4
to
4c27acc
Compare
@@ -26,6 +26,18 @@ public static TypeInfo typeInfo(Type type) { | |||
return new TypeInfo(type); | |||
} | |||
|
|||
public static TypeName getTypeName(Type type) { |
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.
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.
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())) | ||
); |
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 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.
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 thatgetTypes(Class<T> targetClass)
is very different from the reference totypes
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.
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.
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
return implementingTypeDefinitions.stream() | ||
.anyMatch(od -> od.getName().equals(targetObjectTypeDef.getName())); | ||
|
||
for (TypeDefinition<?> t : types.values()) { |
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.
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.
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
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 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" | ||
} |
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 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) |
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 callsgetAllImplementationsOf
first.getTypes
gets called bygetAllImplementationsOf
to collect allImplementingTypeDefinition
s. (src)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.