Closed dnyg closed 3 years ago
Repos are always awesome for reproducing 👍
Two things upfront:
master
, not v1 releases, which have some divergence (but nothing related to performance)
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 😄
Created a PR https://github.com/dnyg/graphql-laravel-performancetest/pull/1 so everyone can easily test my mentioned approach.
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.
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.
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.
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!
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 :-)
Do we think this is sufficiently addressed with https://github.com/rebing/graphql-laravel/pull/405 ?
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
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.
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
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
=> ~750mslazyload_types=false
=> ~1000msThat seems to be ~25% faster.
But, yes, in these tests it's still 750ms…
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.
_Disclamer: All the measurements that I've made are made in development mode with APPDEBUG=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.
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
(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:
I'm leaning towards changing this in a future version:
Container
contract to the constructor->app->make()
->makeOnce()
with an internal cache to prevent, when having many resolvers using the same dependency, constantly having go through the framework againI 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?
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.
Update time!
No PR yet, just fooling around with https://github.com/mfn/graphql-laravel-performancetest/tree/mfn-octane , where in the end I'm using L8/PHP8/octane+roadrunner
Disclaimer: this is naive testing, don't cite any of this anywhere as some kind of benchmark revelation blabla.
ab
which seems to be default on OSX?)http://127.0.0.1:8000/graphql?query=%7B%0A%20%20test%20%7B%0A%20%20%20%20id%0A%20%20%7D%0A%7D
(which returns { "data": { "test": { "id": "1" } } }
)APP_ENV=production
and APP_DEBUG=false
as well as running php artisan optimize
php artisan serve
and php artisan octane:start
, neither which is meant for productionThe following ones are based on the commit https://github.com/mfn/graphql-laravel-performancetest/commit/95bf435b3e42aabffa406304b561291113b1cb64 (no PHP8 required, no octane, because I can't run 7.4 with the latest head of the branch)
TL;DR:
Time taken for tests: 57.208 seconds
Requests per second: 1.75 [#/sec] (mean)
Time per request: 572.077 [ms] (mean)
TL;DR:
Time taken for tests: 22.526 seconds
Requests per second: 4.44 [#/sec] (mean)
Time per request: 225.260 [ms] (mean)
TL;DR:
Time taken for tests: 58.474 seconds
Requests per second: 1.71 [#/sec] (mean)
Time per request: 584.741 [ms] (mean)
TL;DR:
Time taken for tests: 21.395 seconds
Requests per second: 4.67 [#/sec] (mean)
Time per request: 213.949 [ms] (mean)
From here one I'm using PHP 8 only with https://github.com/mfn/graphql-laravel-performancetest/commit/aa1989123dbb70fffb765dcc8568be917ea6da95
TL;DR:
Time taken for tests: 19.025 seconds
Requests per second: 5.26 [#/sec] (mean)
Time per request: 190.247 [ms] (mean)
TL;DR:
Time taken for tests: 8.548 seconds
Requests per second: 11.70 [#/sec] (mean)
Time per request: 85.478 [ms] (mean)
TL;DR:
Time taken for tests: 18.894 seconds
Requests per second: 5.29 [#/sec] (mean)
Time per request: 188.942 [ms] (mean)
TL;DR:
Time taken for tests: 8.598 seconds
Requests per second: 11.63 [#/sec] (mean)
Time per request: 85.984 [ms] (mean)
Well, I was once more thinking about how to improve the performance of the library and caching etc. once I got initial octane compatibility problems solved in https://github.com/rebing/graphql-laravel/pull/755 , I already saw the performance it brings and did some more structured ~benchmarking~ testing here.
IMHO it shows how a different execution architecture almost negates the practical need for performance improvements. That is, I believe with having such options like octane available in the future, we'll see new approaches to solve performance issues, like I described here.
Unfortunately I don't have the PoC from https://github.com/rebing/graphql-laravel/issues/390#issuecomment-513580974 anymore, which OTOH also clearly shows how much more performance can be squeezes out iff the graphql-php would have the lazy load capability not only for types, but the schema queries/mutations itself.
However, after discovering in which direction the future of PHP can go here with octane+roadrunner/etc. I feel less motivated digging into graphql-php for this; also I lack quite the expertise for this and to me I see a "high level" solution here.
This might not be a popular action, but I'm closing this issue now (discussion can of course continue).
Using graphql-php is a fundamental choice and I think at this point it's clear that bottlenecks are to be found in the reference implementation and not here. Also don't twist my words, graphql-php is awesome and is super fast IMHO, let's be realistic we're talking about a synthetic edge case also here with the 5k queries in a single schema so I'm not saying it's another libraries fault.
But I also think we're clearly having options in the future available here and I see it beneficial to make sure graphql-laravel plays well with octane => https://github.com/rebing/graphql-laravel/issues/756
✌️
@mfn does the php artisan optimize command do nothing since laravel 5.5?
https://laravel-news.com/laravel-5-6-removes-artisan-optimize
@tm1000 you can check it yourself, there still is an optimize command and it "does stuff".
IDK about 5.5, that's quite old.
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):
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.
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.