MergedField single field support #4056
Conversation
| @Internal | ||
| public static String mkNameForPath(MergedField mergedField) { | ||
| return mkNameForPath(mergedField.getFields()); | ||
| return mergedField.getResultKey(); |
There was a problem hiding this comment.
dont materialise the list just to get the first one and its result key
| } | ||
| this.collectFields(parameters, field.getSelectionSet(), visitedFragments, subFields, null, incrementalSupport); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This was a consumer pattern - not wont materialise the list to consume the values
| .addDeferredExecution(deferredExecution)) | ||
| ); | ||
| MergedField currentMergedField = fields.get(name); | ||
| fields.put(name, currentMergedField.newMergedFieldWith(field,deferredExecution)); |
There was a problem hiding this comment.
The newMergedFieldWith method doesn't allocate a builder to make a new merged field like this builder does
|
Is that a breaking change? |
Test Results 323 files 323 suites 3m 23s ⏱️ Results for commit d98674b. ♻️ This comment has been updated with latest results. |
Is this a general comment? Or a specific bit of text The answer is its NOT a breaking change - I actually know this because I reverted the implementation (modulo the new forEach method and fieldCount method) to the old and then ran all the new tests on "builder" ness and they passed along with all the other tests. So it acts exactly like the old and has the same signature |
| private final Field singleField; | ||
| private final ImmutableList<DeferredExecution> deferredExecutions; | ||
|
|
||
| private MergedField(ImmutableList<Field> fields, ImmutableList<DeferredExecution> deferredExecutions) { |
There was a problem hiding this comment.
We have two classes now - a MergedField and a MultiMergedField and the second is only used when we have multiple fields and hence burns a list but otherwise its a single value
| */ | ||
| public List<Field> getFields() { | ||
| return fields; | ||
| return ImmutableList.of(singleField); |
There was a problem hiding this comment.
While this creates a list each call - we have changed the key consuming code so that this is not generally called.
There was a problem hiding this comment.
It could perhaps be deprecated since calling it makes a list and we want to discourage that if we can
There was a problem hiding this comment.
Cant - DFE has a getFields() method
|
|
||
| mergedSelectionSet.getSubFields().values().forEach(mergedField -> { | ||
| if (mergedField.getFields().size() > mergedField.getDeferredExecutions().size()) { | ||
| if (mergedField.getFieldsCount() > mergedField.getDeferredExecutions().size()) { |
There was a problem hiding this comment.
dont materialise the list to get a count
|
nice! do you see any improvement? |
| */ | ||
| private ImmutableList.@Nullable Builder<Field> fields; | ||
| private @Nullable Field singleField; | ||
| private final ImmutableList.Builder<DeferredExecution> deferredExecutions = new ImmutableList.Builder<>(); |
There was a problem hiding this comment.
The builder has been made more complicated and smarter in memory allocation BUT its not strictly needed. Because in the graphql-java engine we never call the builder in the field collector any more. We use helpers that directly create MergedFields without a builder. So the builder code is improved, its not strictly needed
| private Builder() { | ||
| } | ||
|
|
||
| private Builder(MergedField existing) { |
There was a problem hiding this comment.
Technically we dont have to change the builder since the field collector now uses a direct instationation methods to make a MergedField without a builder. But I thought why not - make it as memory efficient as possible even if its more complicated with its lazy instatation
| } | ||
|
|
||
| public MergedField build() { | ||
| return new MergedField(fields.build(), deferredExecutions.build()); |
There was a problem hiding this comment.
bit hard to see how the new .build() method works in this diff
| @Override | ||
| public int hashCode() { | ||
| return Objects.hashCode(label); | ||
| } |
There was a problem hiding this comment.
Figured it should have a .equals / hashCode
I didnt measure strongly again - it wont be faster but it will use less memory and that can translate into speed and also it will scale better if you have lots of fields |
| BiMap<GraphQLDirective, QueryAppliedDirective> gqlDirectiveCounterParts = HashBiMap.create(); | ||
| BiMap<QueryAppliedDirective, GraphQLDirective> gqlDirectiveCounterPartsInverse = gqlDirectiveCounterParts.inverse(); | ||
| mergedField.getFields().forEach(field -> { | ||
| mergedField.forEach(field -> { |
There was a problem hiding this comment.
dont materlise the list view - forEach can handle single fields more efficiently without creating a list of 1
MergedField allows us to represent on or more fields in one structure BUT 90% of the time there is only a single field. So we are allocating memory for a list of one field all the time
This changes it so that if there is only a single field, no list of allocated
It also changes a few things do they dont call
mergedField.getFields()which will materialised the list. Rather there are new methods to test its size without materialising the list and so on. Also a.forEach()method thats aware of the state of the internal list or not