Skip to content

Commit

Permalink
As previously reported by @flaushi in #7471 (comment), we discovered
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Ocramius authored and guilhermeblanco committed Jun 12, 2019
1 parent 05799d8 commit a8f9e25
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 5 deletions.
25 changes: 23 additions & 2 deletions tests/Doctrine/Tests/ORM/Query/QueryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,25 @@

namespace Doctrine\Tests\ORM\Query;

use DateTime;
use Doctrine\Common\Cache\ArrayCache;
use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\DBAL\ParameterType;
use Doctrine\ORM\EntityManagerInterface;
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 EntityManagerInterface */
/** @var EntityManagerMock */
protected $em;

protected function setUp() : void
Expand Down Expand Up @@ -403,4 +406,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());
}
}
8 changes: 5 additions & 3 deletions tests/Doctrine/Tests/OrmTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Mapping\Driver\AnnotationDriver;
use Doctrine\ORM\Proxy\Factory\ProxyFactory;
use Doctrine\Tests\Mocks;
use function is_array;
use function realpath;

Expand Down Expand Up @@ -78,17 +79,18 @@ protected function createAnnotationDriver($paths = [])
* for a particular test,
*
* @param Connection|array $conn
* @param mixed $conf
* @param EventManager|null $eventManager
* @param bool $withSharedMetadata
*
* @return EntityManagerInterface
* @return Mocks\EntityManagerMock
*/
protected function getTestEntityManager(
$conn = null,
$conf = null,
$eventManager = null,
$withSharedMetadata = true
) {
) : Mocks\EntityManagerMock {
$metadataCache = $withSharedMetadata
? self::getSharedMetadataCacheImpl()
: new ArrayCache();
Expand Down Expand Up @@ -129,7 +131,7 @@ protected function getTestEntityManager(
$conn = DriverManager::getConnection($conn, $config, $eventManager);
}

return Mocks\EntityManagerMock::create($conn, $config, $eventManager)->getWrappedEntityManager();
return Mocks\EntityManagerMock::create($conn, $config, $eventManager);
}

protected function enableSecondLevelCache($log = true)
Expand Down

0 comments on commit a8f9e25

Please sign in to comment.