Prevent UnitOfWork lookup for DBAL types specified in Doctrine\ORM\Query#setParameter()
#7528
Conversation
…()` should not be called for a well specified parameter type As previously reported by @flaushi in doctrine#7471 (comment), 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.
Ocramius
force-pushed
the
fix/#7527-prevent-unit-of-work-lookup-for-known-value-types
branch
from
2f7508b
to
3702a71
Compare
…e()` 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.
Ocramius
force-pushed
the
fix/#7527-prevent-unit-of-work-lookup-for-known-value-types
branch
from
3702a71
to
23af164
Compare
|
I'm wondering if there is an actual way to prevent the regression without defined types though... |
@Ocramius the only way I can imagine now is to create a fitness function based on the benchmark results and assert that they are equal to a baseline (with some minor acceptable delta). However this would require a reliable and standardised environment to run them (at least manually), which we don't have at the moment. |
|
Nono, that's something I can unit test (besides the performance bench, which I'll do later): I don't have a technical solution to prevent processing parameters with inferred types. |
@Ocramius my point is mostly related to the fact that the bug fixed bug @alcaeus makes us always try to load the metadata of an object, which is the (only?) way we currently have to check if we're dealing with an entity or not. The optimisation point then (IMHO) is the validation of whether the given value is a potential entity or not, which is 100% valid but unrelated to this PR and might open a can of worms - one that is kind of being tackled in What am I not seeing? |
|
I'm just wondering if I can recover from the performance regression in this scenario (no specified type): $query->setParameter('foo', new DateTime());Not sure if this is possible, and patch doesn't touch this, but I'm open for suggestions if anybody sees a possible architectural solution. |
|
@Ocramius we had the previous performance because As far as I can see, the performance issues are now happening because we're going till the metadata driver and trying to load the metadata of an object that is NOT an entity only to ignore the exception and return the original object. The only think I can think of is to verify if an object is a potential entity or not without relying on class metadata factory - removing the need for throwing an unused exception (which also has performance implications). |
Which is a bit of an oxymoron: the only thing telling us if an object is an entity or not is class metadata. I think we can finalise the patch as-is and close it here. I'll try adding the benchmarks later today. |
I know, we can always add a nasty white list fated to rot through the years 🤣 |
… inferred query parameter types
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*.
|
@lcobucci this is ready to 🚢 The merge is intentional, since I was moving back and forth with the benchmark branch. |
|
@Ocramius Travis says no, though 😛 I'll review it in a bit. |
|
Yeah, CS issues, can fix myself later |
Ocramius
force-pushed
the
fix/#7527-prevent-unit-of-work-lookup-for-known-value-types
branch
from
cb079ec
to
a41f567
Compare
Ocramius
requested review from
lcobucci and
Majkl578
and removed request for
lcobucci
|
@lcobucci @Ocramius When is 2.6.4 gonna be tagged with this fix? Unfortunately we are "stuck" on 2.6.2 until this gets tagged as it breaks our application in the queries where we are binding DateTime objects (as the code on 2.6.3 now reaches our custom MappingDriver implementation and it throws an exception there). Seeing how this has such an impact is there any reason why 2.6.4 couldn't be tagged as soon as possible? |
lcobucci
changed the title
UnitOfWork lookup for DBAL types specified in Doctrine\ORM\Query#setParameter()UnitOfWork lookup for DBAL types specified in Doctrine\ORM\Query#setParameter()
Ocramius
deleted the
fix/#7527-prevent-unit-of-work-lookup-for-known-value-types
branch
…es (javiereguiluz) This PR was squashed before being merged into the 2.0.x-dev branch (closes #2946). Discussion ---------- Fixed some failing tests because of recent Doctrine changes Tries to fix some failing tests caused by this Doctrine change: doctrine/orm#7528 Commits ------- 5b9212e Fixed some failing tests because of recent Doctrine changes
v2.6.4 [](https://travis-ci.org/doctrine/orm) In this release we've fixes many bugs (including a performance regression) and made the v2.x series compatible with PHP 7.4. -------------------------------------------- - Total issues resolved: **11** - Total pull requests resolved: **32** - Total contributors: **30** Improvement ----------- - [7785: Fix "access array offset on value of type null" PHP 7.4 notices](doctrine#7785) thanks to @mlocati - [7142: Rename this repository to doctrine/orm](doctrine#7142) thanks to @greg0ire Bug ------------------ - [7821: Bug: doctrine#7820 paginator ignores dbal type conversions in identifiers](doctrine#7821) thanks to @Ocramius - [7778: Guard L2C regions against corrupted data](doctrine#7778) thanks to @umpirsky - [7767: PersistentCollection::matching() does not respect the collections native sorting](doctrine#7767) thanks to @stephanschuler - [7766: Respect collection orderBy meta when matching()](doctrine#7766) thanks to @stephanschuler - [7761: Do not modify UOW on PersistentCollection::clear() when owner has DEFFERED&doctrine#95;EXPLICIT change tracking policy](doctrine#7761) thanks to @paxal - [7750: Fix incorrect return of null values in L2C](doctrine#7750) thanks to @AlexSmerw - [7737: Fix MEMBER&doctrine#95;OF comparison when using criteria in query builder](doctrine#7737) thanks to @Smartel1 - [7735: Null in fields value in Cached Entity several times on day on high-load project.](doctrine#7735) thanks to @AlexSmerw - [7630: Fix doctrine#7629 - `scheduledForSynchronization` leaks memory when using `@ORM\ChangeTrackingPolicy("DEFERRED&doctrine#95;EXPLICIT")`](doctrine#7630) thanks to @yethee - [7528: Prevent `UnitOfWork` lookup for DBAL types specified in `Doctrine\ORM\Query#setParameter()`](doctrine#7528) thanks to @Ocramius - [7322: JoinedSubclassPersister pass identifier types on delete](doctrine#7322) thanks to @dennisenderink and @fred-jan - [7266: Call to a member function resolveAssociationEntries() on boolean {"detail":"&doctrine#91;object&doctrine#93; (Symfony\\Component\\Debug\\Exception\\FatalThrowableError(code: 0): Call to a member function resolveAssociationEntries() on boolean at /www/vendor/doctrine/orm/lib/Doctrine/ORM/Cache/DefaultQueryCache.php:140)"}](doctrine#7266) thanks to @mingmingxianseng - [4632: DDC-3789: Paginator does not convert entity ids if they are value objects](doctrine#4632) thanks to @doctrinebot Documentation ------------- - [7818: Add note into docs about not using SimpleAnnotationReader](doctrine#7818) thanks to @SenseException - [7791: Fix preFlush event documentation stating incorrectly that flush can be called safely](doctrine#7791) thanks to @Steveb-p - [7753: Add ORM annotations in getting-started docs](doctrine#7753) thanks to @SenseException and @wajdijurry - [7744: Fixed a typo-error](doctrine#7744) thanks to @noobshow - [7732: &doctrine#91;Documentation&doctrine#93; Missing comma fix](doctrine#7732) thanks to @lchrusciel - [7729: Update DATE&doctrine#95;ADD and DATE&doctrine#95;SUB docs](doctrine#7729) thanks to @JoppeDC - [7672: Added cross-links to relevant documentation](doctrine#7672) thanks to @jschaedl - [7612: Update ordered-associations.rst](doctrine#7612) thanks to @spirlici - [7610: Change APC to OPcache in improving-performance.rst ](doctrine#7610) thanks to @smtchahal - [7596: Correct method names and broken link in docs](doctrine#7596) thanks to @mbessolov - [7577: Fix of single link to dbal docs in advanced-configuration.rst](doctrine#7577) thanks to @SenseException - [7572: Remove codeigniter Framework example](doctrine#7572) thanks to @SenseException - [7571: Fix typo in inheritance mappings docs](doctrine#7571) thanks to @batwolf - [7557: Change Stackoverflow tag to doctrine-orm](doctrine#7557) thanks to @malarzm - [7551: &doctrine#91;2.6&doctrine#93; Migrate repository name doctrine/doctrine2 -> doctrine/orm](doctrine#7551) thanks to @Majkl578 - [7530: Documentation error typo fix: s/Used-defined/User-Defined](doctrine#7530) thanks to @vladyslavstartsev - [7519: doctrine#7518 Fixed type mismatch between `EntityRepository#&doctrine#95;&doctrine#95;construct()` and its documented constructor arguments](doctrine#7519) thanks to @koftikes - [7518: `EntityRepository::&doctrine#95;&doctrine#95;construct()` expects `Doctrine\ORM\EntityManager` instead of actual required `EntityManagerInterface`](doctrine#7518) thanks to @koftikes - [7490: Fix broken link](doctrine#7490) thanks to @vladyslavstartsev - [7483: Fixed a minor syntax issue](doctrine#7483) thanks to @javiereguiluz CI ----------------- - [7794: Fix test compatibility with DBAL 2.10.x-dev](doctrine#7794) thanks to @lcobucci - [7731: Replace custom install script with add-on](doctrine#7731) thanks to @greg0ire - [7473: Incremental CS checks in 2.x branches](doctrine#7473) thanks to @Majkl578
](https://travis-ci.org/doctrine/orm){kind=link}
Fixes #7527