Graphql-laravel: Performance scaling issues

[dnyg]

In my project I have about 200 mutations, 200 queries and 400 types, and it looks like request performance degrades for every type you add.

I've created a repo to demonstrate: https://github.com/dnyg/graphql-laravel-performancetest

It contains a clean Laravel 5.8 installation with graphql-laravel installed. It has 5000 types and 5000 queries which all returns a simple hardcoded result (no database required for testing):

public function resolve($root, $args, $context, ResolveInfo $resolveInfo)
{
    return [
        'id' => 1
    ];
}

Obviously the retrospection call takes a long time, but the requests themselves are also painfully slow. The simple query below returns a hardcoded value, but it takes on average 1.93s to execute on my local machine.

{
  test {
    id
  }
}

If I remove the other queries and types, it executes in 162ms on average.

What can be done to resolve this issue to the application can scale while maintaining reasonable performance?

_The Xdebug profiler result hinted that the request spent 38% of the request time running the function buildObjectTypeFromClass in Rebing\GraphQL\GraphQL.php` in the original project - I imagine the number is much higher in the test-repo._

[mfn]

Repos are always awesome for reproducing 👍

Two things upfront:

• I tested with master, not v1 releases, which have some divergence (but nothing related to performance)
  
  
  
  • therefore some type adjustments needed to be made
  
  
• I've added a command to run said query as to have something easily to test from command line

    public function handle()
    {
        $graphql = <<<'GRAPHQL'
{
  test {
    id
  }
}
GRAPHQL;

        $result = \Rebing\GraphQL\Support\Facades\GraphQL::query($graphql);

        echo json_encode($result, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES), "\n"; exit;
    }

I've then ran this using PHP 7.3.5 (cli):
/usr/local/bin/php -dzend_extension=/usr/local/lib/php/pecl/20180731/xdebug.so -d xdebug.profiler_enable=1 -d xdebug.profiler_output_dir=./ artisan graphql:query

The cachegrind output is ~76MB uncompressed => cachegrind.out.29514.zip

In this scenario buildObjectTypeFromFields pops up, but it's not the only one: GraphQL\Type\Schema::__construct() also takes significant time:

I always wanted to explore the performance aspects, so thanks for making the first step with the repo!

I've never dealt with caching on webonxy. The docs over there only talk about caching in the context of the "Type language", i.e. a schema build from the DSL, which this library does not use.

Exciting challenges ahead I would say 😄

[mfn]

Created a PR https://github.com/dnyg/graphql-laravel-performancetest/pull/1 so everyone can easily test my mentioned approach.

[mfn]

Maybe http://webonyx.github.io/graphql-php/type-system/schema/#lazy-loading-of-types will help here.

Though it talks about lazy loading of types and the current indication is that the building of the many schemas is (already|also) a bottleneck.

[dnyg]

Thank you for your contribution to solve this issue 👍

I have approved and merged your PR on the test repo, and I will look into lazy loading of types.

[mfn]

In \Rebing\GraphQL\GraphQL::buildObjectTypeFromFields I tried to make 'fields' resolve as closure, i.e. something like this:

        $fnTypeFields = function () use ($fields) {
            $typeFields = [];
            foreach ($fields as $name => $field) {
                if (is_string($field)) {
                    $field = $this->app->make($field);
                    $name = is_numeric($name) ? $field->name : $name;
                    $field->name = $name;
                    $field = $field->toArray();
                } else {
                    $name = is_numeric($name) ? $field['name'] : $name;
                    $field['name'] = $name;
                }
                $typeFields[$name] = $field;
            }
        };

        return new ObjectType(array_merge([
            'fields' => $fnTypeFields,
        ], $opts));

But alas it didn't make any different; I also didn't give this attempt _too much_ thought, it was just something I could derive quickly but nothing changed.

[dnyg]

A colleague of mine managed to pull out a 30.64% performance improvement with these changes.

Unfortunately it caused a single test to fail, but looks promising!

[mfn]

Unrelated to the latency issue I just merged https://github.com/rebing/graphql-laravel/pull/389 into master which I was able to measure (slightly) lower memory use with such schemas/types as found in https://github.com/dnyg/graphql-laravel-performancetest

I still consider it experimental and haven't it given much testing outside the test suite, so if you upgrade and it breaks YMMV :-)

[mfn]

Do we think this is sufficiently addressed with https://github.com/rebing/graphql-laravel/pull/405 ?

[dnyg]

In the test-repo, updating to the latest release and setting lazyload_types to true improves the request time by 10.81%

This is an improvement, but still far from the expected performance. The request still takes 1721ms to execute on my machine.

I will look into the XDebug profiling to pinpoint the potential issue later

[mfn]

This number seems low compared to what I think I saw.

When look back what I wrote https://github.com/rebing/graphql-laravel/pull/394#pullrequestreview-259904338 I would say a it should be more like 15-20%?

But maybe that difference between 10% in your tests and 15% in mine is negligible and the same.

[mfn]

Here are _my_ current numbers with https://github.com/dnyg/graphql-laravel-performancetest/pull/2 and APP_DEBUG=false

 $ until false; do php artisan graphql:query; done
758ms
753ms
773ms
762ms
734ms
791ms
748ms
793ms
768ms
748ms
^C

[mfn]

When manually disabling lazyload_types, I see this

 $ until false; do php artisan graphql:query; done
1382ms
1013ms
1012ms
1011ms
988ms
998ms
1013ms
1021ms
1097ms
^C

Let's make nice rounded numbers for easy comparison:

• lazyload_types=true => ~750ms
• lazyload_types=false => ~1000ms

That seems to be ~25% faster.

But, yes, in these tests it's still 750ms…

[mfn]

From my understanding with graphql-php, I don't think it's possible to fix. I would be glad to be proven wrong though 😄

@crissi implemented a "type loader" and it works as expected: types are lazy loaded.

What were are left with in the performance repo is having a schema definition with 5k queries.

But, at least with graphql-php and what I understand: you cannot lazy load the queries for a schema, at least not individually. What I did in https://github.com/rebing/graphql-laravel/issues/390#issuecomment-509024935 is providing a lazy loader for the _whole_ schema.

But since there's only one schema in the example and it's the one we need a query from, the outcome is the same.

If you you comment out 4999 of the queries, the benchmark time drop down to ~10ms for me (after warming up).

See also https://webonyx.github.io/graphql-php/type-system/schema/ , this is what was done:

$schema = new Schema([
    'query' => $registry->get('Query'),
    'typeLoader' => function($name) use ($registry) {
        return $registry->get($name);
    }
]);

Now, this example talks about 'query' => $registry->get('Query') but doesn't give an actual example what is returns. The Types ($registry) features however this:

    private function MyTypeA()
    {
        return new ObjectType([
            'name' => 'MyTypeA',
            'fields' => function() {
                return [
                    'b' => ['type' => $this->get('MyTypeB')]
                ];
            }
        ]);
    }

Witness that fields is defined as function () {…} and this is how you could also do it for the 5k queries but it doesn't solve any problem because _within_ it, it still has to return all 5k query types, similar to this part of the closure:

                return [
                    'b' => ['type' => $this->get('MyTypeB')]
                ];

but 'type' isn't a function (and it's also not supported) so lazy loading at this place isn't possible.

Well maybe I got it all backwards but currently doesn't seem so to me.

[dnyg]

_Disclamer: All the measurements that I've made are made in development mode with APP_DEBUG=true and without caching config or optimized composer autoloading._

I just looked through the Xdebug profiler cachegrind for the test-repo, and it looks like lazy loading the types has shifted all the attention away from getTypeMap and on to buildObjectTypeFromFields which is now responsible for well over 80% of the request time.

It doesn't look like there's any single operation within this function that causes it to be slow. It's just slow because there are so many queries.

(I did notice, however, that replacing $field = $this->app->make($field) with $field = new $field did improve performance with 5.29%)

I will look into Webonyx code to see if I can figure out whether their example is misleading, or there actually is a way to lazy-load queries.

As for resolving the issue; The issue was never about types - it was about performance. I'm very happy with the performance increase caused by #405 but I think we should ask ourselves: What is a reasonable time for an API to return a hardcoded string of 28 bytes ({"data":{"test":{"id":"1"}}})?

The number you mentioned when commenting out the additional queries (10ms) seems reasonable to me.

[mfn]

I made a PoC hack showing that when I _pretend_ the webonxy library would be able to lazy evaluate a field within the fields, the time goes down from ~750ms to ~20ms:

 $ until false; do php artisan graphql:query; done
21ms
20ms
20ms
20ms
19ms
20ms
20ms
21ms
20ms
^C

No idea what else it breaks but at least this sample works.
(also AFAIK it's wrong to return new instances from these calls but rather should treat them as singletons 🤷‍♀️)

See https://github.com/mfn/graphql-laravel-performancetest/pull/1/files

[mfn]

_(I did notice, however, that replacing $field = $this->app->make($field) with $field = new $field did improve performance with 5.29%)_

I think this is also something worth exploring.

Not using make could fix two things:

• the performance as outlined above
• people are (rightly so!) using the types constructor for DI. But given that all the queries and (lazy) types are built even for cases were the respective resolvers might not run, it's easily a waste of resources

I'm leaning towards changing this in a future version:

• only provide the Container contract to the constructor
• any resolver still can use ->app->make()
• (_maybe_) provide helpers like ->makeOnce() with an internal cache to prevent, when having many resolvers using the same dependency, constantly having go through the framework again

[crissi]

I made a PoC hack showing that when I _pretend_ the webonxy library would be able to lazy evaluate a field within the fields, the time goes down from ~750ms to ~20ms:

 $ until false; do php artisan graphql:query; done
21ms
20ms
20ms
20ms
19ms
20ms
20ms
21ms
20ms
^C

No idea what else it breaks but at least this sample works.
(also AFAIK it's wrong to return new instances from these calls but rather should treat them as singletons 🤷‍♀️)

See https://github.com/mfn/graphql-laravel-performancetest/pull/1/files

did you look at this closer for a complete solution?

[mfn]

did you look at this closer for a complete solution?

Not really, lacking the time for digging deeper, understanding this better and bringing this to webonxy attention.