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
Improve ENF isConditional #3241
Improve ENF isConditional #3241
Conversation
*/ | ||
@SuppressWarnings({"unchecked", "rawtypes"}) | ||
private Set<GraphQLInterfaceType> getInterfacesCommonToAllOutputTypes(GraphQLSchema schema) { | ||
Ref<Set<GraphQLInterfaceType>> ref = new Ref<>(); |
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.
Why do you need this Ref class?
Set> set = null;
if (set = null) {
set = new LinkedHashSet()
} else {
set.retainAll(x)
}
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.
It's Java. Can't touch non (effectively) final variables from inside lambdas.
(This gets used inside the forEachFieldDefinitions
lambda).
It's basically a replacement for AtomicReference
or Set<GraphQLInterfaceType>[]
but more performant.
I will however move this down closer to the lambda usage.
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.
Ahh I get it - another trick is an array with one entry
Set<GraphQLInterfaceType> ref[] = {null}
and then use the one entry in a mutable way.
I like your approach.
@Internal | ||
public class Ref<T> { | ||
public T value; | ||
} |
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 dont see how this is useful?
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 get it now
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.
Maybe if this was named MutableRef
it might be clearer on how it can be used
Benchmarks using a large query. Old code
New code
|
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.
Small naming suggestion on Ref - I like how this changed no tests and it still works
I am pretty sure there is lots of conditional tests
No description provided.