perf: improve data loading

glesys · Mar 11, 2021 · 893eb4d · 893eb4d
1 parent 73b946d
commit 893eb4d
Show file tree

Hide file tree

Showing 14 changed files with 265 additions and 261 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 - **BREAKING**: Strict check for expected value in `assertPromiseFulfills`.
+- **BREAKING**: Improved performance when using data loaders with a tailored implementation based on `amphp/amp` instead of `leinonen/php-dataloader`. See [UPGRADE.md](UPGRADE.md) for details on how to upgrade.
+
 
 ## [4.0.0] - 2020-12-10
 

diff --git a/README.md b/README.md
@@ -228,15 +228,16 @@ class Article
     public function comments(Article $source, $args, $context, $info)
     {
         return $context['loader'](function ($articleIds) {
-            $comments = Comment::whereIn('article_id', $articleIds)->get();
-            return collect($articleIds)->map(function ($articleId) use ($comments) {
-                return $comments->where('article_id', $articleId);
-            });
-        })->load($source->id);
+            return Comment::whereIn('article_id', $articleIds)
+                ->cursor()
+                ->groupBy('article_id');
+        }, [])->load($source->id);
     }
 }
 ```
 
+_NOTE:_ You can provide a default value as the second argument to the loader factory. This can be useful when you know some Articles might not have any Comments, like in the example above. Defaults to `null`.
+
 #### Shared Data Loaders
 
 If you have multiple resolvers working with the same underlying data you don't need to duplicate your code or deal with extra round trips to the database.
@@ -255,26 +256,23 @@ class Article
 {
     public function comments(Article $source, $args, $context, $info)
     {
-        return $context['loader'](Closure::fromCallable([$this, 'loadComments']))
+        return $context['loader'](Closure::fromCallable([$this, 'loadComments']), [])
             ->load($source->id);
     }
 
     public function topVotedComment(Article $source, $args, $context, $info)
     {
-        return $context['loader'](Closure::fromCallable([$this, 'loadComments']))
-            ->load($source->id)
-            ->then(function ($articleComments) {
-                return collect($articleComments)->sortByDesc('votes')->first();
-            });
+        $comments = yield $context['loader'](Closure::fromCallable([$this, 'loadComments']))
+            ->load($source->id);
+
+        return collect($comments)->sortByDesc('votes')->first();
     }
 
     private function loadComments($articleIds)
     {
-        $comments = Comment::whereIn('article_id', $articleIds)->get();
-
-        return collect($articleIds)->map(function ($articleId) use ($comments) {
-            return $comments->where('article_id', $articleId);
-        });
+        return Comment::whereIn('article_id', $articleIds)
+            ->cursor()
+            ->groupBy('article_id');
     }
 }
 ```

diff --git a/UPGRADE.md b/UPGRADE.md
@@ -0,0 +1,46 @@
+## Upgrade from v4 to v5
+
+### BREAKING: Promises are based on `amphp/amp` instead of `react/promise`
+
+If you have any resolvers waiting for promises using the `->then()` method you need to replace these with the `yield` keyword.
+
+**Example:**
+
+```php
+public function topVotedComment(Article $source, $args, $context, $info)
+{
+    return $context['loader'](Closure::fromCallable([$this, 'loadComments']))
+        ->load($source->id)
+        ->then(function ($articleComments) {
+            return collect($articleComments)->sortByDesc('votes')->first();
+        });
+}
+```
+
+to
+
+```php
+public function topVotedComment(Article $source, $args, $context, $info)
+{
+    $comments = yield $context['loader'](Closure::fromCallable([$this, 'loadComments']))
+        ->load($source->id);
+
+    return collect($comments)->sortByDesc('votes')->first();
+}
+```
+
+### BREAKING: The signature of `Butler\Graphql\DataLoader@__construct` has changed
+
+If you manually instantiate `Butler\Graphql\DataLoader` anywhere you need to update your code. It's no longer necessary to provide a `React\EventLoop\LoopInterface` to the constructor.
+
+**Example:**
+
+```php
+$dataLoader = new DataLoader($this->getLoop());
+```
+
+to
+
+```php
+$dataLoader = new DataLoader();
+```
diff --git a/composer.json b/composer.json
@@ -41,8 +41,8 @@
     "name": "glesys/butler-graphql",
     "require": {
         "php": "^7.2.5|^8.0",
+        "amphp/amp": "^2.5",
         "illuminate/support": "5.6.*|5.7.*|5.8.*|^6.0|^7.0|^8.0",
-        "leinonen/php-dataloader": "^1.0",
         "webonyx/graphql-php": "^14.3.0"
     },
     "require-dev": {

diff --git a/src/CacheMap.php b/src/CacheMap.php
diff --git a/src/Concerns/AssertsPromises.php b/src/Concerns/AssertsPromises.php
@@ -2,34 +2,36 @@
 
 namespace Butler\Graphql\Concerns;
 
+use Amp\Loop;
 use Closure;
 use Exception;
-use React\EventLoop\Factory as LoopFactory;
-use React\EventLoop\LoopInterface;
-use React\Promise\PromiseInterface;
 
-use function React\Promise\all;
+use function Amp\call;
+use function Amp\Promise\all;
 
 trait AssertsPromises
 {
-    private $loop;
-
     /**
-     * @param  \React\Promise\PromiseInterface|array  $promise
+     * @param  \Amp\Promise|\React\Promise\PromiseInterface|mixed  $promise
      * @param  mixed|Closure  $expectedValue
      */
     public function assertPromiseFulfills($promise, $expectedValue = null): void
     {
         $this->addToAssertionCount(1);
 
-        if (! $promise instanceof PromiseInterface) {
+        if (is_array($promise)) {
             $promise = all($promise);
         }
 
         try {
-            $result = $this->waitForPromise($promise);
-        } catch (Exception $_) {
-            $this->fail('Failed asserting that promise fulfills. Promise was rejected.');
+            $result = null;
+            Loop::run(function () use (&$result, $promise) {
+                $result = yield call(function () use ($promise) {
+                    return $promise;
+                });
+            });
+        } catch (Exception $e) {
+            $this->fail('Failed asserting that promise fulfills. Promise was rejected: ' . $e->getMessage());
         }
 
         if ($expectedValue instanceof Closure) {
@@ -45,58 +47,4 @@ public function assertPromiseFulfills($promise, $expectedValue = null): void
             );
         }
     }
-
-    protected function getLoop(): LoopInterface
-    {
-        if (! $this->loop) {
-            $this->loop = LoopFactory::create();
-        }
-
-        return $this->loop;
-    }
-
-    private function waitForPromise(PromiseInterface $promise)
-    {
-        $wait = true;
-        $resolved = null;
-        $exception = null;
-        $rejected = false;
-
-        $promise->then(
-            function ($c) use (&$resolved, &$wait) {
-                $resolved = $c;
-                $wait = false;
-                $this->getLoop()->stop();
-            },
-            function ($error) use (&$exception, &$rejected, &$wait) {
-                $exception = $error;
-                $rejected = true;
-                $wait = false;
-                $this->getLoop()->stop();
-            }
-        );
-
-        // Explicitly overwrite argument with null value. This ensure that this
-        // argument does not show up in the stack trace in PHP 7+ only.
-        $promise = null;
-
-        while ($wait) {
-            $this->getLoop()->run();
-        }
-
-        if ($rejected) {
-            if (! $exception instanceof Exception) {
-                $type = is_object($exception) ? get_class($exception) : gettype($exception);
-                $exception = new \UnexpectedValueException(
-                    'Promise rejected with unexpected value of type ' . $type,
-                    0,
-                    $exception instanceof \Throwable ? $exception : null
-                );
-            }
-
-            throw $exception;
-        }
-
-        return $resolved;
-    }
 }
diff --git a/src/Concerns/HandlesGraphqlRequests.php b/src/Concerns/HandlesGraphqlRequests.php
@@ -20,8 +20,13 @@
 use Illuminate\Support\Str;
 use Illuminate\Validation\ValidationException;
 
+use function Amp\call;
+
 trait HandlesGraphqlRequests
 {
+    private $classCache;
+    private $namespaceCache;
+
     /**
      * Invoke the Graphql request handler.
      *
@@ -30,6 +35,9 @@ trait HandlesGraphqlRequests
      */
     public function __invoke(Request $request)
     {
+        $this->classCache = [];
+        $this->namespaceCache = null;
+
         $loader = app(DataLoader::class);
 
         $schema = BuildSchema::build($this->schema(), [$this, 'decorateTypeConfig']);
@@ -64,9 +72,7 @@ public function errorFormatter(GraphqlError $graphqlError)
         $throwable = $graphqlError->getPrevious();
 
         $this->reportException(
-            $throwable instanceof Exception
-            ? $throwable
-            : $graphqlError
+            $throwable instanceof Exception ? $throwable : $graphqlError
         );
 
         if ($throwable instanceof ModelNotFoundException) {
@@ -138,9 +144,11 @@ public function resolveField($source, $args, $context, ResolveInfo $info)
             ?? $this->fieldFromArray($source, $args, $context, $info)
             ?? $this->fieldFromObject($source, $args, $context, $info);
 
-        return $field instanceof \Closure
-            ? $field($source, $args, $context, $info)
-            : $field;
+        return call(static function () use ($field, $source, $args, $context, $info) {
+            return $field instanceof \Closure
+                ? $field($source, $args, $context, $info)
+                : $field;
+        });
     }
 
     public function resolveType($source, $context, ResolveInfo $info)
@@ -156,8 +164,7 @@ public function fieldFromResolver($source, $args, $context, ResolveInfo $info)
         $className = $this->resolveClassName($info);
         $methodName = $this->resolveFieldMethodName($info);
 
-        if (app()->has($className) || class_exists($className)) {
-            $resolver = app($className);
+        if ($resolver = $this->make($className)) {
             if (method_exists($resolver, $methodName)) {
                 return $resolver->{$methodName}($source, $args, $context, $info);
             }
@@ -211,8 +218,7 @@ public function typeFromParentResolver($source, $context, ResolveInfo $info)
         $className = $this->resolveClassName($info);
         $methodName = $this->resolveTypeMethodName($info);
 
-        if (app()->has($className) || class_exists($className)) {
-            $resolver = app($className);
+        if ($resolver = $this->make($className)) {
             if (method_exists($resolver, $methodName)) {
                 return $resolver->{$methodName}($source, $context, $info);
             }
@@ -268,7 +274,7 @@ public function resolveTypeMethodName(ResolveInfo $info): string
 
     public function namespace(): string
     {
-        return config('butler.graphql.namespace');
+        return $this->namespaceCache ?? $this->namespaceCache = config('butler.graphql.namespace');
     }
 
     public function queriesNamespace(): string
@@ -293,4 +299,17 @@ public function decorateResponse(array $data): array
         }
         return $data;
     }
+
+    protected function make(string $className)
+    {
+        if (array_key_exists($className, $this->classCache)) {
+            return $this->classCache[$className];
+        }
+
+        $class = app()->has($className) || class_exists($className)
+            ? app($className)
+            : null;
+
+        return $this->classCache[$className] = $class;
+    }
 }