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 a FetchedValue
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
The
graphql.execution.FetchedValueis a wrapper object for "data fetched values" that are then passed around in the engineIts shape is like this
That is its the actual fetched object and if the fetched result is a
graphql.execution.DataFetcherResultthen 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 fetchedValueaspect, eg the unboxed fetched value.So we could avoid a MAJOR allocation here by NOT wrapping values in a
graphql.execution.FetchedValueEvery value that comes via a DataFetcher is wrapped today in a
graphql.execution.FetchedValueThis means we do an allocation for every value returned which is mostly pointless.
The wrinkle here in getting ride of this this
It is either a promise or a materialised FetchedValue here.
graphql.execution.instrumentation.parameters.InstrumentationFieldCompleteParameters#getFetchedValuereturns an object that is assumed to be aFetchedValueThis is actually declared as
Objectbut it was alwaysFetchedValueso nominally it might have the errors and localcontext on it. ThisInstrumentationis 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
fetchFieldmethod change is ok - we dont really want ExecutionStrategy to be API even if it is.I think the
Instrumentationsemantic 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