-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Description
The graphql.execution.FetchedValue
is a wrapper object for "data fetched values" that are then passed around in the engine
Its shape is like this
@PublicApi
public class FetchedValue {
private final Object fetchedValue;
private final Object localContext;
private final ImmutableList<GraphQLError> errors;
That is its the actual fetched object and if the fetched result is a graphql.execution.DataFetcherResult
then the errors and local context are placed into the wrapper object.
The thing is this is never actually used by the graphql engine for anything other than the private final Object fetchedValue
aspect, eg the unboxed fetched value.
So we could avoid a MAJOR allocation here by NOT wrapping values in a graphql.execution.FetchedValue
Every value that comes via a DataFetcher is wrapped today in a graphql.execution.FetchedValue
This means we do an allocation for every value returned which is mostly pointless.
The wrinkle here in getting ride of this this
- ExecutionStrategy has a protected DUCK type method
@DuckTyped(shape = "CompletableFuture<FetchedValue> | FetchedValue")
protected Object fetchField(ExecutionContext executionContext, ExecutionStrategyParameters parameters) {
It is either a promise or a materialised FetchedValue here.
graphql.execution.instrumentation.parameters.InstrumentationFieldCompleteParameters#getFetchedValue
returns an object that is assumed to be aFetchedValue
public Object getFetchedValue() {
return fetchedValue;
}
This is actually declared as Object
but it was always FetchedValue
so nominally it might have the errors and localcontext on it. This Instrumentation
is called BEFORE we complete a value.
IF we got rid of FetchedValue then this be a semantic breaking change to the Instrumentation
- ie its not an API breaking change but it is a semantic one.
Brads view
I think that the fetchField
method change is ok - we dont really want ExecutionStrategy to be API even if it is.
I think the Instrumentation
semantic breaking change is more impactful BUT if we can avoid a double allocation for EVERY value we ever return then I think this will be worth the trade off