From b914dd4e446876d583c61f92e0239007a58124b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Tue, 11 Dec 2018 16:23:23 +0100 Subject: [PATCH 1/2] Improving performance by caching parsed docblocks --- src/ControllerQueryProvider.php | 54 +++------- src/ControllerQueryProviderFactory.php | 12 ++- src/Reflection/CachedDocBlockFactory.php | 100 ++++++++++++++++++ src/Reflection/CommentParser.php | 73 ------------- tests/AbstractQueryProviderTest.php | 8 +- tests/ControllerQueryProviderTest.php | 11 +- tests/Integration/EndToEndTest.php | 8 +- .../Reflection/CachedDocBlockFactoryTest.php | 28 +++++ tests/Reflection/CommentParserTest.php | 23 ---- 9 files changed, 173 insertions(+), 144 deletions(-) create mode 100644 src/Reflection/CachedDocBlockFactory.php delete mode 100644 src/Reflection/CommentParser.php create mode 100644 tests/Reflection/CachedDocBlockFactoryTest.php delete mode 100644 tests/Reflection/CommentParserTest.php diff --git a/src/ControllerQueryProvider.php b/src/ControllerQueryProvider.php index a48893c..36c85af 100644 --- a/src/ControllerQueryProvider.php +++ b/src/ControllerQueryProvider.php @@ -3,23 +3,15 @@ namespace TheCodingMachine\GraphQL\Controllers; -use function array_map; use function array_merge; -use function get_class; -use function gettype; -use GraphQL\Type\Definition\InputType; use GraphQL\Type\Definition\OutputType; use phpDocumentor\Reflection\Fqsen; use phpDocumentor\Reflection\Types\Nullable; use phpDocumentor\Reflection\Types\Self_; -use phpDocumentor\Reflection\Types\Static_; use Psr\Container\ContainerInterface; use ReflectionMethod; -use TheCodingMachine\GraphQL\Controllers\Annotations\Exceptions\ClassNotFoundException; -use TheCodingMachine\GraphQL\Controllers\Mappers\RecursiveTypeMapper; -use TheCodingMachine\GraphQL\Controllers\Mappers\TypeMapperInterface; +use TheCodingMachine\GraphQL\Controllers\Reflection\CachedDocBlockFactory; use TheCodingMachine\GraphQL\Controllers\Types\UnionType; -use function is_object; use Iterator; use IteratorAggregate; use phpDocumentor\Reflection\DocBlock; @@ -28,7 +20,6 @@ use phpDocumentor\Reflection\Types\Array_; use phpDocumentor\Reflection\Types\Boolean; use phpDocumentor\Reflection\Types\Compound; -use phpDocumentor\Reflection\Types\ContextFactory; use phpDocumentor\Reflection\Types\Float_; use phpDocumentor\Reflection\Types\Iterable_; use phpDocumentor\Reflection\Types\Mixed_; @@ -82,6 +73,10 @@ class ControllerQueryProvider implements QueryProviderInterface * @var ContainerInterface */ private $registry; + /** + * @var CachedDocBlockFactory + */ + private $cachedDocBlockFactory; /** * @param object $controller @@ -94,7 +89,8 @@ class ControllerQueryProvider implements QueryProviderInterface */ public function __construct($controller, AnnotationReader $annotationReader, RecursiveTypeMapperInterface $typeMapper, HydratorInterface $hydrator, AuthenticationServiceInterface $authenticationService, - AuthorizationServiceInterface $authorizationService, ContainerInterface $registry) + AuthorizationServiceInterface $authorizationService, ContainerInterface $registry, + CachedDocBlockFactory $cachedDocBlockFactory) { $this->controller = $controller; $this->annotationReader = $annotationReader; @@ -103,6 +99,7 @@ public function __construct($controller, AnnotationReader $annotationReader, Rec $this->authenticationService = $authenticationService; $this->authorizationService = $authorizationService; $this->registry = $registry; + $this->cachedDocBlockFactory = $cachedDocBlockFactory; } /** @@ -149,8 +146,6 @@ public function getFields(): array private function getFieldsByAnnotations(string $annotationName, bool $injectSource): array { $refClass = new \ReflectionClass($this->controller); - $docBlockFactory = \phpDocumentor\Reflection\DocBlockFactory::createInstance(); - $contextFactory = new ContextFactory(); //$refClass = ReflectionClass::createFromInstance($this->controller); @@ -168,19 +163,8 @@ private function getFieldsByAnnotations(string $annotationName, bool $injectSour continue; } - $docComment = $refMethod->getDocComment() ?: '/** */'; - - // context is changing based on the class the method is declared in. - // we assume methods will be returned packed by classes so we do this little cache - if ($oldDeclaringClass !== $refMethod->getDeclaringClass()->getName()) { - $context = $contextFactory->createFromReflector($refMethod); - $oldDeclaringClass = $refMethod->getDeclaringClass()->getName(); - } - - $docBlockObj = $docBlockFactory->create($docComment, $context); - - // TODO: change CommentParser to use $docBlockObj methods instead. - $docBlock = new CommentParser($refMethod->getDocComment()); + $docBlockObj = $this->cachedDocBlockFactory->getDocBlock($refMethod); + $docBlockComment = $docBlockObj->getSummary()."\n".$docBlockObj->getDescription()->render(); $methodName = $refMethod->getName(); $name = $queryAnnotation->getName() ?: $methodName; @@ -202,7 +186,7 @@ private function getFieldsByAnnotations(string $annotationName, bool $injectSour $type = $this->mapReturnType($refMethod, $docBlockObj); } - $queryList[] = new QueryField($name, $type, $args, [$this->controller, $methodName], null, $this->hydrator, $docBlock->getComment(), $injectSource); + $queryList[] = new QueryField($name, $type, $args, [$this->controller, $methodName], null, $this->hydrator, $docBlockComment, $injectSource); } } @@ -256,8 +240,6 @@ private function getDocBlocReturnType(DocBlock $docBlock, \ReflectionMethod $ref private function getSourceFields(): array { $refClass = new \ReflectionClass($this->controller); - $docBlockFactory = \phpDocumentor\Reflection\DocBlockFactory::createInstance(); - $contextFactory = new ContextFactory(); /** @var SourceField[] $sourceFields */ $sourceFields = $this->annotationReader->getSourceFields($refClass); @@ -300,20 +282,12 @@ private function getSourceFields(): array throw FieldNotFoundException::wrapWithCallerInfo($e, $refClass->getName()); } - $docBlock = new CommentParser($refMethod->getDocComment()); - $methodName = $refMethod->getName(); - // context is changing based on the class the method is declared in. - // we assume methods will be returned packed by classes so we do this little cache - if ($oldDeclaringClass !== $refMethod->getDeclaringClass()->getName()) { - $context = $contextFactory->createFromReflector($refMethod); - $oldDeclaringClass = $refMethod->getDeclaringClass()->getName(); - } + $docBlockObj = $this->cachedDocBlockFactory->getDocBlock($refMethod); + $docBlockComment = $docBlockObj->getSummary()."\n".$docBlockObj->getDescription()->render(); - $docComment = $refMethod->getDocComment() ?: '/** */'; - $docBlockObj = $docBlockFactory->create($docComment, $context); $args = $this->mapParameters($refMethod->getParameters(), $docBlockObj); @@ -328,7 +302,7 @@ private function getSourceFields(): array $type = $this->mapReturnType($refMethod, $docBlockObj); } - $queryList[] = new QueryField($sourceField->getName(), $type, $args, null, $methodName, $this->hydrator, $docBlock->getComment(), false); + $queryList[] = new QueryField($sourceField->getName(), $type, $args, null, $methodName, $this->hydrator, $docBlockComment, false); } return $queryList; diff --git a/src/ControllerQueryProviderFactory.php b/src/ControllerQueryProviderFactory.php index 18beb5b..e4d0dde 100644 --- a/src/ControllerQueryProviderFactory.php +++ b/src/ControllerQueryProviderFactory.php @@ -6,6 +6,7 @@ use Psr\Container\ContainerInterface; use TheCodingMachine\GraphQL\Controllers\Mappers\RecursiveTypeMapperInterface; +use TheCodingMachine\GraphQL\Controllers\Reflection\CachedDocBlockFactory; use TheCodingMachine\GraphQL\Controllers\Security\AuthenticationServiceInterface; use TheCodingMachine\GraphQL\Controllers\Security\AuthorizationServiceInterface; @@ -31,16 +32,22 @@ class ControllerQueryProviderFactory * @var ContainerInterface */ private $registry; + /** + * @var CachedDocBlockFactory + */ + private $cachedDocBlockFactory; public function __construct(AnnotationReader $annotationReader, HydratorInterface $hydrator, AuthenticationServiceInterface $authenticationService, - AuthorizationServiceInterface $authorizationService, ContainerInterface $registry) + AuthorizationServiceInterface $authorizationService, ContainerInterface $registry, + CachedDocBlockFactory $cachedDocBlockFactory) { $this->annotationReader = $annotationReader; $this->hydrator = $hydrator; $this->authenticationService = $authenticationService; $this->authorizationService = $authorizationService; $this->registry = $registry; + $this->cachedDocBlockFactory = $cachedDocBlockFactory; } /** @@ -57,7 +64,8 @@ public function buildQueryProvider($controller, RecursiveTypeMapperInterface $ty $this->hydrator, $this->authenticationService, $this->authorizationService, - $this->registry + $this->registry, + $this->cachedDocBlockFactory ); } } diff --git a/src/Reflection/CachedDocBlockFactory.php b/src/Reflection/CachedDocBlockFactory.php new file mode 100644 index 0000000..ed2094f --- /dev/null +++ b/src/Reflection/CachedDocBlockFactory.php @@ -0,0 +1,100 @@ + + */ + private $docBlockArrayCache = []; + /** + * @var array + */ + private $contextArrayCache = []; + /** + * @var ContextFactory + */ + private $contextFactory; + + /** + * @param CacheInterface $cache The cache we fetch data from. Note this is a SAFE cache. It does not need to be purged. + * @param DocBlockFactory|null $docBlockFactory + */ + public function __construct(CacheInterface $cache, ?DocBlockFactory $docBlockFactory = null) + { + $this->cache = $cache; + $this->docBlockFactory = $docBlockFactory ?: DocBlockFactory::createInstance(); + $this->contextFactory = new ContextFactory(); + } + + /** + * Fetches a DocBlock object from a ReflectionMethod + * + * @param ReflectionMethod $refMethod + * @return DocBlock + */ + public function getDocBlock(ReflectionMethod $refMethod): DocBlock + { + $key = 'docblock_'.md5($refMethod->getDeclaringClass()->getName().'::'.$refMethod->getName()); + if (isset($this->docBlockArrayCache[$key])) { + return $this->docBlockArrayCache[$key]; + } + + if ($cacheItem = $this->cache->get($key)) { + [ + 'time' => $time, + 'docblock' => $docBlock + ] = $cacheItem; + + if (filemtime($refMethod->getFileName()) === $time) { + $this->docBlockArrayCache[$key] = $docBlock; + return $docBlock; + } + } + + $docBlock = $this->doGetDocBlock($refMethod); + + $this->cache->set($key, [ + 'time' => filemtime($refMethod->getFileName()), + 'docblock' => $docBlock + ]); + $this->docBlockArrayCache[$key] = $docBlock; + + return $docBlock; + } + + private function doGetDocBlock(ReflectionMethod $refMethod): DocBlock + { + $docComment = $refMethod->getDocComment() ?: '/** */'; + + $refClass = $refMethod->getDeclaringClass(); + $refClassName = $refClass->getName(); + + if (!isset($this->contextArrayCache[$refClassName])) { + $this->contextArrayCache[$refClassName] = $this->contextFactory->createFromReflector($refMethod); + } + + return $this->docBlockFactory->create($docComment, $this->contextArrayCache[$refClassName]); + } +} diff --git a/src/Reflection/CommentParser.php b/src/Reflection/CommentParser.php deleted file mode 100644 index f9f3d26..0000000 --- a/src/Reflection/CommentParser.php +++ /dev/null @@ -1,73 +0,0 @@ -getAllLinesWithoutStars($docBlock); - - // First, let's go to the first Annotation. - // Anything before is the pure comment. - - $oneAnnotationFound = false; - // Is the line an annotation? Let's test this with a regexp. - foreach ($lines as $line) { - if ($oneAnnotationFound === false && preg_match("/^[@][a-zA-Z]/", $line) === 0) { - $this->comment .= $line."\n"; - } else { - //$this->parseAnnotation($line); - $oneAnnotationFound = true; - } - } - - $this->comment = rtrim($this->comment, "\n"); - } - - /** - * @param string $docComment - * @return string[] - */ - private function getAllLinesWithoutStars(string $docComment): array - { - if (strpos($docComment, '/**') === 0) { - $docComment = substr($docComment, 3); - } - - // Let's remove all the \r... - $docComment = str_replace("\r", '', $docComment); - - $commentLines = explode("\n", $docComment); - $commentLinesWithoutStars = array_map(function(string $line) { - return ltrim($line, " \t*"); - }, $commentLines); - - // Let's remove the trailing /: - $lastComment = $commentLinesWithoutStars[count($commentLinesWithoutStars)-1]; - $commentLinesWithoutStars[count($commentLinesWithoutStars)-1] = substr($lastComment, 0, strlen($lastComment)-1); - - if ($commentLinesWithoutStars[count($commentLinesWithoutStars)-1] == "") { - array_pop($commentLinesWithoutStars); - } - - if (isset($commentLinesWithoutStars[0]) && $commentLinesWithoutStars[0] === '') { - $commentLinesWithoutStars = array_slice($commentLinesWithoutStars, 1); - } - return $commentLinesWithoutStars; - } - - /** - * Returns the comment out of a Docblock, ignoring all annotations - * - * @return string - */ - public function getComment(): string - { - return $this->comment; - } -} diff --git a/tests/AbstractQueryProviderTest.php b/tests/AbstractQueryProviderTest.php index c791310..af9bab8 100644 --- a/tests/AbstractQueryProviderTest.php +++ b/tests/AbstractQueryProviderTest.php @@ -14,6 +14,7 @@ use Mouf\Picotainer\Picotainer; use PHPUnit\Framework\TestCase; use Psr\Container\ContainerInterface; +use Symfony\Component\Cache\Simple\ArrayCache; use TheCodingMachine\GraphQL\Controllers\Fixtures\TestObject; use TheCodingMachine\GraphQL\Controllers\Fixtures\TestObject2; use TheCodingMachine\GraphQL\Controllers\Mappers\CannotMapTypeException; @@ -22,6 +23,7 @@ use TheCodingMachine\GraphQL\Controllers\Mappers\TypeMapperInterface; use TheCodingMachine\GraphQL\Controllers\Containers\EmptyContainer; use TheCodingMachine\GraphQL\Controllers\Containers\BasicAutoWiringContainer; +use TheCodingMachine\GraphQL\Controllers\Reflection\CachedDocBlockFactory; use TheCodingMachine\GraphQL\Controllers\Security\VoidAuthenticationService; use TheCodingMachine\GraphQL\Controllers\Security\VoidAuthorizationService; @@ -197,7 +199,8 @@ protected function buildControllerQueryProvider($controller) $this->getHydrator(), new VoidAuthenticationService(), new VoidAuthorizationService(), - $this->getRegistry() + $this->getRegistry(), + new CachedDocBlockFactory(new ArrayCache()) ); } @@ -216,7 +219,8 @@ protected function getControllerQueryProviderFactory(): ControllerQueryProviderF $this->getHydrator(), new VoidAuthenticationService(), new VoidAuthorizationService(), - $this->getRegistry()); + $this->getRegistry(), + new CachedDocBlockFactory(new ArrayCache())); } return $this->controllerQueryProviderFactory; } diff --git a/tests/ControllerQueryProviderTest.php b/tests/ControllerQueryProviderTest.php index 0c7973d..5c87a4a 100644 --- a/tests/ControllerQueryProviderTest.php +++ b/tests/ControllerQueryProviderTest.php @@ -13,6 +13,7 @@ use GraphQL\Type\Definition\StringType; use GraphQL\Type\Definition\ObjectType; use GraphQL\Type\Definition\UnionType; +use Symfony\Component\Cache\Simple\ArrayCache; use TheCodingMachine\GraphQL\Controllers\Fixtures\TestController; use TheCodingMachine\GraphQL\Controllers\Fixtures\TestControllerNoReturnType; use TheCodingMachine\GraphQL\Controllers\Fixtures\TestControllerWithArrayParam; @@ -31,6 +32,7 @@ use TheCodingMachine\GraphQL\Controllers\Containers\EmptyContainer; use TheCodingMachine\GraphQL\Controllers\Containers\BasicAutoWiringContainer; use TheCodingMachine\GraphQL\Controllers\Mappers\CannotMapTypeException; +use TheCodingMachine\GraphQL\Controllers\Reflection\CachedDocBlockFactory; use TheCodingMachine\GraphQL\Controllers\Security\AuthenticationServiceInterface; use TheCodingMachine\GraphQL\Controllers\Security\AuthorizationServiceInterface; use TheCodingMachine\GraphQL\Controllers\Security\VoidAuthenticationService; @@ -189,7 +191,8 @@ public function isLogged(): bool } }, new VoidAuthorizationService(), - new EmptyContainer() + new EmptyContainer(), + new CachedDocBlockFactory(new ArrayCache()) ); $fields = $queryProvider->getFields(); @@ -212,7 +215,8 @@ public function isAllowed(string $right): bool { return true; } - },new EmptyContainer() + },new EmptyContainer(), + new CachedDocBlockFactory(new ArrayCache()) ); $fields = $queryProvider->getFields(); @@ -268,7 +272,8 @@ public function testFromSourceFieldsInterface() $this->getHydrator(), new VoidAuthenticationService(), new VoidAuthorizationService(), - new EmptyContainer() + new EmptyContainer(), + new CachedDocBlockFactory(new ArrayCache()) ); $fields = $queryProvider->getFields(); $this->assertCount(1, $fields); diff --git a/tests/Integration/EndToEndTest.php b/tests/Integration/EndToEndTest.php index 022554f..5de3f6f 100644 --- a/tests/Integration/EndToEndTest.php +++ b/tests/Integration/EndToEndTest.php @@ -14,6 +14,7 @@ use function print_r; use Psr\Container\ContainerInterface; use Psr\Container\NotFoundExceptionInterface; +use Symfony\Component\Cache\Simple\ArrayCache; use Symfony\Component\Cache\Simple\NullCache; use TheCodingMachine\GraphQL\Controllers\AnnotationReader; use TheCodingMachine\GraphQL\Controllers\ControllerQueryProviderFactory; @@ -27,6 +28,7 @@ use TheCodingMachine\GraphQL\Controllers\QueryProviderInterface; use TheCodingMachine\GraphQL\Controllers\Containers\BasicAutoWiringContainer; use TheCodingMachine\GraphQL\Controllers\Containers\EmptyContainer; +use TheCodingMachine\GraphQL\Controllers\Reflection\CachedDocBlockFactory; use TheCodingMachine\GraphQL\Controllers\Schema; use TheCodingMachine\GraphQL\Controllers\Security\AuthenticationServiceInterface; use TheCodingMachine\GraphQL\Controllers\Security\AuthorizationServiceInterface; @@ -57,7 +59,8 @@ public function setUp() $container->get(HydratorInterface::class), $container->get(AuthenticationServiceInterface::class), $container->get(AuthorizationServiceInterface::class), - $container->get(BasicAutoWiringContainer::class) + $container->get(BasicAutoWiringContainer::class), + $container->get(CachedDocBlockFactory::class) ); }, BasicAutoWiringContainer::class => function(ContainerInterface $container) { @@ -97,6 +100,9 @@ public function hydrate(array $data, InputType $type) return new Contact($data['name']); } }; + }, + CachedDocBlockFactory::class => function() { + return new CachedDocBlockFactory(new ArrayCache()); } ]); } diff --git a/tests/Reflection/CachedDocBlockFactoryTest.php b/tests/Reflection/CachedDocBlockFactoryTest.php new file mode 100644 index 0000000..123d782 --- /dev/null +++ b/tests/Reflection/CachedDocBlockFactoryTest.php @@ -0,0 +1,28 @@ +getDocBlock($refMethod); + $this->assertSame('Fetches a DocBlock object from a ReflectionMethod', $docBlock->getSummary()); + $docBlock2 = $cachedDocBlockFactory->getDocBlock($refMethod); + $this->assertSame($docBlock2, $docBlock); + + $newCachedDocBlockFactory = new CachedDocBlockFactory($arrayCache); + $docBlock3 = $newCachedDocBlockFactory->getDocBlock($refMethod); + $this->assertEquals($docBlock3, $docBlock); + } +} diff --git a/tests/Reflection/CommentParserTest.php b/tests/Reflection/CommentParserTest.php deleted file mode 100644 index d855b2f..0000000 --- a/tests/Reflection/CommentParserTest.php +++ /dev/null @@ -1,23 +0,0 @@ -assertSame("Foo\nBar", $commentParser->getComment()); - } -} From fca5725853a7441a6530b98c8717010101a92249 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Tue, 11 Dec 2018 16:27:04 +0100 Subject: [PATCH 2/2] Fixing PHPStan --- src/ControllerQueryProvider.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ControllerQueryProvider.php b/src/ControllerQueryProvider.php index 36c85af..5e510e6 100644 --- a/src/ControllerQueryProvider.php +++ b/src/ControllerQueryProvider.php @@ -4,6 +4,7 @@ namespace TheCodingMachine\GraphQL\Controllers; use function array_merge; +use GraphQL\Type\Definition\InputType; use GraphQL\Type\Definition\OutputType; use phpDocumentor\Reflection\Fqsen; use phpDocumentor\Reflection\Types\Nullable;