Lightweight data fetchers #2953

bbakerman · 2022-09-10T06:12:47Z

The idea here is to make the object creation for DataFetchers even lighter.

This is basically here for PropertyDataFetcher to implement since it ONLY needs the source really to get property values from.

The premise is that we avoid allocating all the objects that go into a DataFetchingEnvironment for simple property fetches

Benchmark Mode Cnt Score Error Units LightWeightPropertyFetchingBenchMark.benchMarkHeavyWeightFetchingThroughput thrpt 25 22.599 ± 1.062 ops/s LightWeightPropertyFetchingBenchMark.benchMarkLightWeightFetchingThroughput thrpt 25 23.303 ± 1.040 ops/s LightWeightPropertyFetchingBenchMark.benchMarkHeavyWeightFetchingAvgTime avgt 25 44.772 ± 2.599 ms/op LightWeightPropertyFetchingBenchMark.benchMarkLightWeightFetchingAvgTime avgt 25 43.132 ± 2.479 ms/op

So for this benchmark - this IS slightly faster - the more simple property fetches there are, the more savings to be made

This benchmark biases towards property fetches on purpose

…ht class

…ht class -using switch

bbakerman · 2022-09-10T08:31:54Z

src/main/java/graphql/GraphQL.java

 import java.util.ArrayList;
 import java.util.List;
 import java.util.Locale;
-import java.util.Map;


bbakerman · 2022-09-10T08:32:44Z

src/main/java/graphql/execution/ExecutionStrategy.java

- executionContext.getGraphQLSchema(),
- executionContext.getCoercedVariables().toMap(),
- executionContext.getGraphQLContext(),
- executionContext.getLocale());


These all move down into the supplier of the DataFetchingEnv - they are ONLY needed for it so UNTIL we ask for the DFE - none of this code is run

Its a bit hard to see - open the fill view to see it

bbakerman · 2022-09-10T08:33:51Z

src/main/java/graphql/execution/ExecutionStrategy.java

 fetchCtx.onCompleted(result, exception);
 if (exception != null) {
- return handleFetchingException(executionContext, environment, exception);
+ return handleFetchingException(executionContext, dataFetchingEnvironment.get(), exception);


we ONLY use the DFE here if we have an exception which is least trodden path

bbakerman · 2022-09-10T09:08:07Z

src/main/java/graphql/schema/DataFetcher.java

+ default T get(GraphQLFieldDefinition fieldDefinition, Object sourceObject, Supplier<DataFetchingEnvironment> environmentSupplier) throws Exception {
+ return get(environmentSupplier.get());
+ }
+


old DataFetchers will be called via delegating back to the old method. Those DFS that implement this method will be called direct

bbakerman · 2022-09-10T09:24:48Z

src/main/java/graphql/execution/ExecutionStrategy.java

+ }
+ return fetchedValue;
+ }
+


moved the fetch call into its own method to reduce method complexity

bbakerman · 2022-09-10T09:25:05Z

...n/java/graphql/execution/instrumentation/parameters/InstrumentationFieldFetchParameters.java

 @PublicApi
 public class InstrumentationFieldFetchParameters extends InstrumentationFieldParameters {
- private final DataFetchingEnvironment environment;
+ private final Supplier<DataFetchingEnvironment> environment;


needs to be a supplier until they ask for it

jord1e · 2022-09-16T14:08:52Z

this wouldn't disable any validation right, or would it?

If we take away looking at solely PropertyDataFetcher, would there be an advantage/disadvantage to introducing a isLeightWeight() method to DataFetcher instead of globally applying it via Backdoor? Would that even be useful?

It could be true by default for PropertyDataFetcher, if we know that we will need the DataFetchingEnvironment two method calls (including getSource(), it will be called later anyway) will be avoided at least (including the moving of fieldDef and the supplier onto the stack).

This would maybe confusing because we have two datafetcher methods and one will not be used when isLeightWeight is false. We could offer some default impls to check for it and throw an exception on invalid method calls in combination with invalid isLightWeight for the datafetcher.

bbakerman · 2022-09-19T00:02:20Z

@jord1e

this wouldn't disable any validation right, or would it?

No - this does the same amount of processing as usual. The saving here is that we don't create the full DFE when calling a DataFetcher that uses the new method signature.

If we take away looking at solely PropertyDataFetcher, would there be an advantage/disadvantage to introducing a isLeightWeight() method to DataFetcher instead of globally applying it via Backdoor?

The Backdoor is not meant to be permanent code. I created it PURELY so I could have a switch to use in a JMH benchmark. This allows me to test code apth X and then code path Y

If this PR got reworked to be accepted then this Backdoor goes away and we would call in via the new method and the old get(DFE env) method would be called via defaulting in the interface. Code (like PropertyDataFetcher) that is aware of the new signature could avoid the extra methed call by implementing the new signature

jord1e · 2022-09-19T12:08:50Z

Cool, almost exactly what I thought then 🙂. a ms is a ms

…ty_fetching # Conflicts: # src/main/java/graphql/schema/PropertyFetchingImpl.java

…ight

bbakerman · 2022-11-01T23:12:38Z

src/main/java/graphql/execution/ExecutionStrategy.java

- Supplier<ExecutableNormalizedField> normalizedFieldSupplier = getNormalizedField(executionContext, parameters, executionStepInfo);
+ Supplier<DataFetchingEnvironment> dataFetchingEnvironment = FpKit.intraThreadMemoize(() -> {

- // DataFetchingFieldSelectionSet and QueryDirectives is a supplier of sorts - eg a lazy pattern


DFE creation is now one big supplier

bbakerman · 2022-11-01T23:15:19Z

src/main/java/graphql/schema/LightDataFetcher.java

+ * and the related field will have a value of {@code null} in the result.
+ */
+ T get(GraphQLFieldDefinition fieldDefinition, Object sourceObject, Supplier<DataFetchingEnvironment> environmentSupplier) throws Exception;
+}


Added a new interface to keep Andi happy and to better delineate what the data fetcher does. Is it light or heavy?

Its MUST be a DataFetcher however because the CodeRegistry requires a DataFetcher when registered

…ty_fetching # Conflicts: # src/main/java/graphql/schema/PropertyFetchingImpl.java

bbakerman added 7 commits September 10, 2022 16:05

WIP - lightweight data fetchers

b05a2e9

WIP - lightweight data fetchers

dc93e0d

WIP - lightweight data fetchers - fixing tests - got rid of LightWeig…

0917386

…ht class

WIP - lightweight data fetchers - fixing tests - got rid of LightWeig…

755881c

…ht class -using switch

WIP - lightweight data fetchers - twizzling benchmark

c1f3a0f

WIP - lightweight data fetchers - twizzling benchmark

56f63ba

WIP - lightweight data fetchers - added Backdoor

0d80881

bbakerman commented Sep 10, 2022

View reviewed changes

WIP - lightweight data fetchers - the old impl was wrong on supplier

7331ae4

Lightweight data fetchers - removed back door

05061b3

bbakerman changed the title ~~WIP - DO NOT MERGE - lightweight data fetchers~~ Lightweight data fetchers Oct 18, 2022

bbakerman added this to the 20.0 milestone Oct 18, 2022

andimarek requested review from andimarek and dondonz October 24, 2022 23:33

dondonz approved these changes Oct 29, 2022

View reviewed changes

bbakerman added 2 commits November 2, 2022 07:35

Merge remote-tracking branch 'origin/master' into light_weight_proper…

45f3abf

…ty_fetching # Conflicts: # src/main/java/graphql/schema/PropertyFetchingImpl.java

Made a LightDataFetcher interface for specifying that you are lightwe…

324795d

…ight

bbakerman commented Nov 1, 2022

View reviewed changes

Revert DataFetcher changes

469723b

bbakerman commented Nov 1, 2022

View reviewed changes

bbakerman added 2 commits November 25, 2022 14:29

Merge remote-tracking branch 'origin/master' into light_weight_proper…

14e43af

…ty_fetching # Conflicts: # src/main/java/graphql/schema/PropertyFetchingImpl.java

Merged in works for Light Weight fetching

52c2623

bbakerman merged commit d75ab95 into master Nov 25, 2022

samuelAndalon mentioned this pull request Apr 15, 2023

feat: update graphql-java version ExpediaGroup/graphql-kotlin#1749

Merged

shalearkane mentioned this pull request Jun 21, 2024

Add LightOperationExecutor support for parameterless operations leangen/graphql-spqr#502

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Lightweight data fetchers #2953

Lightweight data fetchers #2953

Uh oh!

bbakerman commented Sep 10, 2022 •

edited

Loading

bbakerman Sep 10, 2022

bbakerman Sep 10, 2022 •

edited

Loading

bbakerman Sep 10, 2022

bbakerman Sep 10, 2022

bbakerman Sep 10, 2022

bbakerman Sep 10, 2022

bbakerman Sep 10, 2022

jord1e commented Sep 16, 2022

bbakerman commented Sep 19, 2022

jord1e commented Sep 19, 2022

bbakerman Nov 1, 2022

bbakerman Nov 1, 2022

Labels

4 participants

Uh oh!

Lightweight data fetchers #2953

Lightweight data fetchers #2953

Uh oh!

Conversation

bbakerman commented Sep 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

bbakerman Sep 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jord1e commented Sep 16, 2022

bbakerman commented Sep 19, 2022

jord1e commented Sep 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

4 participants

bbakerman commented Sep 10, 2022 •

edited

Loading

bbakerman Sep 10, 2022 •

edited

Loading