From 960a437d46e6bc0df5049da0ae5e9a9f5d067516 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 16 Dec 2018 15:37:45 +0100 Subject: [PATCH 1/4] #7527 failing test case: `UnitOfWork#getSingleIdentifierValue()` should not be called for a well specified parameter type As previously reported by @flaushi in https://github.com/doctrine/doctrine2/pull/7471#discussion_r241949045, we discovered that binding a parameter causes a `ClassMetadataFactory#getClassMetadata()` call, which in turn leads to large performance regression when using any `object` type as parameter. Following two snippets lead to an internal `ClassMetadataFactory#getClassMetadata()` call, which in turn leads to an exception being thrown and garbage collected, plus multiple associated performance implications: ```php $query->setParameter('foo', new DateTime()); $query->getResult(); ``` ```php $query->setParameter('foo', new DateTime(), DateTimeType::NAME); $query->getResult(); ``` This is due to following portion of code: https://github.com/doctrine/doctrine2/blob/434820973cadf2da2d66e7184be370084cc32ca8/lib/Doctrine/ORM/Query.php#L406-L409 Notice how `$value = $this->processParameterValue($value);` happens before attempting to infer the type for the parameter value. That call leads to this segment being reached, which leads to the regression: https://github.com/doctrine/doctrine2/blob/434820973cadf2da2d66e7184be370084cc32ca8/lib/Doctrine/ORM/AbstractQuery.php#L423-L433 Assuming the bound parameter type is provided, we can completely skip attempting to introspect the given object: ```php $query->setParameter('foo', new DateTime(), DateTimeType::NAME); $query->getResult(); ``` Processing the parameter value is not needed in this case, so we can safely skip that logic for all known parameters. In order to not introduce a BC break or change the `AbstractQuery#processParameterValue()` implementation, we could filter out all parameters for which the type is given upfront, and later on merge them back in instead. The test expectation to be set is for `UnitOfWork#getSingleIdentifierValue()` to never be called. --- tests/Doctrine/Tests/ORM/Query/QueryTest.php | 30 ++++++++++++++++---- tests/Doctrine/Tests/OrmTestCase.php | 7 ++--- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Query/QueryTest.php b/tests/Doctrine/Tests/ORM/Query/QueryTest.php index 8a3caf66f05..0045647a09a 100644 --- a/tests/Doctrine/Tests/ORM/Query/QueryTest.php +++ b/tests/Doctrine/Tests/ORM/Query/QueryTest.php @@ -2,24 +2,26 @@ namespace Doctrine\Tests\ORM\Query; +use DateTime; use Doctrine\Common\Cache\ArrayCache; use Doctrine\Common\Collections\ArrayCollection; - -use Doctrine\DBAL\Cache\QueryCacheProfile; -use Doctrine\ORM\EntityManager; +use Doctrine\DBAL\Types\Type; use Doctrine\ORM\Internal\Hydration\IterableResult; use Doctrine\ORM\Query\Parameter; use Doctrine\ORM\Query\QueryException; +use Doctrine\ORM\UnitOfWork; use Doctrine\Tests\Mocks\DriverConnectionMock; +use Doctrine\Tests\Mocks\EntityManagerMock; use Doctrine\Tests\Mocks\StatementArrayMock; use Doctrine\Tests\Models\CMS\CmsAddress; use Doctrine\Tests\Models\CMS\CmsUser; +use Doctrine\Tests\Models\Generic\DateTimeModel; use Doctrine\Tests\OrmTestCase; class QueryTest extends OrmTestCase { - /** @var EntityManager */ - protected $_em = null; + /** @var EntityManagerMock */ + protected $_em; protected function setUp() { @@ -400,4 +402,22 @@ public function testResultCacheProfileCanBeRemovedViaSetter() : void self::assertAttributeSame(null, '_queryCacheProfile', $query); } + + /** @group 7527 */ + public function testValuesAreNotBeingResolvedForSpecifiedParameterTypes() : void + { + $unitOfWork = $this->createMock(UnitOfWork::class); + + $this->_em->setUnitOfWork($unitOfWork); + + $unitOfWork + ->expects(self::never()) + ->method('getSingleIdentifierValue'); + + $query = $this->_em->createQuery('SELECT d FROM ' . DateTimeModel::class . ' d WHERE d.datetime = :value'); + + $query->setParameter('value', new DateTime(), Type::DATETIME); + + self::assertEmpty($query->getResult()); + } } diff --git a/tests/Doctrine/Tests/OrmTestCase.php b/tests/Doctrine/Tests/OrmTestCase.php index 46e8ea143ee..6fdbe147110 100644 --- a/tests/Doctrine/Tests/OrmTestCase.php +++ b/tests/Doctrine/Tests/OrmTestCase.php @@ -11,6 +11,7 @@ use Doctrine\ORM\Configuration; use Doctrine\ORM\Mapping\Driver\AnnotationDriver; use Doctrine\Tests\Mocks; +use Doctrine\Tests\Mocks\EntityManagerMock; /** * Base testcase class for all ORM testcases. @@ -113,10 +114,8 @@ protected function createAnnotationDriver($paths = [], $alias = null) * @param mixed $conf * @param \Doctrine\Common\EventManager|null $eventManager * @param bool $withSharedMetadata - * - * @return \Doctrine\ORM\EntityManager */ - protected function _getTestEntityManager($conn = null, $conf = null, $eventManager = null, $withSharedMetadata = true) + protected function _getTestEntityManager($conn = null, $conf = null, $eventManager = null, $withSharedMetadata = true) : EntityManagerMock { $metadataCache = $withSharedMetadata ? self::getSharedMetadataCacheImpl() @@ -160,7 +159,7 @@ protected function _getTestEntityManager($conn = null, $conf = null, $eventManag $conn = DriverManager::getConnection($conn, $config, $eventManager); } - return Mocks\EntityManagerMock::create($conn, $config, $eventManager); + return EntityManagerMock::create($conn, $config, $eventManager); } protected function enableSecondLevelCache($log = true) From 23af164d7aae5e2ed31b5b93172af74b43f5551b Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 16 Dec 2018 16:20:26 +0100 Subject: [PATCH 2/4] Note: this will still lead to the `UnitOfWork#getSingleIdentifierValue()` still being called when not specifying the type of a DQL parameter being bound via `Doctrine\ORM\Query#setParameter()`: ```php $query->setParameter('foo', $theValue, $theType); ``` A full parameter bind is required in order to gain back performance: ```php $query->setParameter('foo', $theValue, $theType); ``` This is up for discussion with patch reviewers. --- lib/Doctrine/ORM/AbstractQuery.php | 6 +- lib/Doctrine/ORM/Query.php | 58 +++++++++++++------ lib/Doctrine/ORM/Query/Parameter.php | 17 +++++- tests/Doctrine/Tests/ORM/QueryBuilderTest.php | 5 +- 4 files changed, 63 insertions(+), 23 deletions(-) diff --git a/lib/Doctrine/ORM/AbstractQuery.php b/lib/Doctrine/ORM/AbstractQuery.php index 25a284ceb4c..0003c5d3ff5 100644 --- a/lib/Doctrine/ORM/AbstractQuery.php +++ b/lib/Doctrine/ORM/AbstractQuery.php @@ -72,7 +72,7 @@ abstract class AbstractQuery /** * The parameter map of this query. * - * @var \Doctrine\Common\Collections\ArrayCollection + * @var ArrayCollection|Parameter[] */ protected $parameters; @@ -306,7 +306,7 @@ public function free() /** * Get all defined parameters. * - * @return \Doctrine\Common\Collections\ArrayCollection The defined query parameters. + * @return ArrayCollection The defined query parameters. */ public function getParameters() { @@ -336,7 +336,7 @@ function (Query\Parameter $parameter) use ($key) : bool { /** * Sets a collection of query parameters. * - * @param \Doctrine\Common\Collections\ArrayCollection|array $parameters + * @param ArrayCollection|mixed[] $parameters * * @return static This query instance. */ diff --git a/lib/Doctrine/ORM/Query.php b/lib/Doctrine/ORM/Query.php index 525aa7a5081..2bc480a9b07 100644 --- a/lib/Doctrine/ORM/Query.php +++ b/lib/Doctrine/ORM/Query.php @@ -19,15 +19,18 @@ namespace Doctrine\ORM; +use Doctrine\Common\Collections\ArrayCollection; use Doctrine\DBAL\LockMode; +use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\ORM\Query\Exec\AbstractSqlExecutor; +use Doctrine\ORM\Query\Parameter; +use Doctrine\ORM\Query\ParameterTypeInferer; use Doctrine\ORM\Query\Parser; use Doctrine\ORM\Query\ParserResult; use Doctrine\ORM\Query\QueryException; -use Doctrine\ORM\Mapping\ClassMetadata; -use Doctrine\ORM\Query\ParameterTypeInferer; -use Doctrine\Common\Collections\ArrayCollection; use Doctrine\ORM\Utility\HierarchyDiscriminatorResolver; +use function array_keys; +use function assert; /** * A Query object represents a DQL query. @@ -387,26 +390,13 @@ private function processParameterMappings($paramMappings) $types = []; foreach ($this->parameters as $parameter) { - $key = $parameter->getName(); - $value = $parameter->getValue(); - $rsm = $this->getResultSetMapping(); + $key = $parameter->getName(); if ( ! isset($paramMappings[$key])) { throw QueryException::unknownParameter($key); } - if (isset($rsm->metadataParameterMapping[$key]) && $value instanceof ClassMetadata) { - $value = $value->getMetadataValue($rsm->metadataParameterMapping[$key]); - } - - if (isset($rsm->discriminatorParameters[$key]) && $value instanceof ClassMetadata) { - $value = array_keys(HierarchyDiscriminatorResolver::resolveDiscriminatorsForClass($value, $this->_em)); - } - - $value = $this->processParameterValue($value); - $type = ($parameter->getValue() === $value) - ? $parameter->getType() - : ParameterTypeInferer::inferType($value); + [$value, $type] = $this->resolveParameterValue($parameter); foreach ($paramMappings[$key] as $position) { $types[$position] = $type; @@ -439,6 +429,38 @@ private function processParameterMappings($paramMappings) return [$sqlParams, $types]; } + /** @return mixed[] tuple of (value, type) */ + private function resolveParameterValue(Parameter $parameter) : array + { + if ($parameter->typeWasSpecified()) { + return [$parameter->getValue(), $parameter->getType()]; + } + + $key = $parameter->getName(); + $originalValue = $parameter->getValue(); + $value = $originalValue; + $rsm = $this->getResultSetMapping(); + + assert($rsm !== null); + + if ($value instanceof ClassMetadata && isset($rsm->metadataParameterMapping[$key])) { + $value = $value->getMetadataValue($rsm->metadataParameterMapping[$key]); + } + + if ($value instanceof ClassMetadata && isset($rsm->discriminatorParameters[$key])) { + $value = array_keys(HierarchyDiscriminatorResolver::resolveDiscriminatorsForClass($value, $this->_em)); + } + + $processedValue = $this->processParameterValue($value); + + return [ + $processedValue, + $originalValue === $processedValue + ? $parameter->getType() + : ParameterTypeInferer::inferType($processedValue), + ]; + } + /** * Defines a cache driver to be used for caching queries. * diff --git a/lib/Doctrine/ORM/Query/Parameter.php b/lib/Doctrine/ORM/Query/Parameter.php index 39e2a7a4f21..6e968a1a9a7 100644 --- a/lib/Doctrine/ORM/Query/Parameter.php +++ b/lib/Doctrine/ORM/Query/Parameter.php @@ -19,6 +19,8 @@ namespace Doctrine\ORM\Query; +use function trim; + /** * Defines a Query Parameter. * @@ -49,6 +51,13 @@ class Parameter */ private $type; + /** + * Whether the parameter type was explicitly specified or not + * + * @var bool + */ + private $typeSpecified; + /** * Constructor. * @@ -58,7 +67,8 @@ class Parameter */ public function __construct($name, $value, $type = null) { - $this->name = trim($name, ':'); + $this->name = trim($name, ':'); + $this->typeSpecified = $type !== null; $this->setValue($value, $type); } @@ -104,4 +114,9 @@ public function setValue($value, $type = null) $this->value = $value; $this->type = $type ?: ParameterTypeInferer::inferType($value); } + + public function typeWasSpecified() : bool + { + return $this->typeSpecified; + } } diff --git a/tests/Doctrine/Tests/ORM/QueryBuilderTest.php b/tests/Doctrine/Tests/ORM/QueryBuilderTest.php index 1cf8ab1646f..17d10f4b2dc 100644 --- a/tests/Doctrine/Tests/ORM/QueryBuilderTest.php +++ b/tests/Doctrine/Tests/ORM/QueryBuilderTest.php @@ -609,8 +609,11 @@ public function testSetParameter() ->setParameter('id', 1); $parameter = new Parameter('id', 1, ParameterTypeInferer::inferType(1)); + $inferred = $qb->getParameter('id'); - $this->assertEquals($parameter, $qb->getParameter('id')); + self::assertSame($parameter->getValue(), $inferred->getValue()); + self::assertSame($parameter->getType(), $inferred->getType()); + self::assertFalse($inferred->typeWasSpecified()); } public function testSetParameters() From ca436f0baeff6f7bd51ec3003819ff38f13de5d8 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Wed, 19 Dec 2018 10:52:11 +0100 Subject: [PATCH 3/4] #7527 performance benchmark - verifying performance impact of inferred query parameter types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As an example result: ``` ./phpbench.phar run tests/Doctrine/Performance/Query --iterations=50 --revs=50 --report=aggregate PhpBench 0.15-dev (dcbe193). Running benchmarks. Using configuration file: /home/ocramius/Documents/doctrine/doctrine2/phpbench.json \Doctrine\Performance\Query\QueryBoundParameterProcessingBench benchExecuteParsedQueryWithInferredParameterTypeI49 P0 [μ Mo]/r: 643.684 634.664 (μs) [μSD μRSD]/r: 17.700μs 2.75% benchExecuteParsedQueryWithDeclaredParameterTypeI49 P0 [μ Mo]/r: 97.673 94.251 (μs) [μSD μRSD]/r: 8.259μs 8.46% 2 subjects, 100 iterations, 100 revs, 0 rejects, 0 failures, 0 warnings (best [mean mode] worst) = 88.460 [370.679 364.458] 127.400 (μs) ⅀T: 37,067.880μs μSD/r 12.980μs μRSD/r: 5.603% suite: 133f0e30090f815142331ebec6af18241694e7c0, date: 2018-12-19, stime: 10:47:10 +------------------------------------+--------------------------------------------------+--------+--------+------+-----+------------+-----------+-----------+-----------+-----------+----------+--------+-------+ | benchmark | subject | groups | params | revs | its | mem_peak | best | mean | mode | worst | stdev | rstdev | diff | +------------------------------------+--------------------------------------------------+--------+--------+------+-----+------------+-----------+-----------+-----------+-----------+----------+--------+-------+ | QueryBoundParameterProcessingBench | benchExecuteParsedQueryWithInferredParameterType | | [] | 50 | 50 | 5,970,568b | 604.680μs | 643.684μs | 634.664μs | 677.640μs | 17.700μs | 2.75% | 6.59x | | QueryBoundParameterProcessingBench | benchExecuteParsedQueryWithDeclaredParameterType | | [] | 50 | 50 | 5,922,424b | 88.460μs | 97.673μs | 94.251μs | 127.400μs | 8.259μs | 8.46% | 1.00x | +------------------------------------+--------------------------------------------------+--------+--------+------+-----+------------+-----------+-----------+-----------+-----------+----------+--------+-------+ ``` This indicates that the performance impact for NOT declaring parameter types explicitly is *MASSIVE*. --- .../Performance/EntityManagerFactory.php | 31 +++++++- .../QueryBoundParameterProcessingBench.php | 79 +++++++++++++++++++ 2 files changed, 109 insertions(+), 1 deletion(-) create mode 100644 tests/Doctrine/Performance/Query/QueryBoundParameterProcessingBench.php diff --git a/tests/Doctrine/Performance/EntityManagerFactory.php b/tests/Doctrine/Performance/EntityManagerFactory.php index fab5eba767c..d759506e46e 100644 --- a/tests/Doctrine/Performance/EntityManagerFactory.php +++ b/tests/Doctrine/Performance/EntityManagerFactory.php @@ -2,6 +2,10 @@ namespace Doctrine\Performance; +use Doctrine\Common\EventManager; +use Doctrine\DBAL\Cache\ArrayStatement; +use Doctrine\DBAL\Cache\QueryCacheProfile; +use Doctrine\DBAL\Connection; use Doctrine\DBAL\Driver\PDOSqlite\Driver; use Doctrine\ORM\Configuration; use Doctrine\ORM\EntityManager; @@ -20,7 +24,7 @@ public static function getEntityManager(array $schemaClassNames) : EntityManager $config->setAutoGenerateProxyClasses(ProxyFactory::AUTOGENERATE_EVAL); $config->setMetadataDriverImpl($config->newDefaultAnnotationDriver([ realpath(__DIR__ . '/Models/Cache'), - realpath(__DIR__ . '/Models/GeoNames') + realpath(__DIR__ . '/Models/GeoNames'), ], true)); $entityManager = EntityManager::create( @@ -36,4 +40,29 @@ public static function getEntityManager(array $schemaClassNames) : EntityManager return $entityManager; } + + public static function makeEntityManagerWithNoResultsConnection() : EntityManagerInterface + { + $config = new Configuration(); + + $config->setProxyDir(__DIR__ . '/../Tests/Proxies'); + $config->setProxyNamespace('Doctrine\Tests\Proxies'); + $config->setAutoGenerateProxyClasses(ProxyFactory::AUTOGENERATE_EVAL); + $config->setMetadataDriverImpl($config->newDefaultAnnotationDriver([ + realpath(__DIR__ . '/Models/Cache'), + realpath(__DIR__ . '/Models/Generic'), + realpath(__DIR__ . '/Models/GeoNames'), + ], true)); + + // A connection that doesn't really do anything + $connection = new class ([], new Driver(), null, new EventManager()) extends Connection + { + public function executeQuery($query, array $params = [], $types = [], QueryCacheProfile $qcp = null) + { + return new ArrayStatement([]); + } + }; + + return EntityManager::create($connection, $config); + } } diff --git a/tests/Doctrine/Performance/Query/QueryBoundParameterProcessingBench.php b/tests/Doctrine/Performance/Query/QueryBoundParameterProcessingBench.php new file mode 100644 index 00000000000..04e17167a07 --- /dev/null +++ b/tests/Doctrine/Performance/Query/QueryBoundParameterProcessingBench.php @@ -0,0 +1,79 @@ +parsedQueryWithInferredParameterType = $entityManager->createQuery($dql); + $this->parsedQueryWithDeclaredParameterType = $entityManager->createQuery($dql); + + foreach (range(1, 10) as $index) { + $this->parsedQueryWithInferredParameterType->setParameter('parameter' . $index, new DateTime()); + $this->parsedQueryWithDeclaredParameterType->setParameter('parameter' . $index, new DateTime(), DateTimeType::DATETIME); + } + + // Force parsing upfront - we don't benchmark that bit in this scenario + $this->parsedQueryWithInferredParameterType->getSQL(); + $this->parsedQueryWithDeclaredParameterType->getSQL(); + } + + public function benchExecuteParsedQueryWithInferredParameterType() : void + { + $this->parsedQueryWithInferredParameterType->execute(); + } + + public function benchExecuteParsedQueryWithDeclaredParameterType() : void + { + $this->parsedQueryWithDeclaredParameterType->execute(); + } +} + From a41f5673bc1b89b5ccac608287747289e35ce0ed Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Thu, 20 Dec 2018 22:45:54 +0100 Subject: [PATCH 4/4] #7527 automated CS checks --- tests/Doctrine/Performance/EntityManagerFactory.php | 7 ++++++- .../Query/QueryBoundParameterProcessingBench.php | 1 - 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/Doctrine/Performance/EntityManagerFactory.php b/tests/Doctrine/Performance/EntityManagerFactory.php index d759506e46e..0e891875db8 100644 --- a/tests/Doctrine/Performance/EntityManagerFactory.php +++ b/tests/Doctrine/Performance/EntityManagerFactory.php @@ -1,5 +1,7 @@ parsedQueryWithDeclaredParameterType->execute(); } } -