Use NullCacheItemPool when using a Paginator. #10095
Conversation
a825c28 to
691fbf6
Compare
89722a6 to
161701b
Compare
|
I think most tests are failing on php 8+ because of some return type hints that don't exist in php < 8. How do you want to handle that? |
"as possible" was a keyword here 😉 When not possible, use phpdoc instead please :) |
This fixes a bug where Doctrine would recreate the DQL to SQL parsing cache on every request which would fill up OPCache with duplicates. We now use a "Null" cache implementation instead of calling Query::expireQueryCache(). @see doctrine#9917
161701b to
7d9a048
Compare
|
🤔 it does not seem you can fix it unless you only fix it for PHP 8 only 😓 |
All PSR Cache implementations should be flagged as That being said, I'd really think we should try to fix that issue without shipping our own PSR Cache implementation. |
Yeah that's what I thought. |
Do you think we should require symfony/cache so that we could use |
Yes well, we know the cause and we're smarter than PHPStan in that case. There's no shame in ignoring false positives.
That's not an option. |
Do you have a solution for fixing this without making our own PSR cache implementation? |
|
I don't and I haven't investigated the issue tbh. But if you're rolling your own cache implementation to fix a paginator bug, I'm confident that you're using a sledgehammer to crack a nut. There has to be a better way. |
Well I agree in a sense. The original call to |
|
A suggested fix is in #10434. |
This PR prevents the Paginator from causing OpCache "wasted memory" to increase _on every request_ when used with Symfony's `PhpFilesAdapter` as the cache implementation for the query cache. Depending on configured thresholds, wasted memory this will either cause periodic opcache restarts or running out of memory and not being able to cache additional scripts ([Details](https://tideways.com/profiler/blog/fine-tune-your-opcache-configuration-to-avoid-caching-suprises)). Fixes doctrine#9917, closes doctrine#10095. There is a long story (doctrine#7820, doctrine#7821, doctrine#7837, doctrine#7865) behind how the Paginator can take care of DBAL type conversions when creating the pagination query. This conversion has to transform identifier values before they will be used as a query parameter, so it has to happen every time the Paginator is used. For reasons, this conversion happens inside `WhereInWalker`. Tree walkers like this are used only during the DQL parsing/AST processing steps. Having a DQL query in the query cache short-cuts this step by fetching the parsing/processing result from the cache. So, to make sure the conversion happens also with the query cache being enabled, this line https://github.com/doctrine/orm/blob/1753d035005c1125c9fb4855c3fa629341e5734d/lib/Doctrine/ORM/Tools/Pagination/Paginator.php#L165 was added in doctrine#7837. It causes `\Doctrine\ORM\Query::parse()` to re-parse the query every time, but will also put the result into the query cache afterwards. At this point, the setup described in doctrine#9917 – which, to my knowledge, is the default in Symfony + DoctrineBundle projects – will ultimately bring us to this code: https://github.com/symfony/symfony/blob/4b3391725f2fc4a072e776974f00a992cbc70515/src/Symfony/Component/Cache/Adapter/PhpFilesAdapter.php#L248-L249 When writing a cache item with an already existing key, the driver has to make sure the opcache will honor the changed PHP file. This is what causes _wasted memory_ to increase. Instead of using `\Doctrine\ORM\Query::expireQueryCache()`, which will force `\Doctrine\ORM\Query::parse()` to parse the query again before putting it into the cache, use `\Doctrine\ORM\Query::useQueryCache(false)`. The subtle difference is the latter will not place the processed query in the cache in the first place. A test case is added to check that repeated use of the paginator does not call the cache to update existing keys. That should suffice to make sure we're not running into the issue, while at the same time not complicating tests by using the `PhpFilesAdapter` directly. Note that in order to observe the described issue in tests, you will need to use the `PhpFilesDriver` and also make sure that OpCache is enabled and also activated for `php-cli` (which is running the unit tests). This particular subquery generated/used by the Paginator is not put into the query cache. The DQL parsing/to-SQL conversion has to happen _every time_ the Paginator is used. This, however, was already the case before this PR. In other words, this PR only changes that we do not store/update the cached result every time, but instead completely omit caching the query.
This fixes a bug where Doctrine would recreate the DQL to SQL parsing
cache on every request which would fill up OPCache with duplicates.
We now use a "Null" cache implementation instead of calling
Query::expireQueryCache().
@see #9917