Description

Felk

opened on Nov 4, 2021

Issue body actions

This is basically a clone of an issue smallrye/smallrye-graphql#930 which I originally opened in SmallRye GraphQL, but it was suspected to be only relevant to graphql-java.

I encountered a performance issue when returning a large array of nested objects as response to a GraphQL query and was wondering whether it would be considered an actual performance issue or out of scope. Here is a minimal test class to illustrate the issue, using graphql-java 17.3 with java 17:

class GraphQLPerformanceTest {
    private static final List<SomeWrapper> objects = IntStream
            .range(0, 10_000_000)
            .mapToObj(i -> new SomeWrapper("value #" + i))
            .collect(Collectors.toList());

    @Value
    public static class SomeWrapper {
        String someValue;
    }

    private final GraphQL graphQL;

    public GraphQLPerformanceTest() {
        TypeDefinitionRegistry typeDefinitionRegistry = new SchemaParser().parse(
                """
                type Query {
                    giveMeLargeResponse: [SomeWrapper]
                }
                type SomeWrapper {
                    someValue: String
                }
                """
        );
        RuntimeWiring wiring = RuntimeWiring.newRuntimeWiring()
                .type(newTypeWiring("Query")
                        .dataFetcher("giveMeLargeResponse", env -> objects))
                .build();
        GraphQLSchema schema = new SchemaGenerator().makeExecutableSchema(typeDefinitionRegistry, wiring);
        this.graphQL = GraphQL.newGraphQL(schema).build();
    }

    @Test
    void testPerformance() {
        graphQL.execute(
            """
            query {
                giveMeLargeResponse {
                    someValue
                }
            }
            """
        );
    }
}

The request takes roughly 12s to return on my Dell precision 7560 notebook with an Intel(R) Core(TM) i9-11950H CPU @ 2.60GHz and intermittedly takes up over 4GB of RAM.

I know 10 million entries is a lot, but I expected the query to return much faster, given that it only needs to serialize and output the data. The majority of the time seems to be spent within completeValueForList:

The flame graph in IntelliJ looks something like this, so I couldn't see any obvious low hanging fruit:

I attached the intellij profiling data and the visualvm snapshot data:

GraphQLPerformanceTest_testPerformance_2021_11_04_111402.zip
graphql-java-performance-snapshot.zip

Reacted by Davide Angelocola

Activity

1. bbakerman commented on Nov 12, 2021

   

   bbakerman

   on Nov 12, 2021

   Member

   More actions

   graphql (and in some regards) requires that certain processing happens on results.

   For example given

   type Query {
                       giveMeLargeResponse: [SomeWrapper]
                   }          
   

   The engine is required to

   • take a list of arbitrary size
   • see that it is an object type
   • descend into its children for field subselection
   • for each child value "co-erce it into a value"
     • eg a int value must go through Int coercing
     • eg a String value must go through String coercing
     • repeat all this if its an object type child

   The graphql-java engine adds one more requirement

   • all of this MUST be asynchronous
   • because any field fetcher can in theory by an async operation
   • so we CompletableFuture .compose() together each of these values
   • we it may well be a synchronous value but we MUST make a CompleteableFuture from them so we can cope wheh there really is a CF that's async

   This is way more overhead than say " a REST call that just throws whatever it got back to a Jackson ObjectMapper and is done"

   In some ways this makes graphql a bad design choice for every large objects. There will always be more processing in graphql because each field could "lead" to a graph of data (in theory)... eg in theory your SomeWrapper. someValue could in fact be a call to an external system to get data and not a Java object property match. We dont know ahead of time so we have to execute each field as if it could lead to "complex" data

   Now with work we might be able to have some hint that a type is a pure Java map in memory and perhaps we could adjust the engine to "skip some of this processing and overhead" but this would be a lot of work and nominally fragile in how you tell us to "know that its all in memory / all synchronous and doesnt need co-ercing"

   That said - thats is defending the current engine. Thanks for reporting this because it food for thought on how the engine could be improved.

   ps. There is a graphql spec idea around @stream that nominally is a way to say that values can start to be sent back early however all of the async + coercion steps would apply so its not going to avoid the CPU overhead with the current engine.

   Reacted by Felix König
2. Felk commented on Nov 12, 2021

   

   Felk

   on Nov 12, 2021

   Author

   More actions

   Alright, thanks for the detailed insights! I agree that any potential improvements sound like they are not worth their complexity. In that sense I'll close this issue.
3. Felk

   closed this as completedon Nov 12, 2021
4. adrian-skybaker commented on Jun 15, 2022

   

   adrian-skybaker

   on Jun 15, 2022

   More actions

   Now with work we might be able to have some hint that a type is a pure Java map in memory and perhaps we could adjust the engine to "skip some of this processing and overhead"

   I think this is absolutely worth pursuing. Each time I profile the cpu or memory allocation our our GraphQL application (which is mostly serving cached in memory data), this overhead is very apparent.

   I would guess that async fetchers generally appear at fairly coarse grained points in the graph, and the vast majority of fields are just plain getters on the returned objects, or standard synchronous methods on resolvers. And the engine knows this immediately on startup.

   
5. floatdrop commented on Apr 19, 2025

   

   floatdrop

   on Apr 19, 2025

   More actions

   In some ways this makes graphql a bad design choice for every large objects.

   Well, even if we have large number of longs, that needs pagination, we will have huge performance penalty, because graphql-java does not check that object is instance of Page interface and much of checking can be omitted. That forces us to wrap List<Long> with some cursor besides it to implement pagination without penalty of nesting Long into node field.
6. bbakerman commented on Apr 20, 2025

   

   bbakerman

   on Apr 20, 2025

   Member

   More actions

   graphql-java does not check that object is instance of Page interface and much of checking can be omitted.

   I am not sure what this Page object type is but the graphql-java engine follows the graphql spec and Page is not a defined idea in the graphql specification.
7. bbakerman commented on Apr 20, 2025

   

   bbakerman

   on Apr 20, 2025

   Member

   More actions

   Since this issue get recommented I will comment on one thing I said a while back

   The graphql-java engine adds one more requirement :
   all of this MUST be asynchronous
   because any field fetcher can in theory by an async operation
   so we CompletableFuture .compose() together each of these values
   we it may well be a synchronous value but we MUST make a CompletableFuture from them so we can cope wheh there really is a CF that's async

   The above is no longer true in the second last version of graphql-java. If a returned object is not a CompletableFuture then the engine now will not wrap it in one. But rather it has a complicated (type unsafe) pattern of resolving polymorphic values that makes the code a smidge more difficult to reason about BUT is enormously more efficient.

   See https://github.com/graphql-java/graphql-java/releases/tag/v22.0 for more details on what changed

   I ran the above performance test (10 million objects with a simple object value) on v21 of graphql-java (the version before the polymorphic returned object fixes) and the fastest run on my machine (run 5 times) was 6.241S

   I then ran it on v23 of graphql-java and the fastest run on my machine (run 5 times) was 3.305S. Thats nearly twice as fast for that particular test. So thats great performance increase!

   That said, no matter how you slice it - graphql (with its super strong type system) is not a great choice for very large responses.

   Reacted by Vsevolod Strukchinsky
8. floatdrop commented on Apr 21, 2025

   

   floatdrop

   on Apr 21, 2025

   More actions

   I am not sure what this Page object type is but the graphql-java engine follows the graphql spec and Page is not a defined idea in the graphql specification.

   I was referring to Connection (which is generated for Page interface in graphql-spqr), my bad.

Issue actions