Singleton property data fetcher #3754

bbakerman · 2024-11-25T05:01:18Z

This adds support for a singleton property data fetcher

Before we create a new PropertyDataFetcher() for every defaulted field in the schema.

This means for large schemas with 1000s of fields, we have a new object per field that ALWAYS fetches values with the name of the field.

This can be static and hence we can save memory by default since MOST fields can use this static fetcher

…e it

github-actions · 2024-11-25T05:05:06Z

Test Results

305 files + 1 305 suites +1 45s ⏱️ +5s
3 460 tests +23 3 455 ✅ +23 5 💤 ±0 0 ❌ ±0
3 549 runs +23 3 544 ✅ +23 5 💤 ±0 0 ❌ ±0

Results for commit b13b7c8. ± Comparison against base commit f83ba25.

This pull request removes 168 and adds 169 tests. Note that renamed tests count towards both.

	?
                __schema { types { fields { args { type { name fields { name }}}}}}
                __schema { types { fields { type { name fields { name }}}}}
                __schema { types { inputFields { type { inputFields { name }}}}}
                __schema { types { interfaces { fields { type { interfaces { name } } } } } }
                __schema { types { name} }
                __type(name : "t") { name }
                a1: __schema { types { name} }
                a1: __type(name : "t") { name }
                a2 :  __type(name : "t1") { name }
…

graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure23@5942ee04 delegate=graphql.AssertTest@6622a690 owner=graphql.AssertTest@6622a690 thisObject=graphql.AssertTest@6622a690 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure24@5a865416 delegate=graphql.AssertTest@6622a690 owner=graphql.AssertTest@6622a690 thisObject=graphql.AssertTest@6622a690 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure25@14faa38c delegate=graphql.AssertTest@6622a690 owner=graphql.AssertTest@6622a690 thisObject=graphql.AssertTest@6622a690 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure20@4d6f623d delegate=graphql.AssertTest@6622a690 owner=graphql.AssertTest@6622a690 thisObject=graphql.AssertTest@6622a690 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure21@e93f3d5 delegate=graphql.AssertTest@6622a690 owner=graphql.AssertTest@6622a690 thisObject=graphql.AssertTest@6622a690 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure22@7a26928a delegate=graphql.AssertTest@6622a690 owner=graphql.AssertTest@6622a690 thisObject=graphql.AssertTest@6622a690 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure3@5f212d84 delegate=graphql.AssertTest@6622a690 owner=graphql.AssertTest@6622a690 thisObject=graphql.AssertTest@6622a690 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure4@2f4919b0 delegate=graphql.AssertTest@6622a690 owner=graphql.AssertTest@6622a690 thisObject=graphql.AssertTest@6622a690 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure5@75b21c3b delegate=graphql.AssertTest@6622a690 owner=graphql.AssertTest@6622a690 thisObject=graphql.AssertTest@6622a690 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertNotNull with different number of error args with non null does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_6prov0_closure6@deb3b60 delegate=graphql.AssertTest@6622a690 owner=graphql.AssertTest@6622a690 thisObject=graphql.AssertTest@6622a690 resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0]
…

♻️ This comment has been updated with latest results.

bbakerman · 2024-11-25T05:08:35Z

src/main/java/graphql/schema/GraphQLCodeRegistry.java

@@ -189,7 +189,7 @@ public static class Builder {
        private final Map<String, DataFetcherFactory<?>> systemDataFetcherMap = new LinkedHashMap<>();
        private final Map<String, TypeResolver> typeResolverMap = new HashMap<>();
        private GraphqlFieldVisibility fieldVisibility = DEFAULT_FIELD_VISIBILITY;
-        private DataFetcherFactory<?> defaultDataFetcherFactory = env -> PropertyDataFetcher.fetching(env.getFieldDefinition().getName());
+        private DataFetcherFactory<?> defaultDataFetcherFactory = PropertyDataFetcher.singletonFactory();


This causes the same instance of the "default" DF to be used

bbakerman · 2024-11-25T05:09:47Z

src/main/java/graphql/schema/PropertyDataFetcher.java

+    private PropertyDataFetcher() {
+        this.function = null;
+        this.propertyName = null;
+    }


we no longer need the property name but its kept allow older code to work that might use direct names or functions

…diff reasons

bbakerman · 2024-11-25T05:12:09Z

src/test/groovy/graphql/DataFetcherTest.groovy

-        newDataFetchingEnvironment().source(dataHolder).fieldType(type).build()
+    def env(String propertyName, GraphQLOutputType type) {
+        def fieldDefinition = GraphQLFieldDefinition.newFieldDefinition().name(propertyName).type(type).build()
+        newDataFetchingEnvironment().source(dataHolder).fieldType(type).fieldDefinition(fieldDefinition).build()


a valid DataFetchingEnvironment will always have a field definition

…mocks

…Helper that does NOT put in an entry for defaulted values

…Helper that does NOT put in an entry for defaulted values- DFF tweaked

…a SingletonPropertyDataFetcher class

…efault # Conflicts: # src/main/java/graphql/schema/PropertyDataFetcher.java

bbakerman · 2024-11-26T04:04:38Z

src/main/java/graphql/schema/GraphQLCodeRegistry.java

@@ -189,7 +189,7 @@ public static class Builder {
        private final Map<String, DataFetcherFactory<?>> systemDataFetcherMap = new LinkedHashMap<>();
        private final Map<String, TypeResolver> typeResolverMap = new HashMap<>();
        private GraphqlFieldVisibility fieldVisibility = DEFAULT_FIELD_VISIBILITY;
-        private DataFetcherFactory<?> defaultDataFetcherFactory = env -> PropertyDataFetcher.fetching(env.getFieldDefinition().getName());
+        private DataFetcherFactory<?> defaultDataFetcherFactory = SingletonPropertyDataFetcher.singletonFactory();


key change here - no object allocated per field

bbakerman · 2024-11-26T04:05:50Z

src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java

-        String fieldName = environment.getFieldDefinition().getName();
-        return new PropertyDataFetcher(fieldName);
+    private DataFetcher<?> dataFetcherOfLastResort() {
+        return SingletonPropertyDataFetcher.singleton();


This code is not actually used by default - unless GraphqlCodeRegistry had a null default DDF

bbakerman · 2024-11-26T04:06:19Z

src/test/groovy/graphql/DataFetcherTest.groovy

+        when:
+        result = fetcher.get(field, dataHolder, { environment })
+        then:
+        result == "propertyValue"


extra tests for light weight data fetching

boils down to the same code however in the end

bbakerman · 2024-11-26T04:37:29Z

src/main/java/graphql/schema/DataFetcherFactories.java

@@ -32,7 +43,7 @@ public static <T> DataFetcherFactory<T> useDataFetcher(DataFetcher<T> dataFetche
     *
     * @return a new data fetcher that wraps the provided data fetcher
     */
-    public static DataFetcher wrapDataFetcher(DataFetcher delegateDataFetcher, BiFunction<DataFetchingEnvironment, Object, Object> mapFunction) {
+    public static DataFetcher<?> wrapDataFetcher(DataFetcher<?> delegateDataFetcher, BiFunction<DataFetchingEnvironment, Object, Object> mapFunction) {


small clean up - generics!

…tcher per property per graphql operation (#2079) ### 📝 Description Inspired by graphql-java/graphql-java#3754. Currently, graphql-kotlin, through the [KotlinDataFetcherFactoryProvider](https://github.com/ExpediaGroup/graphql-kotlin/blob/master/generator/graphql-kotlin-schema-generator/src/main/kotlin/com/expediagroup/graphql/generator/execution/KotlinDataFetcherFactoryProvider.kt#L62) creates a [PropertyDataFetcher](https://github.com/ExpediaGroup/graphql-kotlin/blob/master/generator/graphql-kotlin-schema-generator/src/main/kotlin/com/expediagroup/graphql/generator/execution/PropertyDataFetcher.kt) per source's property. This instance is created [every single time ](https://github.com/graphql-java/graphql-java/blob/master/src/main/java/graphql/schema/GraphQLCodeRegistry.java#L100)the graphql-java [DataFetcherFactory](https://github.com/graphql-java/graphql-java/blob/master/src/main/java/graphql/schema/DataFetcherFactory.java) is invoked, which happens to be on runtime per property per graphql-operation. This PR will introduce a new object class `SingletonPropertyDataFetcher` which will host its own singleton factory that will always return `SingletonPropertyDataFetcher` which will store all `KProperty.Getter<*>`s in a ConcurrentHashMap. Instead of just replacing the SimpleKotlinDataFetcherFactoryProvider, I am creating a new one, to avoid breaking changes, and to allow users to decide what they want, this switch might come with a cost, we are avoiding object allocations, in favor of a singleton that will possibly hold thousands of `KProperty.Getter<*>`s. --------- Co-authored-by: Samuel Vazquez <samvazquez@expediagroup.com>

bbakerman added 2 commits November 25, 2024 15:44

Use a singleton PropertyFetcher by default

689e401

Use a singleton PropertyFetcher by default - schema generator will us…

a5a22c0

…e it

bbakerman added this to the 23.x breaking changes milestone Nov 25, 2024

bbakerman commented Nov 25, 2024

View reviewed changes

Use a singleton PropertyFetcher by default - renamed back to env for …

0d80213

…diff reasons

bbakerman commented Nov 25, 2024

View reviewed changes

bbakerman added 6 commits November 25, 2024 16:17

Use a singleton PropertyFetcher by default - tweaked test wiring

612084e

Use a singleton PropertyFetcher by default - tweaked test wiring and …

b8bfd4b

…mocks

Use a singleton PropertyFetcher by default - now with SchemaGenerator…

70bac09

…Helper that does NOT put in an entry for defaulted values

Use a singleton PropertyFetcher by default - now with SchemaGenerator…

b8b2e26

…Helper that does NOT put in an entry for defaulted values- DFF tweaked

Use a singleton PropertyFetcher by default - broke out the code into …

424cac7

…a SingletonPropertyDataFetcher class

Merge branch 'singeton_property_data_fetcher' into no_df_if_codereg_d…

d1b132a

…efault # Conflicts: # src/main/java/graphql/schema/PropertyDataFetcher.java

bbakerman commented Nov 26, 2024

View reviewed changes

bbakerman added 3 commits November 26, 2024 15:09

Use a singleton PropertyFetcher by default - tweaked tests

886d745

Use a singleton PropertyFetcher by default - tweaked tests moar

9ca4d30

Use a singleton PropertyFetcher by default - deprecated DFF method

b13b7c8

bbakerman commented Nov 26, 2024

View reviewed changes

bbakerman added this pull request to the merge queue Nov 26, 2024

Merged via the queue into master with commit 19d128c Nov 26, 2024
2 checks passed

dondonz added the performance work that is primarily targeted as performance improvements label Nov 27, 2024

samuelAndalon mentioned this pull request Apr 10, 2025

feat: SingletonPropertyDataFetcher, avoid allocating a PropertyDataFetcher per property per graphql operation ExpediaGroup/graphql-kotlin#2079

Merged

This was referenced Apr 29, 2025

provide a SimpleDataFetcherFactory to avoid creating a DataFetcherFactoryEnvironment per getDataFetcher call #3941

Open

Retriveing singleton property fetchers will not create DataFetcherFactoryEnvironment objects #3942

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Singleton property data fetcher #3754

Singleton property data fetcher #3754

bbakerman commented Nov 25, 2024

github-actions bot commented Nov 25, 2024 •

edited

Loading

bbakerman Nov 25, 2024

bbakerman Nov 25, 2024

bbakerman Nov 25, 2024

bbakerman Nov 26, 2024

bbakerman Nov 26, 2024

bbakerman Nov 26, 2024

bbakerman Nov 26, 2024

bbakerman Nov 26, 2024

Singleton property data fetcher #3754

Singleton property data fetcher #3754

Conversation

bbakerman commented Nov 25, 2024

github-actions bot commented Nov 25, 2024 • edited Loading

Test Results

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 25, 2024 •

edited

Loading